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

[Kumar Srinivasan]

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