<AWT Dev> [9] Review Request: JDK-6463901 Either generify or deprecate sun.awt.EventListenerAggregate

Petr Pchelko petr.pchelko at oracle.com
Wed Mar 26 11:51:49 UTC 2014 

Hello, Anthony.

Of course. This left from one of the pervious versions. Thank you for noticing it. 
I'll remove the synchronized before the push. 

With best regards. Petr.


On 26.03.2014, at 15:32, Anthony Petrov <anthony.petrov at oracle.com> wrote:

> Hi Petr,
> 
> Thanks for the update.
> 
> src/share/classes/sun/awt/datatransfer/SunClipboard.java
>> 422     public synchronized void checkChange(long[] formats) {
> 
> I'm concerned about making this method synchronized. I haven't investigated deeply, but it doesn't seem that the checkChange() has called any methods that synchronize on the Clipboard instance previously. And even if it has, it doesn't do that for the whole time of executing this method. Your fix changes the threading contract of this method considerably and could potentially lead to deadlocks and whatnot.
> I suggest to reevaluate this part of the change.
> 
> --
> best regards,
> Anthony
> 
> On 3/26/2014 12:13 PM, Petr Pchelko wrote:
>> Hello,
>> 
>> Looks like I forgot to actually add a link to the new version. Here it
>> is: http://cr.openjdk.java.net/~pchelko/9/6463901/webrev.02/
>> 
>> With best regards. Petr.
>> 
>> 
>> On 19.03.2014, at 12:44, Petr Pchelko <petr.pchelko at oracle.com
>> <mailto:petr.pchelko at oracle.com>> wrote:
>> 
>>> Hello, Anthony, Sergey.
>>> 
>>> Thank you for the review.
>>> 
>>>> Now that the flavorListeners is never null, I believe that the
>>>> condition should check the value of currentDataFlavors, rather than
>>>> rely on emptiness of the listeners collection: note that there's
>>>> removeListener method, so the collection may become empty again, and
>>>> we don't want to reinitialize the currentDataFlavors for the second time.
>>> I'm now initializing it in the constructor, the same time as the list
>>> is initialized.
>>> 
>>>> As for the Set vs. List issue, note that Set implementations
>>>> generally do not guarantee a particular iteration order - it may
>>>> change after adding/removing listeners. This is particularly true for
>>>> hash-based collections. So if we use a HashSet, listeners added by
>>>> other code could be executed in different order at different times
>>>> which could confuse some applications (maybe?). Since we don't expect
>>>> a large number of listeners to be added to the collection, I don't
>>>> think that removing a listener would be a bottle-neck (and this is
>>>> the only operation that actually benefits from using HashSet in this
>>>> code). So personally, I'd choose a List instead. But I don't have a
>>>> strong opinion on this.
>>> I don't think the iteration order would have any effect on the
>>> applications. The only reason I'm choosing a Set is to avoid
>>> duplicated notifications in case 2 identical listeners were
>>> accidentally added. It's actually would be a bug in the client code,
>>> so I'm OK to replace the Set with the List if you think it would be
>>> better.
>>> 
>>> With best regards. Petr.
>>> 
>>> On 18.03.2014, at 23:07, Anthony Petrov <anthony.petrov at oracle.com
>>> <mailto:anthony.petrov at oracle.com>> wrote:
>>> 
>>>> Hi Petr,
>>>> 
>>>>> 259         if (flavorListeners.isEmpty()) {
>>>>> 260             currentDataFlavors = getAvailableDataFlavorSet();
>>>>> 
>>>>> 261         }
>>>>> 262         flavorListeners.add(listener);
>>>> 
>>>> Now that the flavorListeners is never null, I believe that the
>>>> condition should check the value of currentDataFlavors, rather than
>>>> rely on emptiness of the listeners collection: note that there's
>>>> removeListener method, so the collection may become empty again, and
>>>> we don't want to reinitialize the currentDataFlavors for the second time.
>>>> 
>>>> As for the Set vs. List issue, note that Set implementations
>>>> generally do not guarantee a particular iteration order - it may
>>>> change after adding/removing listeners. This is particularly true for
>>>> hash-based collections. So if we use a HashSet, listeners added by
>>>> other code could be executed in different order at different times
>>>> which could confuse some applications (maybe?). Since we don't expect
>>>> a large number of listeners to be added to the collection, I don't
>>>> think that removing a listener would be a bottle-neck (and this is
>>>> the only operation that actually benefits from using HashSet in this
>>>> code). So personally, I'd choose a List instead. But I don't have a
>>>> strong opinion on this.
>>>> 
>>>> --
>>>> best regards,
>>>> Anthony
>>>> 
>>>> On 3/18/2014 7:47 PM, Sergey Bylokhov wrote:
>>>>> On 3/18/14 7:44 PM, Petr Pchelko wrote:
>>>>>> Hello, Sergey.
>>>>>> 
>>>>>> Thank you for the review.
>>>>>> The updated version is located here:
>>>>>> http://cr.openjdk.java.net/~pchelko/9/6463901/webrev.01/
>>>>>> 
>>>>>> It addresses both of your comments.
>>>>> I am curious why Set and not a List(ArrayList)?
>>>>>> 
>>>>>> With best regards. Petr.
>>>>>> 
>>>>>> On 18.03.2014, at 19:06, Sergey Bylokhov
>>>>>> <Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>>
>>>>>> wrote:
>>>>>> 
>>>>>>> Hi, Petr.
>>>>>>> 
>>>>>>> A few notes:
>>>>>>> 
>>>>>>> 314         if (prevDataFlavors != null && currentDataFlavors != null
>>>>>>> 315                 && prevDataFlavors.equals(currentDataFlavors)) {
>>>>>>> 316             return;
>>>>>>> 317         }
>>>>>>> 
>>>>>>> I suppose we should return in case both of them will be null?
>>>>>>> Objects.equals should be a little bit more readable here.
>>>>>>> 
>>>>>>> 
>>>>>>> 440                 flavorListeners.stream()
>>>>>>> 441                         .filter(Objects::nonNull)
>>>>>>> 442                         .forEach(listener ->
>>>>>>> SunToolkit.postEvent(appContext,
>>>>>>> 443                                 new PeerEvent(this,
>>>>>>> 444                                         () ->
>>>>>>> listener.flavorsChanged(new FlavorEvent(SunClipboard.this)),
>>>>>>> 445
>>>>>>>                                        PeerEvent.PRIORITY_EVENT)));
>>>>>>> 
>>>>>>> here is a place where we can reformat it to be more readable.
>>>>>>> 
>>>>>>> On 18.03.2014 18:34, Petr Pchelko wrote:
>>>>>>>> Hello, AWT Team.
>>>>>>>> 
>>>>>>>> Please review the fix for the issue:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6463901
>>>>>>>> The fix is available at:
>>>>>>>> http://cr.openjdk.java.net/~pchelko/9/6463901/webrev/
>>>>>>>> 
>>>>>>>> The bug states that we should deprecate or generify the
>>>>>>>> EventListenerAggregate class.
>>>>>>>> However it's an internal class in sun.awt package so we could remove
>>>>>>>> it.
>>>>>>>> 
>>>>>>>> I've used grep on JDK source to verify that this class is not used
>>>>>>>> any more.
>>>>>>>> Clipboard regression, functional and JCK tests run fine.
>>>>>>>> 
>>>>>>>> With best regards. Petr.
>>>>>>>> 
>>>>>>> 
>>>>>>> --
>>>>>>> Best regards, Sergey.
>>>>>>> 
>>>>> 
>>>>> 
>>> 
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20140326/ccedd4f7/attachment-0001.html>

More information about the awt-dev mailing list