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.pet...@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.pche...@oracle.com
>> <mailto:petr.pche...@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.pet...@oracle.com
>>> <mailto:anthony.pet...@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.bylok...@oracle.com <mailto:sergey.bylok...@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.
>>>>>>>
>>>>>
>>>>>
>>>
>>
