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

Anthony Petrov anthony.petrov at oracle.com
Tue Jun 5 07:38:01 PDT 2012 

Hi Alex,

The code at lines 163-171 and 202-209 is very similar, and I believe it 
can be extracted into a separate utility function.

--
best regards,
Anthony

On 6/5/2012 5:46 PM, Alexander Zuev wrote:
> 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