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

[Alexander Zuev]

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