> On May 25, 2016, at 3:38 PM, Kevin Rushforth <kevin.rushfo...@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”.

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();

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.

ModuleHelper.java
  57             // ignore

This deserves to be an InternalError.  This is temporary until FX is 
transitioned to be built with JDK 9.

Otherwise, look fine to me.
Mandy

Reply via email to