[rfc][icedtea-web] try to properly(?) react on java/j2se version

[Jie Kang]

----- Original Message -----
> On 02/03/2015 04:40 PM, Jiri Vanek wrote:
> > On 02/02/2015 08:35 PM, Jie Kang wrote:
> >>
> >>
> >> ----- Original Message -----
> >>> hi!
> >>>
> >>> I was recently acknowledged with ITW misbehaving when interpretting
> >>> java/j2se's version attribute.
> >>> When they "dont match" then itw simply continues(???)
> >>>
> >>> The useces I was informed about, was, that the application was known not
> >>> to
> >>> run on jdk7
> >>>
> >>> So they included `version="1.6.0"` and itw happily runs without any
> >>> warning.
> >>>
> >>>
> >>> I'm adding reaction - in strict mode dye, in non strict mode popup
> >>> window.
> >>>
> >>> Actually  - I'm absolutely unsure what to do with this case...
> >>
> >> Hello,
> >>
> >>
> >> Is there a website I can go for testing this? Or could you create a
> >> reproducer for this?
> >
> > one BAD example is
> > http://argouml.tigris.org/files/documents/4/383/ArgoUML-stable.jnlp
> >
> > the behavior of this appis exactly why not to do what i did.
> >   - forgoten jnlp app
> >   - rewuires exactly 1.4.whatever
> >   => unwonted dialog rise up
> >
> >
> >>
> >>
> >> +    public static class JreVersion extends Version{
> >>
> >> I'd strongly prefer this to be in it's own file along with it's own tests.
> >
> > hmhhm.. why? :(( It is so single case, that I dont wont to puboic it more.
> >>
> >> As well can you add some comments to the differences between JreVersion
> >> and Version. When do we
> >> use Version instead of JreVersion and when do we use JreVersion instead of
> >> Version? It isn't 100%
> >> clear to me at the moment;;
> >>
> >> +                    throw new RuntimeException("Strict run is  deffined,
> >> and your JRE - " +
> >> getJreVersion() + " - dont match requested JRE - " + v);
> >>
> >> s/deffined/defined
> >>
> >> Should this string be localized?
> > nope - localised exceptions are root of another evil.
> > But I will localize the message which goes to dialogue - if some dialogue
> > appears at all.
> >>
> >> +        //this fails, do we need workaround?
> >> +        //Assert.assertTrue(new Version("1.5-").matches("1.4"));
> >>
> >> I think rather than workaround, a patch to the Version class would be
> >> appropriate.
> >
> > I will change it to:
> >   +        //this fails, do we need patch it?
> >   +        //Assert.assertTrue(new Version("1.5-").matches("1.4"));
> >>
> >>
> >
> > My concerns are that - is nowhere speciifed.. and dont give much sense...
> >
> >
> >
> > A way - I'm thinking of two changes
> >
> > for this patch:
> >   when invalid jdk is detected
> >   - strict
> >     - warning to console
> >     - strict mode -  dialogue popup, with "run: yes/no"
> >   - nostrict,
> >    - only warning to console
> >
> > Corrct jdk detected
> >   - jsut info to console.
> >
> >
> >
> 
> here its. What do you think?

Hello,

A few nits:

diff -r 6d7f7e0e3829 netx/net/sourceforge/jnlp/Version.java
--- a/netx/net/sourceforge/jnlp/Version.java	Wed Feb 11 09:56:37 2015 +0100
+++ b/netx/net/sourceforge/jnlp/Version.java	Wed Feb 11 17:17:54 2015 +0100
@@ -17,6 +17,10 @@
 package net.sourceforge.jnlp;
 
 import java.util.*;

Can you fix this mass import as well?


+     * This is special case of version, used only for checking jre version. If
+     * jre do not match, in strict not-headless mode the dialog with
+     * confirrmation appears If jre do not match, in strict headless mode the
+     * exception is thrown If jre match, or non-strict mode is run, then only
+     * message is printed

The comment is good but a little hard to read. Can you change it to a bullet-point format like:

     * This is special case of version, used only for checking jre version. 
     * If JRE doesn't match in strict mode: 
     *   Headless: An exception is thrown
     *   Non-headless: Confirmation dialog appears warning users
     * If JRE matches and/or we are in non-strict mode:
     *   Warning message is printed

diff -r 6d7f7e0e3829 netx/net/sourceforge/jnlp/resources/Messages.properties
--- a/netx/net/sourceforge/jnlp/resources/Messages.properties	Wed Feb 11 09:56:37 2015 +0100
+++ b/netx/net/sourceforge/jnlp/resources/Messages.properties	Wed Feb 11 17:17:54 2015 +0100
@@ -54,6 +54,11 @@
 AboutDialogueTabNews=News
 AboutDialogueTabGPLv2=GPLv2
 
+# version check minidialogue
+JREversionDontMatch=Warning - your JRE - {0} - don't match requested JRE - {1}

The grammar/spelling here is a little off: could you apply these changes?
s/don't match/doesn't match the

+JREContinueDialogSentence2=Do you wont to continue in running?

s/wont/want
s/continue in/continue


diff -r 6d7f7e0e3829 tests/netx/unit/net/sourceforge/jnlp/VersionTest.java
--- a/tests/netx/unit/net/sourceforge/jnlp/VersionTest.java	Wed Feb 11 09:56:37 2015 +0100
+++ b/tests/netx/unit/net/sourceforge/jnlp/VersionTest.java	Wed Feb 11 17:17:54 2015 +0100
@@ -95,4 +95,53 @@
 
 
     }
+    
+    

Could you remove the extra spaces?

+      @Test
+    public void cornerCases() {

Can you remove the extra tab on @Test?

+        Assert.assertTrue(new Version("1.5").matches("1.5"));
+        Assert.assertTrue(new Version("1.5+").matches("1.5"));
+        Assert.assertTrue(new Version("1.5+").matches("1.6"));
+        Assert.assertFalse(new Version("1.5+").matches("1.4"));
+        Assert.assertFalse(new Version("1.5").matches("1.4"));
+        Assert.assertFalse(new Version("1.5").matches("1.6"));
+    }

+    @Test(expected = RuntimeException.class)
+    public void jreVersionTestFails1() {
+        //no exception occures

+    @Test(expected = RuntimeException.class)
+    public void jreVersionTestFails2() {
+        //no exception occures

For these two tests, the comment doesn't seem to apply as the test expects an exception. Could you remove the comment?


Everything else looks fine.


Cheers,

> 
> 
> J.
> >
> >
> > For future:
> >   - I think ITW needs some mechanism to rember decision on those swarms of
> >   dialogues
> >   - some big work here will be needed and I started to work on prototype
> >
> >
> > J.
> >
> >
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern