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

Anthony Petrov anthony.petrov at oracle.com
Tue Jun 5 08:49:53 PDT 2012 

Code replication allows for fixing a bug in one place and forgetting 
about it in another. Extracting common code in functions prevents such 
issues.

Also, in this particular case we would benefit from caching a method ID 
only once instead of having two copies of it.

--
best regards,
Anthony

On 6/5/2012 7:29 PM, Alexander Zuev wrote:
> Yes, they are similar, but making a new function for the 8 lines of 
> code? Don't think it's worth it.
> 
> With best regards,
> Alex
> 
> On 6/5/12 18:38, Anthony Petrov wrote:
>> 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