[rfc][icedtea-web] PolicyEditor gains a real parser

[Andrew Azores]

On 01/15/2015 01:54 PM, Andrew Azores wrote:
> On 01/15/2015 01:51 PM, Jie Kang wrote:
>>
>>
>> ----- Original Message -----
>>> On 01/05/2015 04:27 PM, Jie Kang wrote:
>>>> Hello,
>>>>
>>>> Long time no see :D
>>>>
>>>> After reading over the patch a few times, I don't have any major
>>>> concerns
>>>> with the code. I haven't noticed any bugs while user-testing as well.
>>>> Great work!
>>>>
>>>>
>>>> One suggestion below and a question:
>>>>
>>>> diff --git
>>>> a/netx/net/sourceforge/jnlp/security/policyeditor/KeystoreInfo.java
>>>> b/netx/net/sourceforge/jnlp/security/policyeditor/KeystoreInfo.java
>>>> new file mode 100644
>>>> --- /dev/null
>>>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/KeystoreInfo.java
>>>> @@ -0,0 +1,113 @@
>>>> [...]
>>>> +    @Override
>>>> +    public int hashCode() {
>>>> +        int result = keyStoreUrl.hashCode();
>>>> +        result = 31 * result + keyStoreType.hashCode();
>>>> +        result = 31 * result + keyStoreProvider.hashCode();
>>>> +        result = 31 * result + keyStorePasswordUrl.hashCode();
>>>> +        return result;
>>>> +    }
>>>>
>>>> I'd suggest replacing this code with usage of the Objects.hash() :
>>>> http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#hash(java.lang.Object...)
>>>>
>>>>
>>>> It performs a extremely similar algorithm as the above but has the
>>>> advantage of looking nicer. One of the answers in this thread shows
>>>> what
>>>> code it ends up running:
>>>> http://stackoverflow.com/questions/13785353/equals-and-hashcode-is-objects-hash-method-broken
>>>>
>>>>
>>>
>>> Nice catch. I'd gotten used to using Apache HashCodeBuilder and when
>>> that wasn't available here, I just let my IDE generate it for me ;)
>>>
>>>>
>>>> diff --git
>>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>>> --- a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>>> @@ -63,39 +63,16 @@
>>>> [...]
>>>> +import javax.swing.*;
>>>>
>>>> Is this package import intended? I think individual imports would be
>>>> preferable (http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style)
>>>>
>>>>
>>>> Everything else is fine to me.
>>>
>>> Oops... I blame my IDE again! That was definitely not intentional.
>>>
>>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> ----- Original Message -----
>>>>> Hi all,
>>>>>
>>>>> Long time no see!
>>>>>
>>>>> I started this patch several months ago, but my school work became far
>>>>> too heavy and I had to put off any further work to finish off this
>>>>> patch. Today I've managed to finish it to a point that I think it's
>>>>> worth submitting for review.
>>>>>
>>>>> It's a big, long patch, so review will probably take quite some time,
>>>>> and may be difficult at times. I know I've appeared to be completely
>>>>> gone for many weeks, but I have actually been checking on my emails
>>>>> fairly often, just haven't had time to reply to any patch reviews. But
>>>>> right now I should have at least a few weeks before school really
>>>>> picks
>>>>> back up again.
>>>>>
>>>>> The gist of this patch is basically that I discovered that
>>>>> policytool's
>>>>> parser is actually available to use without any dirty reflection hacks
>>>>> or anything like that, but it's hidden away and not really documented,
>>>>> and is marked as an internal API. But it's open and so I think the
>>>>> worst
>>>>> case scenario if it's removed later is that we just fork and
>>>>> maintain an
>>>>> older copy. It's much easier and IMHO smarter than writing a whole new
>>>>> parser from scratch to duplicate the one that policytool uses, anyhow.
>>>>> This patch just rips out the old, crappy code that used horrible regex
>>>>> to try to parse policy files, and plops in the policytool parser
>>>>> instead. Anything else pretty much comes down to reconciling the
>>>>> existing PolicyEditor code and structures with the new ones that the
>>>>> parser gives and expects.
>>>>>
>>>>> Nice side effect: all policyeditor tests now pass, even the tricky
>>>>> comments handling ones ;)
>>>>>
>>>>> Thanks,
>>>>> --
>>>>> Andrew Azores
>>>>>
>>>>
>>>
>>> New patch attached that addresses your two nits, and nothing else.
>>
>> Hey,
>>
>> Looks good to me. +1. Up to you if you'd like another set of eyes
>> before pushing. Heads up if you're not aware: jvanek and ldracz are
>> both on PTO till this coming Monday Jan 19th.
>>
>>
>> Regards,
>>
>>>
>>> Thanks,
>>> --
>>> Andrew Azores
>>>
>>
>
> Okay, thanks for the notice. I think I will wait for another pair of
> eyes on this before pushing - really don't want to accidentally break
> PolicyEditor :)
>

Ultra ping! ;)

Thanks,
-- 
Andrew Azores