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

[Jie Kang]

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


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.



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
> 

-- 

Jie Kang