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