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

[Kumar Srinivasan]

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