Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

Great!  Thanks Prasanta.

Mandy

On 6/15/18 5:42 AM, Prasanta Sadhukhan wrote:

I confirm jdk.unsupported.desktop is not present in doc build.

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

Thanks for confirming that.

Mandy

On 6/14/18 3:53 PM, Kevin Rushforth wrote:
I verified on an earlier version of the patch that it wasn't in the 
docs, but it would be good for Prasanta to double-check.


-- Kevin


On 6/14/2018 1:29 PM, Phil Race wrote:

you surely mean
@since 11

I believe it has been verified that it is excluded from the docs build 
.. right Prasanta ?


-phil

On 06/14/2018 01:26 PM, mandy chung wrote:
I reviewed the module-info.java change. @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

On 6/14/18 1:29 PM, Phil Race wrote:

you surely mean
@since 11


Oops typo. Yes @since 11

Mandy

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

I reviewed the module-info.java change.  @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

On 5/9/18 7:48 AM, Alan Bateman wrote:

On 09/05/2018 15:42, Philip Race wrote:

:

Qn. to Mandy & Alan : it seems there is no need to mention this module
in make/common/Modules.gmk in order to get it built, but is there any
advantage in doing so ? I mean without it, there is no conscious 
listing of

it as a module nor classification of it.
Right, it's not necessary to list modules that are defined to the 
application class loader and are only in the JDK image. There's a 
comment at the top of the make file on this but it probably could be a 
bit clearer.


The build determines the modules to be compiled from the source 
directory per the modular source layout.  make/common/Modules.gmk serves 
a few purpose and the relevant one here is the module to class loader 
mapping.  It lists the modules defined to the boot loader and platform 
boot loader.


Mandy

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

On 5/7/18 2:26 AM, Prasanta Sadhukhan wrote:

Modified webrev to use InteropProvider
http://cr.openjdk.java.net/~psadhukhan/fxswing.9/


This version looks good.   InteropProviderImpl name is okay with me.

Mandy

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

I skimmed through the sources.  It's good to see that this patch
is straight forward.  A couple of comments:

jdk.unsupported.desktop is defined to the application class loader
which I think it's fine as FX modules are defined to the same class
loader.

I expect src/java.base/share/lib/security/default.security will
need to be modified and grant permissions to jdk.unsupported.desktop.
Kevin and I talked about potential issues with running with security
manager enabled.  Maybe it is a separate task to follow up and it
may reveal if any operation needs doPrivileged - that's fine with me.

The docs build starts with a known set of root modules for javadoc
generation.  I believe jdk.unsupported.desktop won't be included
in the docs build which is intended (as it's unsupported).  Can
you verify it?

Nit: jdk.swing.interop.* source files
it'd be good to keep import statements sorted by the names.

Mandy

On 5/4/18 5:00 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review an enhancement to remove the tight coupling of JDK 
internal class from FX so that
when javafx.* modules are removed from Openjdk build in jdk11, FX in 
general, and fx swing interop, in particular, can still build and 
function.


Right now, FX uses 6 jdk internal packages
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does so 
with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it need 
not be in the default module graph.


The module-info.java will look like this:
|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||
Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
||

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[mandy chung]

It can be addressed later while I think this is a simple change to the 
getInstance method.  I guess you prefer to keep PlatformLogger as close 
to the original version and that's fine.


Mandy

On 3/27/18 4:46 AM, Kevin Rushforth wrote:
It doesn't seem harmful to keep the current implementation. This seems 
better to leave for the follow-on JBS issue (JDK-8200236) unless there 
something I am missing.


-- Kevin


mandy chung wrote:

You don't need the loggers map and getLogger method can simply return
 return new PlatformLogger(System.getLogger(name));

Other than this, looks fine.

Mandy


On 3/26/18 4:36 AM, Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in 
-http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth
Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs;openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
  public static void setUpClass() {
  >>>System.err.println("SelectBindingTest : log messages are
expected from these tests.");


All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


Ajit Ghaisas wrote:
 

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

 Bug :https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

 Request you to review.

Regards,
Ajit
   
   

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[mandy chung]

You don't need the loggers map and getLogger method can simply return
 return new PlatformLogger(System.getLogger(name));

Other than this, looks fine.

Mandy


On 3/26/18 4:36 AM, Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in - 
http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth
Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
  public static void setUpClass() {
  >>>System.err.println("SelectBindingTest : log messages are
expected from these tests.");


All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

 Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

 Request you to review.

Regards,
Ajit
   

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[mandy chung]

On 3/23/18 9:34 AM, Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

 Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/



buildSrc/addExports
   FX modules are compiled together and I don't expect these 
--add-exports are needed.  I suspect it's because of the boot JDK and 
this is a temporary dance?



PlatformLogger.java
 150 public static synchronized PlatformLogger getLogger(String name) {

This keeps the weak reference to all PlatformLogger created.  A 
simplification is to return


   new PlatformLogger(System.getLogger(name));

System::getLogger should return the same instance if it has been 
created.  I also suspect the caller of PlatformLogger::getLogger keeps a 
strong reference and calls it once.


I recalled there were some native methods calling the Java API to set 
level.  It has been a while back since I looked at it and things miight 
have changed.  Is there such reference any more?


Other than the above comments, this change looks good.

Mandy

Re: [11] RFR : JDK-8195800 : Eliminate dependency on sun.reflect.misc in javafx modules

[mandy chung]

On 3/19/18 7:10 AM, Ambarish Rapte wrote:


Hi Kevin, Alan & Mandy,

    Request you to review this fix:

    Webrev: 
http://cr.openjdk.java.net/~arapte/fx/8195800/webrev.00/ 



    JBS: https://bugs.openjdk.java.net/browse/JDK-8195800



This looks okay.  Nit FieldUtil isn't a trampoline class and you may 
just drop this comment:


  30 /*
  31  * Create a trampoline class.
  32  */

Mandy

Re: [9] Review request: 8180040: Exclude jdk.packager module from unified JDK 9 docs

[Mandy Chung]

> On May 9, 2017, at 6:52 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
> wrote:
> 
> Thanks for the review. We currently don't use package-info.java anywhere, but 
> I can file a separate bug for converting all of our package.html to 
> package-info.java.

That’d be good.

> I can add the the missing copyright headers at the same time.
> 

OK.

No need for an updated webrev.

Mandy

> -- Kevin
> 
> 
> Mandy Chung wrote:
>> 
>>> On May 9, 2017, at 6:08 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
>>> <mailto:kevin.rushfo...@oracle.com> wrote:
>>> 
>>> Please review the following to exclude jdk.packager module from the JDK 
>>> docs bundle:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8180040 
>>> <https://bugs.openjdk.java.net/browse/JDK-8180040>
>>> http://cr.openjdk.java.net/~kcr/8180040/webrev.00/ 
>>> <http://cr.openjdk.java.net/~kcr/8180040/webrev.00/>
>>> 
>>> I also added a missing package description for the jdk.packager.services 
>>> package (in the jdk.packager.services module), since the 
>>> jdk.packager.services module will remain in the docs.
>>> 
>> 
>> I suggest to convert package.html to package-info.java.  Also need copyright 
>> header.
>> 
>> build.properties change looks fine.
>> 
>> Mandy

Re: [9] Review request: 8180040: Exclude jdk.packager module from unified JDK 9 docs

[Mandy Chung]

> On May 9, 2017, at 6:08 PM, Kevin Rushforth  
> wrote:
> 
> Please review the following to exclude jdk.packager module from the JDK docs 
> bundle:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180040
> http://cr.openjdk.java.net/~kcr/8180040/webrev.00/
> 
> I also added a missing package description for the jdk.packager.services 
> package (in the jdk.packager.services module), since the 
> jdk.packager.services module will remain in the docs.

I suggest to convert package.html to package-info.java.  Also need copyright 
header.

build.properties change looks fine.

Mandy

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Mandy Chung]

Looks good.

"Deploying an Application as a Module” section is duplicated in several
JavaBean*Property classes.  One alternative is to move it to the package
summary.  I have no objection to leave it as is. 

Mandy


> On May 3, 2017, at 4:30 PM, Kevin Rushforth  
> wrote:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8177566
> 
> Here is the updated webrev with (I hope) all comments addressed:
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/complete-webrev/
> 
> For those who reviewed the earlier webrev, I have prepared delta webrevs.
> 
> * Delta webrev for the fix itself (just a slight change to the error message, 
> plus I hid the unused public methods in MethodUtil) :
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-fix-only.00/
> 
> * No changes in the tests
> 
> * Delta webrev for the doc changes:
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/delta-doc-only.00/
> 
> * The sparse javadocs are also updated here:
> 
> http://cr.openjdk.java.net/~kcr/8177566/webrev.01/javadoc/
> 
> -- Kevin
> 
> 
> Kevin Rushforth wrote:
>> This review is being cross-posted to both openjfx-dev and jigsaw-dev.
>> 
>> Please review the proposed fix for:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8177566
>> http://cr.openjdk.java.net/~kcr/8177566/webrev.00/complete-webrev/
>> 
>> Details of the fix as well as notes to reviewers are in the bug report [1] 
>> (e.g., I've also generated separate webrevs for the fix itself, the doc 
>> changes, and the test changes).
>> 
>> -- Kevin
>> 
>> [1] 
>> https://bugs.openjdk.java.net/browse/JDK-8177566?focusedCommentId=14074243=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14074243
>>  
>> 

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Mandy Chung]

> On May 2, 2017, at 2:22 PM, Kevin Rushforth  
> wrote:
> 
> Here is the message:
> 
> IllegalAccessException: class com.sun.javafx.property.MethodHelper cannot 
> access class com.foo (in module foo.app) because module foo.app does not open 
> com.foo to javafx.base 
> 

It would be better to emit a more informative message e.g. “javafx.base cannot 
access class com.foo (in module foo.app).  Either exports com.foo unqualifiedly 
or open com.foo to javafx.base”.  Also in MethodUtil::invoke
  61if (!module.isExported(packageName)) {
You can do this check only if module.isNamed.

> It is roughly the same message that any similar illegal access would generate 
> (e.g., we get similar error messages when FXML tries to call setAccessible 
> for a field annotated with @FXML if the module is not "open" to javafx.fxml).
> 
>> javafx.base/src/main/java/com/sun/javafx/property/MethodHelper.java
>> javafx.fxml/src/main/java/com/sun/javafx/fxml/MethodHelper.java
>> javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
>>   45 public static Object invoke(Method m, Object obj, Object[] params)
>> 
>> To avoid 3 ModuleHelper classes, the invoke method can take
>> the callerModule argument to replace this line: 
>>   56 final Module thisModule = MethodHelper.class.getModule();
>>   
> 
> I'm fairly certain that won't work. Module::addOpens is caller sensitive and 
> will only work when called from the module in question.
> 

You are right.  Wont’t work.
>> javafx.base/src/main/java/com/sun/javafx/reflect/MethodUtil.java
>>   There are a few other public methods which I think JavaFX doesn’t
>>   need and can be removed.
>>   
> 
> Yes, I could do this to reduce the public footprint of the class. To minimize 
> the diffs between the original and our copy, I might just comment out the 
> "public". That would also make it easier to add them back in a future version 
> (e.g., to eventually get rid of all dependency on sun.reflect.misc). Thoughts?

I will leave it up to you.
Mandy

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Mandy Chung]

Hi Kevin,

> On May 1, 2017, at 5:21 PM, Kevin Rushforth  
> wrote:
> 
> This review is being cross-posted to both openjfx-dev and jigsaw-dev.
> 
> Please review the proposed fix for:
> 
> https://bugs.openjdk.java.net/browse/JDK-8177566
> http://cr.openjdk.java.net/~kcr/8177566/webrev.00/complete-webrev/

First pass of comment:

javafx.base/src/main/java/com/sun/javafx/property/PropertyReference.java
 196 try {
 197 return 
(ReadOnlyProperty)MethodHelper.invoke(propertyGetter, bean, (Object[])null);

 198 } catch (Exception ex) {
 199 throw new RuntimeException(ex);
 200 }

Do you have an example exception thrown if the package is not
open to javafx.base?  IAE is thrown by MethodHelper.invoke.
Are you detecting this and throw an exception with friendlier
message?

javafx.base/src/main/java/com/sun/javafx/property/MethodHelper.java
javafx.fxml/src/main/java/com/sun/javafx/fxml/MethodHelper.java
javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
  45 public static Object invoke(Method m, Object obj, Object[] params)

To avoid 3 ModuleHelper classes, the invoke method can take
the callerModule argument to replace this line: 
  56 final Module thisModule = MethodHelper.class.getModule();

javafx.base/src/main/java/com/sun/javafx/reflect/MethodUtil.java
  There are a few other public methods which I think JavaFX doesn’t
  need and can be removed.

Mandy

Re: [9] Review request: 8178015: Clarify requirement for app modules to export/open packages to javafx modules

[Mandy Chung]

+1
Mandy

> On Apr 20, 2017, at 11:06 AM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
> wrote:
> 
> Here is an updated webrev with a few suggested wording changes (e.g., removed 
> the reference to ModuleDescriptor, changed "accessible by" back to 
> "accessible to").
> 
> http://cr.openjdk.java.net/~kcr/8178015/webrev.02/
> 
> Additionally, I removed the example in the FXML annotation showing the use of 
> "opens to" in module-info.java (but left the example in Application).
> 
> -- Kevin
> 
> 
> Kevin Rushforth wrote:
>> 
>> 
>> Mandy Chung wrote:
>>>> On Apr 18, 2017, at 12:48 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
>>>> wrote:
>>>> 
>>>> 
>>>> 
>>>> Alan Bateman wrote:
>>>>   
>>>>> On 18/04/2017 19:19, Kevin Rushforth wrote:
>>>>> 
>>>>>> Good suggestion. Here is an updated webrev with Mandy's suggestion and 
>>>>>> yours:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~kcr/8178015/webrev.01/
>>>>>> 
>>>>>> -- Kevin
>>>>>>
>>>>> This looks mostly okay.
>>>>> 
>>>>> I guess for FXML then I assume that the annotated member could be public, 
>>>>> in which case the package just needs to be exported (no need to open).
>>>>>  
>>>> Yes, it could be, but the more typical use of the annotation is on 
>>>> non-public members, so that was why I chose that as the example.
>>>>
>>> 
>>> I have the same comment as Alan.  The example in the javadoc may be 
>>> perceived as a recommendation.  We should probably recommend the annotated 
>>> member be moved to public and encapsulated in its module, just exports it 
>>> to javafx.fxml to use.
>> 
>> For the use of the FXML annotation, that is exactly the recommendation. If 
>> you want the FXMLLoader to modify a public member in a public class, you 
>> don't need the annotation at all (although I guess it can still be used as 
>> useful documentation).
>> 
>> In the longer "Introduction to FXML" doc, which is included with the API 
>> docs and linked from the javafx.fxml package description, the only examples 
>> we give of using the @FXML annotation are for non-public members. See:
>> 
>> http://download.java.net/java/jdk9/jfxdocs/javafx/fxml/doc-files/introduction_to_fxml.html#fxml_annotation
>>  
>> 
>> Specifically, the description of the FXML annotation says:
>> 
>>   However, for developers who prefer more restricted visibility for
>>   controller fields or handler methods, the javafx.fxml.FXML
>>   annotation can be used. This annotation marks a protected or private
>>   class member as accessible to FXML.
>> 
>> 
>> For consistency, it seems best to show the example in the FXML javadocs as 
>> an example of using "opens" since that is what would be needed for the 
>> typical use case, such as those we document elsewhere.
>> 
>> -- Kevin
>> 
>> 
>>> Mandy

Re: [9] Review request: 8178015: Clarify requirement for app modules to export/open packages to javafx modules

[Mandy Chung]

> On Apr 18, 2017, at 12:48 PM, Kevin Rushforth  
> wrote:
> 
> 
> 
> Alan Bateman wrote:
>> 
>> 
>> On 18/04/2017 19:19, Kevin Rushforth wrote:
>>> Good suggestion. Here is an updated webrev with Mandy's suggestion and 
>>> yours:
>>> 
>>> http://cr.openjdk.java.net/~kcr/8178015/webrev.01/
>>> 
>>> -- Kevin
>> This looks mostly okay.
>> 
>> I guess for FXML then I assume that the annotated member could be public, in 
>> which case the package just needs to be exported (no need to open).
> 
> Yes, it could be, but the more typical use of the annotation is on non-public 
> members, so that was why I chose that as the example.

I have the same comment as Alan.  The example in the javadoc may be perceived 
as a recommendation.  We should probably recommend the annotated member be 
moved to public and encapsulated in its module, just exports it to javafx.fxml 
to use.  

Mandy

Re: [9] Review request: 8178015: Clarify requirement for app modules to export/open packages to javafx modules

[Mandy Chung]

> On Apr 17, 2017, at 5:00 PM, Kevin Rushforth  
> wrote:
> 
> Please review the following javadoc change:
> 
> https://bugs.openjdk.java.net/browse/JDK-8178015
> http://cr.openjdk.java.net/~kcr/8178015/webrev.00/
> 

+ * Applications in a Module
:
+ * {@link Module#isOpen(String,Module) open} the containing package to the
+ * {@code javafx.graphics} module.

to “at least” javafx.graphics module.

Otherwise looks good.
Mandy

Re: [9] Review request: 8173080: Add licenses for non-distributed third-party source code in repo

[Mandy Chung]

> On Apr 5, 2017, at 3:51 PM, Kevin Rushforth  
> wrote:
> 
> Dave,
> 
> Please review the following fix to add missing .md license files for 
> non-distributed third-party source code in our repo:
> 
> https://bugs.openjdk.java.net/browse/JDK-8173080
> http://cr.openjdk.java.net/~kcr/8173080/webrev.00/
> 

The license files look good to me.

Mandy

Re: [9] Review request: 8170701: Update FXML documentation for setAccessible

[Mandy Chung]

> On Mar 6, 2017, at 7:11 AM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
> wrote:
> 
> 
> 
> Mandy Chung wrote:
>> 
>>> On Mar 4, 2017, at 5:14 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
>>> <mailto:kevin.rushfo...@oracle.com> wrote:
>>> 
>>> http://cr.openjdk.java.net/~kcr/8170701/webrev.01/ 
>>> <http://cr.openjdk.java.net/~kcr/8170701/webrev.01/>
>>> 
>>> 
>> 
>>   40  * object {@link Module#isOpen opens} the containing package to the
>> 
>> Nit: s/@link/@linkplain
>> 
>>   41  * {@code javafx.fxml} module, either in its {@link ModuleDescriptor}
>>   42  * (module-info.class) or by calling {@link Module#addOpens}.
>> 
>> Do you intend to take out “(module-info.class)”?  
>>   
> 
> I was thinking to leave it in, since module-info.class is the most common way 
> to specify a ModuleDescription. Maybe better would be:
> 
> @{link ModuleDescriptor} (e.g., in its module-info.class)
> 

That’s okay.
>>   43  * An object is also reflectively accessible if it is declared as 
>> public,
>> 
>> Does “it” mean its constructor?
>>   
> 
> No. It means the declaration itself, for example:
> 
> @FXML
> public String myString;
> 
> 
> as opposed to:
> 
> @FXML
> private String myString;

I see.   I’m not close to this spec.  I am not sure if it worths further 
clarification such as “it is a public member”.  I’ll leave it up for you to 
decide.

Mandy

Re: [9] Review request: 8170701: Update FXML documentation for setAccessible

[Mandy Chung]

> On Mar 4, 2017, at 5:14 PM, Kevin Rushforth  
> wrote:
> 
> http://cr.openjdk.java.net/~kcr/8170701/webrev.01/
> 

  40  * object {@link Module#isOpen opens} the containing package to the

Nit: s/@link/@linkplain

  41  * {@code javafx.fxml} module, either in its {@link ModuleDescriptor}
  42  * (module-info.class) or by calling {@link Module#addOpens}.

Do you intend to take out “(module-info.class)”?  

  43  * An object is also reflectively accessible if it is declared as public,

Does “it” mean its constructor?

  44  * is in a public class, and the module containing that class
  45  * {@link Module#isExported(String,Module) exports}

Nit: s/@link/@linkplain

  46  * the containing package to the {@code javafx.fxml} module.

This is word-smithing and formatting nit.  No need to send a new webrev.

Mandy

Re: [9] Review request: 8170702: Document that javafx.graphics needs explicit access to application main class

[Mandy Chung]

> On Mar 4, 2017, at 1:04 AM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
> wrote:
> 
> 
> 
> Mandy Chung wrote:
>> 
>>> On Mar 3, 2017, at 10:36 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
>>> <mailto:kevin.rushfo...@oracle.com> wrote:
>>> 
>>> [fixed subject line]
>>> 
>>> Please review the following to document that javafx.graphics needs explicit 
>>> access to the Application class.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8170702 
>>> <https://bugs.openjdk.java.net/browse/JDK-8170702>
>>> http://cr.openjdk.java.net/~kcr/8170702/webrev.00/ 
>>> <http://cr.openjdk.java.net/~kcr/8170702/webrev.00/>
>>> 
>> 
>>   69  * containing package must be {@link Module#isExported(String,Module) 
>> exported}
>> 
>> @linkplain instead?
>>   
> 
> I was following the pattern in Module.java, etc., which uses a regular @link 
> in similar cases.
> 

Module.java should probably use @linkplain if that’s the case.  
>>  239 StackTraceElement[] cause = 
>> Thread.currentThread().getStackTrace();
>> 
>> Good candidate to use StackWalker API.
>>   
> 
> This is pre-existing code (since JDK 7), and I don't want to change the 
> implementation this late while fixing a doc bug. I will file a follow-on bug 
> to consider improving this for JDK 10.
> 

It's fine for JDK 10.
>> Is @throws RuntimeException an existing behavior?  I’d think CNFE and 
> 
> Yes, this seems like another good place to document the restriction. I'll 
> post a .01 version of the webrev with this update.

Sounds good.

Mandy

Re: [9] Review request: 8170702: Document that javafx.graphics needs explicit access to application main class

[Mandy Chung]

> On Mar 3, 2017, at 10:36 PM, Kevin Rushforth  
> wrote:
> 
> [fixed subject line]
> 
> Please review the following to document that javafx.graphics needs explicit 
> access to the Application class.
> 
> https://bugs.openjdk.java.net/browse/JDK-8170702
> http://cr.openjdk.java.net/~kcr/8170702/webrev.00/

  69  * containing package must be {@link Module#isExported(String,Module) 
exported}

@linkplain instead?

 239 StackTraceElement[] cause = Thread.currentThread().getStackTrace();

Good candidate to use StackWalker API.

Is @throws RuntimeException an existing behavior?  I’d think CNFE and 
InaccessibleAE might be more appropriate.

line 209 “It must be a subclass of Application or a RuntimeException will be 
thrown.”

I think this statement should be extended to cover if the class and its 
constructor are public and exported.

Mandy

Re: [9] Review request: 8170701: Update FXML documentation for setAccessible

[Mandy Chung]

> On Mar 3, 2017, at 10:29 PM, Kevin Rushforth  
> wrote:
> 
> Please review the following to update the FXML docs to document the 
> requirement for a module that annotates non-public members with @FXML to 
> "open" the containing package to the javafx.fxml module.
> 
> https://bugs.openjdk.java.net/browse/JDK-8170701
> http://cr.openjdk.java.net/~kcr/8170701/webrev.00/

The doc change looks fine to me.

Minor suggestion.  You may refer "module-info class” to {@link 
ModuleDescriptor} which can be synthesized (although it may not be common).

You may consider referencing {@link Module#isOpened} where you may say “the 
containing package of the object is {@link Module#isOpened opened} to {@code 
javafx.fxml} module.

Mandy

Re: [9] Review request: 8172526: Third-party licenses for javafx.* modules are missing

[Mandy Chung]

+1

Mandy

> On Feb 13, 2017, at 5:30 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
> wrote:
> 
> Fixed. Thanks.
> 
> http://cr.openjdk.java.net/~kcr/8172526/webrev.01/ 
> <http://cr.openjdk.java.net/~kcr/8172526/webrev.01/>
> 
> -- Kevin
> 
> 
> Mandy Chung wrote:
>> 
>>> On Feb 13, 2017, at 1:50 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
>>> <mailto:kevin.rushfo...@oracle.com> wrote:
>>> 
>>> Dave & Mandy,
>>> 
>>> Please review the following fix to deliver the JavaFX third-party license 
>>> content into the JDK build.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8172526 
>>> <https://bugs.openjdk.java.net/browse/JDK-8172526>
>>> http://cr.openjdk.java.net/~kcr/8172526/webrev.00/ 
>>> <http://cr.openjdk.java.net/~kcr/8172526/webrev.00/>
>>> 
>> 
>> 
>> javafx.graphics/src/main/legal/jpeg_v7.md
>> 
>>5 jcapimin.c
>>6  *
>> 
>> /* is missing.
>> 
>> Otherwise looks okay.
>> 
>> Mandy

Re: [9] Review request: 8172526: Third-party licenses for javafx.* modules are missing

[Mandy Chung]

> On Feb 13, 2017, at 1:50 PM, Kevin Rushforth  
> wrote:
> 
> Dave & Mandy,
> 
> Please review the following fix to deliver the JavaFX third-party license 
> content into the JDK build.
> 
> https://bugs.openjdk.java.net/browse/JDK-8172526
> http://cr.openjdk.java.net/~kcr/8172526/webrev.00/


javafx.graphics/src/main/legal/jpeg_v7.md

   5 jcapimin.c
   6  *

/* is missing.

Otherwise looks okay.

Mandy

Re: [9] Review request: JDK-8170485: Switch to building JavaFX with new module-info syntax

[Mandy Chung]

> On Dec 6, 2016, at 8:10 AM, Kevin Rushforth  
> wrote:
> 
> Chien & Dave,
> 
> Please review the preliminary webrev to allow building JavaFX with jdk-9+148 
> and later:
> 
> https://bugs.openjdk.java.net/browse/JDK-8170485
> http://cr.openjdk.java.net/~kcr/8170485/webrev.00/
> 
> The details are in the JBS issue and also in the "HEADS-UP" message [1] I 
> sent yesterday.
> 
> As indicated in JBS, I will update the webrev later this week to bump the 
> minimum build to 148 once 148 is promoted and available on java.net. No other 
> changes are anticipated (unless you find anything while reviewing it).

Looks good. 

Mandy

Re: [9] Review request: DK-8169417: JavaFX to include jake-compatible versions of module-info.java with import bundles

[Mandy Chung]

Looks good to me.

Mandy

> On Nov 8, 2016, at 4:11 PM, Kevin Rushforth  
> wrote:
> 
> Hi,
> 
> Please review the following fix to include jake-compatible versions of 
> module-info.java with our input bundles:
> 
> https://bugs.openjdk.java.net/browse/JDK-8169417
> http://cr.openjdk.java.net/~kcr/8169417/webrev.00/
> 
> These new files end up in a new "modules_src_jake" subdirectory of 
> modular-sdk, and are zipped up delivered to the JDK build as 
> javafx-exports.zip along with the existing directories. The existing jdk9 
> build will not be affected. The jisgaw/jake build will take the 
> modules_src_jake files in preference to modules_src.
> 
> NOTE: for the files that have a jake-equivalent, we will need to keep them in 
> sync going forward for as long as we need both.
> 
> -- Kevin
> 

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

[Mandy Chung]

> On May 27, 2016, at 1:30 AM, Tom Schindl  wrote:
> 
> Do you have an example how to construct such a Layer?


// path is the path to javafx-swt.jar
ModuleFinder finder = ModuleFinder.of(path);
Configuration cf = Layer.boot()
.configuration()
.resolveRequires(finder, ModuleFinder.of(), Set.of("javafx.swt"));

// “parent” is the class loader to which SWT types are visible
Layer layer = Layer.boot().defineModulesWithOneLoader(cf, parent);

// the class loader defining javafx.swt module
ClassLoader loader = layer.findLoader("javafx.swt”);

[1] will show more examples of Layer.
Mandy
[1] 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/df35a805b405/test/java/lang/reflect/Layer/LayerAndLoadersTest.java

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

[Mandy Chung]

> On May 26, 2016, at 8:38 AM, Kevin Rushforth <kevin.rushfo...@oracle.com> 
> wrote:
> 
> Yes, I've tested it in both modes (with a simple HelloFXCanvas program) -- as 
> an automatic jar file and as just an ordinary jar on the classpath.

an automatic module needs to be on modulepath.

For container-like environment, it can create a Layer to load javafx.swt with a 
parent class loader that loads SWT classes - would that be feasible?

Mandy

> 
> -- Kevin
> 
> 
> Tom Schindl wrote:
>> Rereading the jira it take that back if javafx.swt can still be loaded as a 
>> simple jar things will work
>> 
>> Tom
>> 
>> Von meinem iPhone gesendet
>> 
>>   
>> 
>>> Am 26.05.2016 um 16:51 schrieb Tom Schindl <tom.schi...@bestsolution.at>
>>> :
>>> 
>>> Hi,
>>> 
>>> I highly doubt this will work in an OSGi-Env like Eclipse (which the 99%) 
>>> use case for SWT useage.
>>> 
>>> The SWT jar is not on the application classpath so how should a module 
>>> (named or unnamed) find the SWT classes?
>>> 
>>> Tom
>>> 
>>> Von meinem iPhone gesendet
>>> 
>>> 
>>> 
>>>> Am 26.05.2016 um 02:43 schrieb Mandy Chung <mandy.ch...@oracle.com>
>>>> :
>>>> 
>>>> 
>>>> 
>>>>   
>>>> 
>>>>> 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
>>>>   
>>>> 
>> 
>>   
>> 

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

[Mandy Chung]

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

Re: [9] Review request: 8153872: Nashorn no longer needs access to com.sun.javafx.application

[Mandy Chung]

> On Apr 19, 2016, at 8:25 AM, Kevin Rushforth  
> wrote:
> 
> Jim,
> 
> Please review the following fix:
> 
> https://bugs.openjdk.java.net/browse/JDK-8153872
> http://cr.openjdk.java.net/~kcr/8153872/webrev.00/
> 
> This is a simple backout of the earlier fix for JDK-8153754.

+1

Glad that this dependency is eliminated.

Mandy

Re: [9] Review request: 8154203: Use StackWalker instead of the now-deprecated sun.reflect.Reflection class

[Mandy Chung]

> On Apr 14, 2016, at 3:27 PM, Kevin Rushforth  
> wrote:
> 
> Hi Vadim,
> 
> Please review:
> 
> https://bugs.openjdk.java.net/browse/JDK-8154203
> http://cr.openjdk.java.net/~kcr/8154203/webrev.00/
> 
> It's a straight-forward fix to replace Reflection::getCallerClass with 
> StackWalker::getCallerClass and remove the (no longer needed) CallerSensitive 
> annotation.

+1

It’s good to see this migrate to use StackWalker.

Mandy