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

[Alexander Scherbatiy]

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