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

[Kevin Rushforth]

My quick comments:

1. The changes to java.desktop module-info.java won't compile when 
applied to jdk-client, since there is already a qualified export of the 
sun.swing package to another internal class. Can you update your patch 
to be based off of jdk-client? I note that it requires a small patch to 
be able to build FX with JDK 11 (it's a known gradle issue), but it's 
not hard to do.


2. As I mentioned in an off-line email, the final version of the 
javafx.swing patch will need to use 'requires static 
jdk.unsupported.desktop' in its module-info.java, so that it will still 
run on JDK 10 EA. This means that you will need to ensure that 
jdk.unsupported.desktop is loaded via a service loader as discussed. 
This will not affect the public API at all, so that can proceed in 
parallel, but this needs to be fixed.


3. As I mentioned in my earlier reply to Mandy's comment, Swing interop 
apps will fail if a security manager is installed, because the 
jdk.unsupported.desktop classes are loaded by the app class loader. This 
can be sorted out later as a follow-on bug, with permissions added to 
the default policy file, etc.


4. As Phil mentioned, some docs would be good. They don't need to be all 
that detailed, since it isn't intended for use by applications, and will 
not be included in the published API docs bundle (i.e., it should be 
excluded from "make docs").


I will do a more detailed review early next week.

-- Kevin

On 5/4/2018 1:53 PM, Phil Race wrote:

Two quick comments.

1) I'd like "Peer" removed from all the exported API.
2) It is probably stable enough now that you can start to add some 
javadoc.


-phil.

On 05/04/2018 05: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] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Kevin Rushforth]

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 noticed this, too, during my testing this morning. It still fails when 
the security manager is enabled. We already have a bug filed for this:


https://bugs.openjdk.java.net/browse/JDK-8202451

I had hoped would be fixed along with the new unsupported API, but I 
agree that this could be fixed later.


-- Kevin


On 5/4/2018 2:08 PM, mandy chung wrote:

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] 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] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Phil Race]

Two quick comments.

1) I'd like "Peer" removed from all the exported API.
2) It is probably stable enough now that you can start to add some javadoc.

-phil.

On 05/04/2018 05: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] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Nir Lisker]

Hi Prasanta,

In SwingNode, lines 870 -883, is there a reason that only the first method
uncommented @Override? I don't understand the comment that wants to skip
@Override.

Some minor code comments:

* LightweightFrameWrapper#isCompEqual: you can skip the 'if' and just
'return c != lwFrame.getLightweightFrame();'.
* Same with SwingInterOpUtils#isUngrabEvent.

* LightweightFrameWrapper#createUngrabEvent: you can directly 'return new
...' (I wouldn't comment about this had you not done this to the other
methods).

* LightweightContentWrapper: there're no new lines between non-empty
methods, especially in LightweightContentProxy. Is this an acceptable code
style?
* Same for DragSourceContextPeerWrapper and the proxy,
DragTargetContextPeerWrapper.
and DispatcherWrapper.

* Extra empty line(s) at the end of DropTargetContextPeerWrapper.

- Nir

On Fri, May 4, 2018 at 3:10 PM, Kevin Rushforth 
wrote:

> Thanks, Prasanta.
>
> As an additional note to reviewers:
>
> The JDK portion of this change (JDK-8202199) is the subject for this
> review.
>
> The FX webrev is enough to be able to test the JDK side, but will need
> additional refactoring (to allow it to continue to build / run with JDK 10)
> before being ready for review. If you spot something of concern in the FX
> webrev, please feel free to bring it up, but don't spend time doing a
> thorough review just yet.
>
> -- Kevin
>
>
>
> On 5/4/2018 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] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Kevin Rushforth]

Thanks, Prasanta.

As an additional note to reviewers:

The JDK portion of this change (JDK-8202199) is the subject for this review.

The FX webrev is enough to be able to test the JDK side, but will need 
additional refactoring (to allow it to continue to build / run with JDK 
10) before being ready for review. If you spot something of concern in 
the FX webrev, please feel free to bring it up, but don't spend time 
doing a thorough review just yet.


-- Kevin


On 5/4/2018 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
||

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

[Prasanta Sadhukhan]

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: Paint Phase Debugging / Performance

[Matthew Elliot]

Hi Pedro,

The first link I have read through many times, it is very useful for ideas
but doesn't really flesh out or go into much detail on each topic. It also
comments a few times on the problems we've encountered, 'what costs what'
is difficult to understand / measure.

The second link I hadn't found my way to and is definitely interesting to
understand more about how things are working internally - thanks.

Regards,
Matt


On 3 May 2018 at 19:17, Pedro Duque Vieira 
wrote:

> Hi Matthew,
>
> On the topic of documents with indications for improving performance.
>
> Don't know if you already found it, but there is a draft here (to which
> I've made a small contribution):
> https://wiki.openjdk.java.net/display/OpenJFX/Performance+Tips+and+Tricks
>
> A bit old though...
>
> And here:
>
> https://wiki.openjdk.java.net/display/OpenJFX/Performance+Ideas
>
>
> --
> Pedro Duque Vieira
>