[9] review request: 8131888: Deliver javafx.swt as a modular jar in JDK 9

[Kevin Rushforth]

Mandy,

Thanks for your feedback. Comments inline.

Mandy Chung wrote:
>   
>> On May 25, 2016, at 3:38 PM, Kevin Rushforth <kevin.rushforth at oracle.com> wrote:
>>
>> Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8131888
>> http://cr.openjdk.java.net/~kcr/8131888/webrev.00/
>>
>> This adds support for the javafx.embed.swt package back into the JDK, which will be delivered as an automatic module in $JAVA_HOME/lib/javafx-swt.jar (final location is TBD).
>>     
>
> The approach to have javafx.swt be an automatic module that can access org.eclipse.swt.* (that may be from an unnamed module) sounds reasonable.  I wonder what the JAR file should be named -  javafx.swt.jar or javafx-swt.jar?  They both have the same module name “javafx.swt”.
>   

Filenames with two dots are a little odd, so I went with the dash. It 
doesn't much matter, though.

> I skimmed through the change.  There are several System.err.println calls that I assume are debugging code to be removed. e.g.
>
> FXCanvas.java
>  247         System.err.println("FXCanvas class successfully initialized”);
>  294                 System.err.println("FXCanvas: FX platform is initlialized”);
>
> PlatformImpl.java
>  308                 System.err.println("FXCanvas: no permission to access JavaFX internals");
>  309                 ex.printStackTrace();
>   

Right. I noted this in the JBS evaluation and will remove all when I 
post the final webrev.

> I reviewed mainly addExportsToFXCanvas and addExportsToFXCanvas methods.  Happy to see StackWalker be useful in this case.  The check to compare the class name with “javafx.embed.swt.FXCanvas” to derermine whether qualified exports should be added.  You can consider checking the caller's module name as a starter.  I know you are planning to look into the integrity check as a follows up.
>   

I had considered adding the check for the module name, but was concerned 
about the OSGi case, as mentioned by Tom Schindl in the bug report, in 
which they add the jar file via a custom classloader; the concern is 
that there is no support in the JDK to make it an automatic module in 
this case. I will file a follow-up issue to improve the integrity checking.


> ModuleHelper.java
>   57             // ignore
>
> This deserves to be an InternalError.  This is temporary until FX is transitioned to be built with JDK 9.
>   

Actually, we can't do this yet, because we still build and test with JDK 
9 build 109 which doesn't have support for the module system. Until we 
upgrade to a JDK with the module system enabled we need this to be a 
silent no-op or all of our tests will fail.

-- Kevin


> Otherwise, look fine to me.
> Mandy