Mandy,
Thanks for your feedback. Comments inline.
Mandy Chung wrote:
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”.
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
