<AWT Dev> [9] Review Request: JDK-6463901 Either generify or deprecate sun.awt.EventListenerAggregate
Anthony Petrov
anthony.petrov at oracle.com
Wed Mar 26 12:10:14 UTC 2014
Thanks for the clarification. The fix looks good to me otherwise.
--
best regards,
Anthony
On 3/26/2014 3:51 PM, Petr Pchelko wrote:
> 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
> <mailto: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>
>>> <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>
>>>> <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>
>>>>>>> <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.
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>
>
More information about the awt-dev
mailing list