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

[Petr Pchelko]

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> 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> 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>
>>>> 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/84819c13/attachment-0001.html>