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

[Anthony Petrov]

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.
>>>>>
>>>
>