[8] Please review my fix for 7124247: [macosx] Implement GraphicsDevice.setDisplayMode()
Alexander Zuev
alexander.zuev at oracle.com
Tue Jun 5 08:29:03 PDT 2012
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