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

Anthony Petrov anthony.petrov at oracle.com
Wed Jun 6 04:53:46 PDT 2012 

Thank you Alex. The fix looks good to me.

--
best regards,
Anthony

On 6/5/2012 10:11 PM, Alexander Zuev wrote:
> Ok, if you both say so - i'm convinced :)
> 
> Here's the new webrev:
> http://cr.openjdk.java.net/~kizune/7124247/jdk8/webrev.04
> 
> With best regards,
> Alex
> 
> On 6/5/12 21:21, Mike Swingler wrote:
>> Agreed. A separate function is best. Otherwise this looks good.
>>
>> Mike Swingler
>> Apple Inc.
>>
>> On Jun 5, 2012, at 8:49 AM, Anthony Petrov wrote:
>>
>>> 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