review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily

[Neil Toda]

Great.  Thanks.

On 4/29/2014 9:45 AM, Kumar Srinivasan wrote:
>
> On 4/29/2014 8:50 AM, Neil Toda wrote:
>>
>> Thanks Kumar.  Will check these.
>>
>> I have FXLauncherTest.java open right now as I type.  I started 
>> looking at it yesterday.  Good
>> that I am looking at the right test case.  Good organization.  I 
>> might also have to add a
>> non FX jar file to test.. unless I've missed that.
>
> there is already a nonFX jar being created, see:
> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/3e8e199e23b2/test/tools/launcher/FXLauncherTest.java#l360 
>
> lines 358-361, all you have to do is add the following, and add the 
> JBS-id in the @bug list.
>
>         if (tr.testStatus) {
>             if (!tr.notContains("jfxrt.jar")) {
>                 System.out.println("testing for extraneous jfxrt jar");
>                 System.out.println(tr);
>                 throw new Exception("jfxrt.jar is being loaded, it 
> should not be!");
>             }
> +           if 
> (!tr.notContains("sun.launcher.LauncherHelper\$FXHelper")) {  // this 
> is a regex
> +System.out.println("testing for extraneous FXhelper");
> +               System.out.println(tr);
> +               throw new Exception("jfxrt.jar is being loaded, it 
> should not be!");
> +           }
>             for (String p : APP_PARMS) {
>                 if (!tr.contains(p)) {
>                     System.err.println("ERROR: Did not find "
>                             + p + " in output!");
>                 }
>             }
>         }
>
> Hope this helps.
>
> Kumar
>>
>> -neil
>>
>>
>>
>>
>> On 4/29/2014 7:17 AM, Kumar Srinivasan wrote:
>>> Neil,
>>>
>>> The changes looks satisfactory, except for a few style nits:
>>> 1. JAVAFX_FXHELPER_CLASS_NAME_SUF -> JAVAFX_FXHELPER_CLASS_NAME_SUFFIX
>>> // 3 more characters won't make much of a difference
>>>
>>> 2. FXHelper.setFXLaunchParameters(what,mode);
>>> // missing space after comma.
>>>
>>>
>>> A  Launcher regression test is required, which ensure that the 
>>> launcher does not load
>>> FXLauncher inadvertently in the future. ie. a regression test.
>>>
>>> You can do this by adding another condition in the method 
>>> testExtraneousJars at:
>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/3e8e199e23b2/test/tools/launcher/FXLauncherTest.java#l360 
>>>
>>> just like jfxrt.jar is being tested now.
>>>
>>> Kumar
>>>
>>> On 4/25/2014 6:55 PM, Neil Toda wrote:
>>>>
>>>> Thanks Kevin.  -neil
>>>>
>>>> On 4/25/2014 8:22 AM, Kevin Rushforth wrote:
>>>>> The code changes looks fine to me. Also, I ran all JavaFX unit 
>>>>> tests with no problems (at least none relating to launching).
>>>>>
>>>>> -- Kevin
>>>>>
>>>>>
>>>>> Neil Toda wrote:
>>>>>> Webrev
>>>>>>
>>>>>>     http://cr.openjdk.java.net/~ntoda/8035782/
>>>>>>
>>>>>> for bug
>>>>>>
>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8035782
>>>>>>
>>>>>> The file : ./jdk/src/share/classes/sun/launcher/LauncherHelper.java
>>>>>>
>>>>>> has been modified so that the inner class FXHelper is not loaded 
>>>>>> unnecessarily.
>>>>>> FXHelper, which is needed to make initializations for any JavaFX 
>>>>>> application, was
>>>>>> being loaded for all applications.
>>>>>>
>>>>>> The fix was straight forward, with the lifting of one method and 
>>>>>> several static
>>>>>> strings into FXHelper's superclass, LauncherHelper.
>>>>>>
>>>>>> Kevin Rushforth supplied three tests of applications not in jar 
>>>>>> files.  These
>>>>>> needed to be explicitly tested.  These tests require the JavaFX 
>>>>>> bundle in the
>>>>>> build, and the return code 2 signifies success.
>>>>>>
>>>>>> Launcher tests for jtreg: ./jdk/test/tools/launcher passed on 
>>>>>> Windows 7 64 and Oracle-Linux6-64.
>>>>>>
>>>>>> JPRT tests were run and passed on scv3.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> -neil
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>