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

[Anthony Petrov]

Hi Alex,

1. src/macosx/native/sun/awt/CGraphicsDevice.m
>  35         // This is a strange mode, where we using 10 pixels per color and pack it into 32 bits

s/where we.../in which we use 10 bits per color component and pack them 
into.../

>  59     for(n = 0; n < numModes; n++ ) {
>   60         CGDisplayModeRef cRef = (CGDisplayModeRef) CFArrayGetValueAtIndex(allModes, n);
>   61         CFStringRef modeString = CGDisplayModeCopyPixelEncoding(cRef);
>   62         thisBpp = getBPPFromModeString(modeString);
>   63         CFRelease(modeString);
>   64         if (cRef == NULL || thisBpp != bpp || CGDisplayModeGetHeight(cRef) != h || CGDisplayModeGetWidth(cRef) != w) {

It looks like it's a bit late to check cRef for NULL at line 64 since 
we've already used it at line 61. :)

>  135     static JNF_CLASS_CACHE(jc_DisplayMode, "java/awt/DisplayMode");
>  136     static JNF_MEMBER_CACHE(getWidthMethod, jc_DisplayMode, "getWidth", "()I");
>  137     static JNF_MEMBER_CACHE(getHeightMethod, jc_DisplayMode, "getHeight", "()I");
>  138     static JNF_MEMBER_CACHE(getBitDepthMethod, jc_DisplayMode, "getBitDepth", "()I");
>  139     static JNF_MEMBER_CACHE(getRefreshRateMethod, jc_DisplayMode, "getRefreshRate", "()I");
>  140 
>  141     w = JNFCallIntMethod(env, newMode, getWidthMethod);
>  142     h = JNFCallIntMethod(env, newMode, getHeightMethod);
>  143     bpp = JNFCallIntMethod(env, newMode, getBitDepthMethod);
>  144     refrate = JNFCallIntMethod(env, newMode, getRefreshRateMethod);

I suggest to do that in Java and pass all the parameters to the native 
method. We simply don't need the newMode reference in native.

> 182     ret = JNFNewObject(env, jc_DisplayMode_ctor, w, h, bpp, refrate);

I haven't read the spec for public methods yet, but just want to check: 
do we have to return a new object each time the current mode is 
requested, or should the returned object belong to a cached array of 
supported display modes?

If we have to/may return a new object though, then I suggest to minimize 
code duplication between the getDisplayMode and getDispalyModes 
implementations since both create these objects.

--
best regards,
Anthony

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