1) Redundant return removed. 2) Kumar and I talked: Ever since the pentium bug, 1 in 80 trillion seems a big risk .. so the code was changed to check for equality with the whole class name: private static final String JAVAFX_FXHELPER_CLASS_NAME_SUFFIX = "sun.launcher.LauncherHelper$FXHelper"; ... if (JAVAFX_FXHELPER_CLASS_NAME_SUFFIX.equals(mainClass.getName()) || doesExtendFXApplication(mainClass)) { // Will abort() if there are problems with FX runtime FXHelper.setFXLaunchParameters(what, mode); return FXHelper.class; } Thanks -neil On 5/1/2014 8:05 AM, David DeHaven wrote:

Do we care about the 1 in more than 80 trillion case where the third party Main-Class would be named "LauncherHelper$FXHelper"? I think the probability is extremely unlikely so I'm fine with it the way it's written.

LauncherHelper.java: 590 return;

Redundant return statement?

-DrD-

...

For completeness the bugid line needs the bugid as shown, otherwise SQE will open another bug to have you fix this.

-26 * @bug 8001533 8004547 +26 * @bug 8001533 8004547 8035782

other than that it looks good, I can push this with the above change.

Anyone else have any concerns with this change before I push ?

Thanks Kumar

On 4/30/2014 1:47 PM, Neil Toda wrote:

...

Please review Launcher change and test.

I've added to the Launcher test : FXLauncherTest.java The test will now check that LauncherHelper$FXHelper is not loaded for non-JavaFX class and jar files.

webrev.02 contains only review suggestions from webrev.01 and the new test class.

http://cr.openjdk.java.net/~ntoda/8035782/webrev.02/

for bug:

https://bugs.openjdk.java.net/browse/JDK-8035782

Thanks

-neil

"T"hanks DrD. -neil On 5/2/2014 9:10 AM, David DeHaven wrote:

That's the first time the pentium bug ever worked in my favor... ;)

The changes look good to me. approved (with a lower case 'a', since I'm not a Reviewer with an upper case 'R').

-DrD-

...

1) Redundant return removed.

2) Kumar and I talked: Ever since the pentium bug, 1 in 80 trillion seems a big risk .. so the code was changed to check for equality with the whole class name:

private static final String JAVAFX_FXHELPER_CLASS_NAME_SUFFIX = "sun.launcher.LauncherHelper$FXHelper"; ...

if (JAVAFX_FXHELPER_CLASS_NAME_SUFFIX.equals(mainClass.getName()) || doesExtendFXApplication(mainClass)) { // Will abort() if there are problems with FX runtime FXHelper.setFXLaunchParameters(what, mode); return FXHelper.class; }

Thanks

-neil

On 5/1/2014 8:05 AM, David DeHaven wrote:

...

Do we care about the 1 in more than 80 trillion case where the third party Main-Class would be named "LauncherHelper$FXHelper"? I think the probability is extremely unlikely so I'm fine with it the way it's written.

LauncherHelper.java: 590 return;

Redundant return statement?

-DrD-

...

For completeness the bugid line needs the bugid as shown, otherwise SQE will open another bug to have you fix this.

-26 * @bug 8001533 8004547 +26 * @bug 8001533 8004547 8035782

other than that it looks good, I can push this with the above change.

Anyone else have any concerns with this change before I push ?

Thanks Kumar

On 4/30/2014 1:47 PM, Neil Toda wrote:

...

Please review Launcher change and test.

I've added to the Launcher test : FXLauncherTest.java The test will now check that LauncherHelper$FXHelper is not loaded for non-JavaFX class and jar files.

webrev.02 contains only review suggestions from webrev.01 and the new test class.

http://cr.openjdk.java.net/~ntoda/8035782/webrev.02/

for bug:

https://bugs.openjdk.java.net/browse/JDK-8035782

Thanks

-neil

Thanks Mandy. Done. On 5/1/2014 10:55 AM, Mandy Chung wrote:

Looks fine to me.

Nit: line 528: space after "(" and before "mainClass" can be removed

Mandy

On 4/30/14 3:42 PM, Kumar Srinivasan wrote:

...

For completeness the bugid line needs the bugid as shown, otherwise SQE will open another bug to have you fix this.

-26 * @bug 8001533 8004547 +26 * @bug 8001533 8004547 8035782

other than that it looks good, I can push this with the above change.

Anyone else have any concerns with this change before I push ?

Thanks Kumar

On 4/30/2014 1:47 PM, Neil Toda wrote:

...

Please review Launcher change and test.

I've added to the Launcher test : FXLauncherTest.java The test will now check that LauncherHelper$FXHelper is not loaded for non-JavaFX class and jar files.

webrev.02 contains only review suggestions from webrev.01 and the new test class.

http://cr.openjdk.java.net/~ntoda/8035782/webrev.02/

for bug:

https://bugs.openjdk.java.net/browse/JDK-8035782

Thanks

-neil