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

[Jie Kang]

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

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern