Re: review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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
Re: review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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
Re: review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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. -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
Re: review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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
Re: review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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
Re: review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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
review request: 8035782 : sun/launcher/LauncherHelper$FXHelper loaded unnecessarily
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
