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

[Andrew Azores]

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 :)

-- 
Andrew Azores