<AWT Dev> [8] Review request for 7092283 Property Window.locationByPlatform is not cleared by calling setVisible(false)

[Anthony Petrov]

Looks fine. Thank you.

--
best regards,
Anthony

On 09/30/2013 04:48 PM, Alexander Scherbatiy wrote:
>
>    Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/7092283/webrev.02/
>      - synchronization on tree lock is used instead of the setter method
>      - the readObject() method is not modified
>
>    Thanks,
>    Alexandr.
>
>
> On 9/27/2013 5:59 PM, Anthony Petrov wrote:
>> On 09/27/2013 05:03 PM, Alexander Scherbatiy wrote:
>>> On 9/27/2013 4:24 PM, Anthony Petrov wrote:
>>>> I'm not sure if this is the right approach because the setter is
>>>> overridable. So whenever you update the field, you may actually call
>>>> user's code which in most cases is undesirable. Also, you really
>>>> shouldn't do this in the context of readObject() because the object
>>>> isn't fully initialized yet, so calling overridable methods at this
>>>> stage is asking for troubles.
>>>>
>>>> I still suggest to access the field directly, but wrap the accessing
>>>> statement with synchronized (getTreeLock()) {}.
>>>      Is it a common practice in awt to use synchronized block directly
>>> instead of the setter method?
>>
>> Yes, in AWT we rarely (if ever) use public overridable setters to
>> change fields. Usually we access the fields directly.
>>
>>>      If a user's code does not use synchronization for the
>>> locationByPlatform setting he really violates the AWT design for this
>>> variable.
>>
>> User code can't access the private field directly. They can only call
>> the setter (as super.set...() for example). Hence, user code can't
>> violate the synchronization scheme directly. But our own code can, and
>> in fact often does, which needs fixing. Hence my suggestion.
>>
>> --
>> best regards,
>> Anthony
>>
>>
>>>
>>>     Thanks,
>>>     Alexandr.
>>>
>>>
>>>> And I wouldn't modify the readObject() at all.
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>> On 09/27/2013 02:33 PM, Alexander Scherbatiy wrote:
>>>>>
>>>>>    Could you review the updated fix:
>>>>>      http://cr.openjdk.java.net/~alexsch/7092283/webrev.01
>>>>>
>>>>>    - the locationByPlatform property is updated by setter method
>>>>>    - the open and closed tests are passed with the fix except one
>>>>> which
>>>>> fails without the fix too:
>>>>> java/awt/Dialog/MakeWindowAlwaysOnTop/MakeWindowAlwaysOnTop.java
>>>>>
>>>>>   Thanks,
>>>>>   Alexandr.
>>>>>
>>>>> On 9/25/2013 5:37 PM, Anthony Petrov wrote:
>>>>>> Hi Alexander,
>>>>>>
>>>>>> The setter and getter for this property access the boolean flag under
>>>>>> the tree lock. I suppose this was the locking design for this field.
>>>>>> So you should reset it under this lock, too. You could also go ahead
>>>>>> and add proper locking to other methods that access it (e.g. show(),
>>>>>> etc.)
>>>>>>
>>>>>> Please run Window, Frame, and Dialog automatic regression tests (open
>>>>>> and closed) to ensure no regressions after this change.
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>> On 09/25/2013 04:50 PM, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review the fix:
>>>>>>>    bug: https://bugs.openjdk.java.net/browse/JDK-7092283
>>>>>>>    webrev: http://cr.openjdk.java.net/~alexsch/7092283/webrev.00
>>>>>>>
>>>>>>>    The Window.hide() method does not clear the locationByPlatform
>>>>>>> property.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>
>>>
>