[8] Please review my fix for 7124247: [macosx] Implement GraphicsDevice.setDisplayMode()

Alexander Zuev alexander.zuev at oracle.com
Tue Jun 5 06:46:37 PDT 2012 

Yet another version of the webrev - fixed some typos in comments (thanks 
Scott Palmer) and
added updating full screen window size if we are currently in full 
screen mode.

Webrev is here: http://cr.openjdk.java.net/~kizune/7124247/jdk8/webrev.03

With best regards,
Alex

On 6/4/12 22:18, Alexander Zuev wrote:
> Mike,
>
>   thanks for a superb review. See comments inline.
>
> With best regards,
> Alex
>
> On 6/1/12 23:18, Mike Swingler wrote:
>> A few notes here:
>>
>> 1. getBestModeForParameters() should be called 
>> copyBestModeForParameters()
>>     - This is to pass the static-analyzer to validate that the 
>> retain/release balance is correct: copy implies returning a retained 
>> object, get implies non-retained. In this case you are returning a 
>> retained object that needs to be released.
> I wasn't aware that static-analyzer takes into account the function 
> names. Anyways - in this case you are quite wrong. I'm not creating 
> any new objects here. What i get as a parameter is an array of the 
> references (pointers) and i'm returning one of them
> that satisfy the conditions based on the other parameters passed. 
> There is no need in releasing the object referred by that
> pointer - quite contrary if i DO release closestMatch in the 
> Java_sun_awt_CGraphicsDevice_nativeSetDisplayMode then next
> time i try to set the same display mode i will get the nice 
> application crash. Something like this:
>
> 0   libsystem_kernel.dylib            0x00007fff85c55ce2 
> __pthread_kill + 10
> 1   libsystem_c.dylib                 0x00007fff8b42e7d2 pthread_kill 
> + 95
> 2   libsystem_c.dylib                 0x00007fff8b41fa7a abort + 143
> 3   libjvm.dylib                      0x0000000102fadb79 
> os::abort(bool) + 25
> 4   libjvm.dylib                      0x0000000103099dd0 
> VMError::report_and_die() + 2306
> 5   libjvm.dylib                      0x0000000102faf255 
> JVM_handle_bsd_signal + 1047
> 6   libsystem_c.dylib                 0x00007fff8b480cfa _sigtramp + 26
> 7   com.apple.CoreFoundation          0x00007fff8d125877 
> CFDictionaryGetValue + 23
> 8   com.apple.CoreGraphics            0x00007fff83b0daec 
> CGDisplayModeGetResolution + 40
> 9   com.apple.CoreGraphics            0x00007fff83b0f84f 
> CGDisplayCopyAllDisplayModes + 278
> 10  liblwawt.dylib                    0x000000015fc1b35d 
> Java_sun_awt_CGraphicsDevice_nativeSetDisplayMode + 57
>
>> 2. getBestModeForParameters() is declared to return a 
>> CGDisplayModeRef, but you are actually returning a bestGuess 
>> CFStringRef. Which is it?
> Glitch. Fixed it. Strange that compiler did not issued a warning about 
> this return
>> 3. getBPPFromModeString() returns an int, but in one case you are 
>> assigning to a jint in 
>> Java_sun_awt_CGraphicsDevice_nativeGetDisplayMode().
>>     - Technically not an issue, but unexpected.
> I can add implicit cast to jint - but technically it is not an issue 
> as you mentioned .
>> 4. In Java_sun_awt_CGraphicsDevice_nativeGetDisplayMode() you call 
>> CGDisplayModeGetRefreshRate(currentMode), but don't do anything with 
>> the return value, then later call it again, assigning it to the jint 
>> refrate.
>>     - What's up with this? Why call it twice?
> Another glitch. Friday evening syndrome.
>> 5. In some places you use (*env)->FindClass(), and in others you use 
>> JNF_CLASS_CACHE().
>>     - You can create array types using JNFNewObjectArray() and 
>> JNF_CLASS_CACHE(), which will benefit from fewer lines, and automatic 
>> exception throwing into Java, instead of just printing and 
>> suppressing the exception.
> Well, valid point - i'll use the JNF functions for array creation for 
> the sake of consistency.
>> 6. Your NSLog() in 
>> Java_sun_awt_CGraphicsDevice_nativeGetDisplayModes() says 
>> "CGRaphivsGevice".
>>     - I think you meant to say "CGraphicsDevice".
> You're absolutely right.
>> 7. None of the if checks in getBestModeForParameters() need else { } 
>> blocks, because they implicitly exit the for loop.
> I've added them for the better code readability but as it makes code 
> more compact i may as well remove them.
>> 8. getBPPFromModeString() can simply return the depth directly 
>> without assigning to a temporary.
>>     - I know this is more of a style thing, but I prefer fewer lines 
>> if possible.
> Hmm. Ok, not a big deal but if it makes you a little bit happier i'll 
> surely do that.
>> 9. Java_sun_awt_CGraphicsDevice_nativeSetDisplayMode() leaks the 
>> closestMatch CGDisplayModeRef.
>>     - The "Create Rule" explains why this is a leak when fixed in 
>> conjunction with (1) above.
>>     -<https://developer.apple.com/library/mac/#documentation/CoreFOundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-103029> 
>>
> How comes? I do not use Copy-methods to get the exact configuration, i 
> get the array of references to the Quartz DisplayMode's
> and i don't create any new CGDisplayMode objects. And i do release the 
> array itself. Where is the memory leak?
>
>> 10. What happens in the case of 
>> Java_sun_awt_CGraphicsDevice_nativeGetDisplayModes() where one of the 
>> CGDisplayModeRef's itterated on in the for loop is null? Is it OK to 
>> have empty nil holes in the returned array?
> I tried to pass the null as one of the array elements back to Java - 
> seems that it's Ok so i will left it as it is now.
>> Overall, I'm discouraged by this lack of attention to detail by both 
>> the author and reviewers.
> Stuff happens, especially at Friday evening, but all for all there was 
> no major problems just some non-critical glitches here and there. And 
> you are one of reviewers so the review process is fairly good at the end.
>
> Anyways - the new webrev can be found here 
> http://cr.openjdk.java.net/~kizune/7124247/jdk8/webrev.02
> Comments are welcome.
>
>
>>
>> Mike Swingler
>> Apple Inc.
>>
>> On Jun 1, 2012, at 10:55 AM, Alexander Zuev wrote:
>>
>>> Artem,
>>>
>>>   1. Done
>>>   2. Yes, it's a wonderful idea, done.
>>>   3. User can construct a new DisplayMode object and ask us to set 
>>> corresponding mode.
>>>
>>>   New webrev can be found at 
>>> http://cr.openjdk.java.net/~kizune/7124247/jdk8/webrev.01
>>>
>>> With best regards,
>>> Alex
>>>
>>> On 6/1/12 19:57, Artem Ananiev wrote:
>>>> Hi, Alex,
>>>>
>>>> here are some comments:
>>>>
>>>> 1. Please, update copyright dates in file headers.
>>>>
>>>> 2. I would pass w, h, bpp and refrate as method params to 
>>>> setDisplayMode() to avoid extra JNI calls from native
>>>>
>>>> 3. Is it possible to call setDisplayMode() with a DisplayMode 
>>>> object that was not obtained via previous getDisplayModes() call? 
>>>> If the answer is "no", why don't you just check for exact match for 
>>>> all parameters (w, h, bpp, refrate) in getBestModeForParameters()?
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>> On 6/1/2012 1:40 PM, Alexander Zuev wrote:
>>>>> Hello,
>>>>>
>>>>> please review my fix for CR 7124247: [macosx] Implement
>>>>> GraphicsDevice.setDisplayMode()
>>>>>
>>>>> CR description can be found here:
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7124247
>>>>>
>>>>> Webrev with the proposed change can be seen here:
>>>>> http://cr.openjdk.java.net/~kizune/7124247/jdk8/webrev.00
>>>>>
>>>>> With best regards,
>>>>> Alex
>

More information about the macosx-port-dev mailing list