<AWT Dev> [8] Request for review: 7193214 Consider simplifying CPlatformWindow.setResizable()

[Denis Fokin]

The fix looks ok. A separate Cr seems a good idea to me.

Thank you,
    Denis.

09.11.2012, в 16:23, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> написал(а):

> I can file separate CR to ,move this logic to the java lvl, cache it if needed and convert native methods to the simple setters?
> 
> 09.11.2012 15:00, Denis S. Fokin wrote:
>> Hi Sergey,
>> 
>> The fix looks fine. But I would slightly change the implementation of the method.
>> 
>> Instead of
>> 
>> jint newBits = window.styleBits & ~mask | bits & mask;
>> 
>> I would use
>> 
>> Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits
>> (JNIEnv *env, jclass clazz, jlong windowPtr, jint mask, jboolean value)
>> 
>> // ....
>> // ....
>> // ....
>> 
>> jint newBits = (value) ? (window.styleBits | mask) : (window.styleBits & ~mask);
>> 
>> Because, now we are trying to encode the boolean value in java and decode it in the native code. It is the only purpose of the 'bits' variable. It would be better to propagate the boolean value to the native code.
>> 
>> I am not sure about window.styleBits value. If it cannot be changed by the system. I would cache it in java and do all the bitwise operations on the java level.
>> 
>> 
>> Thank you,
>>   Denis.
>> 
>> On 11/7/2012 8:03 PM, Sergey Bylokhov wrote:
>>> Hi, Anthony.
>>> Since this check is not necessary for the fix, I just drop it.
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~serb/7193214/webrev.01/
>>> GUI jck tests passed.
>>> 
>>> 06.11.2012 19:34, Anthony Petrov wrote:
>>>> Hi Sergey,
>>>> 
>>>> Generally I'm fine with nativeSetNSWindowStyleBits updating the bits
>>>> only when they've changed. Did you run AWT regression tests for
>>>> top-level areas (both open and closed) to make sure there's no
>>>> regressions associated with this change? Shouldn't we force
>>>> reinstalling some style bits when e.g. a window gets hidden and then
>>>> subsequently shown again?
>>>> 
>>>> -- 
>>>> best regards,
>>>> Anthony
>>>> 
>>>> On 11/6/2012 6:54 PM, Sergey Bylokhov wrote:
>>>>> 
>>>>> Hi Everyone,
>>>>> Please review the fix.
>>>>> In the fix, unnecessary calls were removed from setResizable(), also
>>>>> now we take into account zoom state in the setResizable() + plus
>>>>> small cleanup;
>>>>> 
>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7193214
>>>>> Webrev can be found at:
>>>>> http://cr.openjdk.java.net/~serb/7193214/webrev.00
>>>>> 
>>>>> -- 
>>>>> Best regards, Sergey.
>>> 
>>> 
>> 
> 
> 
> -- 
> Best regards, Sergey.
>