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

[Andrew Azores]

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.

Thanks,
-- 
Andrew Azores
-------------- next part --------------
A non-text attachment was scrubbed...
Name: policyeditor-real-parser-2.patch
Type: text/x-patch
Size: 183226 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150115/b93fc616/policyeditor-real-parser-2-0001.patch>