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

[Sergey Bylokhov]

On 27.09.2013 16:24, Anthony Petrov wrote:
> Hi Alexander,
>
> 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()) {}. And I wouldn't modify 
> the readObject() at all.
I suppose to fix readObject the field can be marked volatile.
>
> -- 
> 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.
>>>>
>>


-- 
Best regards, Sergey.