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

[Nir Lisker]

Hi Prasanta,

If @override is not added, then it will cause recursion between
> LightweightContent.java:130 and LightweightContent.java:187


I don't understand how the existence or lack thereof of an @Override can
cause or prevent recursion (bar reflection). Since all the methods of the
interface are overriden in SwingNode (regardless of the annotation) their
default implementation is not used, and SwingNode is recursion-free.
What is more worrisome is that if the interface methods are not overriden
in some other implementing class, there will be infinite recursion, but
this a fault in the design of the interface (I'd say), and out of scope for
this issue. On the up side, one of the methods in the interface is
deprecated, so it didn't go completely under the radar.

Personally, I force the use of @Override where applicable since it can only
help and I only brought this up since you uncommented 1 of the annotations
and left 2 others.

Thanks,
Nir

On Sat, May 5, 2018 at 6:20 PM, Prasanta Sadhukhan <
prasanta.sadhukhan at oracle.com> wrote:

> Updated webrev to modify java.desktop module-info.java (only difference
> between webrev7 and this) to remove the duplicate exports of sun.awt so we
> will have now
>
> exports sun.awt to
>         jdk.accessibility,
>         jdk.unsupported.desktop;
>
> http://cr.openjdk.java.net/~psadhukhan/fxswing.8/
>
> Regards
> Prasanta
>
> On 5/5/2018 2:28 PM, Prasanta Sadhukhan wrote:
>
>> Hi All,
>>
>> I have tried to address all comments from Nir, Phil, Mandy and Kevin got
>> so far in this webrev
>> http://cr.openjdk.java.net/~psadhukhan/fxswing.7
>>
>> >>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.
>> If @override is not added, then it will cause recursion between
>> LightweightContent.java:130 and LightweightContent.java:187 thereby causing
>> StackOverflowError. I am also not sure about the comment of skipping
>> @Override but I have removed it from this webrev.
>>
>> Regards
>> Prasanta
>> On 5/5/2018 4:32 AM, Kevin Rushforth wrote:
>>
>>> 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
>>>>>
>>>>> ||
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>