review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily (original) (raw)

Neil Toda neil.toda at oracle.com
Tue Apr 29 16:49:07 UTC 2014

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 : APPPARMS) { 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. JAVAFXFXHELPERCLASSNAMESUF -> JAVAFXFXHELPERCLASSNAMESUFFIX // 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

More information about the core-libs-dev mailing list