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

[Mike Swingler]

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.

2. getBestModeForParameters() is declared to return a CGDisplayModeRef, but you are actually returning a bestGuess CFStringRef. Which is it?

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.

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?

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.

6. Your NSLog() in Java_sun_awt_CGraphicsDevice_nativeGetDisplayModes() says "CGRaphivsGevice".
	- I think you meant to say "CGraphicsDevice".

7. None of the if checks in getBestModeForParameters() need else { } blocks, because they implicitly exit the for loop.

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.

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>

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?

Overall, I'm discouraged by this lack of attention to detail by both the author and reviewers.

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
>