Review Request: 8001533: Java launcher must launch JavaFX applications

[David DeHaven]

> java.c: L427 it would be helpful to add a comment to explain
>    the case where appClass is different than mainClass.
>    Probably the comment above L425 should be updated to
>    reflect the support for JavaFX

Done.


>    L428-430: is this fallback needed?  Would it be better
>    if LauncherHelper.getApplicationClass() always returns
>    a non-null class if the mainClass has been loaded successfully.
>    Looks like this is the case in your implementation.

Good point, the helper would have aborted by that point. How about I change to NULL_CHECK(appClass) just for safety's sake?


> LauncherHelper.java
>   L746 missing space between 'if' and '('
>   The javadoc for the checkAndLoadMain method would need to be
>   updated

Done, along with a couple more that Kumar found.


>   The change looks okay to me and I can't spot anything wrong there.
> 
>   L496-517 somewhat duplicates the logic added for FX in the
>   getMainClassFromJar method.  Have you considered some refactoring
>   work you could do to simplify the fix since I think once you get
>   the classname of the entry point (either from a JAR or command-line
>   and with and without the static void main method), the logic is
>   essentially the same.  To elaborate, I see that FXHelper.launchName
>   L707-725 is needed mainly to give a useful error message.  When
>   you find the classname of the entry point, perhaps you can load
>   the class and catch any linkage error and determine if it's caused
>   by the absence of FX runtime and output an appropriate error.
>   If the main class is successfully loaded, then proceed with
>   L496-517 (or something like that).  Just an idea you can explore.

Yes, I do feel that especially in the -jar case there is some repetition. The trouble is the ambiguity of ClassNotFoundException.

I'll poke at it and see what I can come up with.


> FXLauncherTest.java - very good test that covers many test cases.
>   Do you plan to add the classpath case (i.e. not from a
>   jar file)?

I hadn't, but if it's worthwhile then we could certainly add a test to do so. Thoughts on this Steve?

-DrD-