Hello, Artem. >> As Anthony said it's just for limiting visibility. I didn’t understand this actually.
The new version looks good to me. With best regards. Petr. > On Jul 1, 2014, at 10:40 PM, Anthony Petrov <anthony.pet...@oracle.com> wrote: > > Hi Artem, > > Note that assert() [1] is still not a run-time check because we specify > -DNDEBUG when building Release binaries. To make it a runtime check, the code > should check the condition explicitly, and if it's false, then raise a Java > exception (e.g. AWTError) and return from the JNI method. However, I realize > that our current code never fails the assertion anyway. So I'm leaving this > issue up to you and other reviewers, in case anyone feels strongly about this. > > Other than that, the fix looks fine to me. Thanks for your hard work. > > > [1] http://msdn.microsoft.com/en-us/library/9sb57dw4(v=vs.100).aspx > > -- > best regards, > Anthony > > On 7/1/2014 6:34 PM, artem malinko wrote: >> Hi, Antony and Petr! Thank you for detailed review. I Hope this version >> is better. >> >> Link: >> http://cr.openjdk.java.net/~mcherkas/artem/webrev.05/ >> >> Petr: >> "4. Lines 37-43. Normally we do not use blocks to group the code together." >> >> As Anthony said it's just for limiting visibility. But maybe code logic >> would be more clear if explicitly null list reference, so I changed it. >> >> "5. Are you sure that you do not need to wait for a frame to actually >> show up on the screen so that all the peers are guaranteed to get created?" >> I'm pretty sure. List peer will be created if it's container peer not >> null. And container peer definitely not null at this stage because it >> was created in the same thread when we invoked setVisible() on JFrame. >> >> Everything else is adjusted according recommendations. >> >> On 30.06.2014 19:14, Anthony Petrov wrote: >>> Hi Artem, >>> >>> 1. Your code still uses wrong formatting. Please just open this page >>> to see the problem with the lines indentation: >>> >>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/src/windows/native/sun/windows/awt_Component.cpp.sdiff.html >>> >>> >>> >>> 2. DASSERT() is only relevant for debug builds which we use very >>> rarely. I'd prefer to make this a run-time check. To compensate for >>> possible performance degradation, I suggest to place it to the else{} >>> branch of your if() statement, so that the check is only performed >>> when it's really needed. >>> >>> >>> 3. A standard copyright header in the test file is missing. Please see >>> other tests for examples. >>> >>> 4. The test should also contain a @bug jtreg tag and mention the >>> concrete bug id that's being verified with this test. >>> >>> 5. The dispose() call is better placed to the finally{} block of the >>> try{} statement to ensure it's always executed. >>> >>> 6. You don't really need a System.exit() call in your test. If the >>> frame is dispose()'d in the finally{} block, the test will terminate >>> by itself. >>> >>> 7. In the catch{} block in the test() method, the if() statements >>> should either be one-liners, or enclose their "then" branches into >>> blocks {}. >>> >>> Also, Petr wrote: >>>> 4. Lines 37-43. Normally we do not use blocks to group the code >>>> together. >>> >>> I think this is okay in this case. A block is used to limit the >>> visibility of the strong reference to the List object. We need it to >>> "convert" it into a WeakReference. >>> >>> >>>> Where is the original reference created? >>> >>> It's created in the same method - CreateHWND(). Please examine the >>> code in AwtList to see that we only need to recreate the native >>> control (i.e. the hwnd) when an app toggles the multiple selection >>> property. So the code and the fix make sense to me. Perhaps we should >>> add a comment before the "if (m_peerObject == NULL)" check to explain >>> why we do this. >>> >>> >>> -- >>> best regards, >>> Anthony >>> >>> On 6/30/2014 2:17 PM, artem malinko wrote: >>>> Thank you, Anthony. >>>> >>>> Yes, I think assertion won't be superfluous to prevent other bugs of >>>> same type. Here is a version with assert. >>>> >>>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/ >>>> >>>> On 27.06.2014 1:12, Anthony Petrov wrote: >>>>> Hi Artem, >>>>> >>>>> Please configure you code editor so that it formats the code that you >>>>> modify according to Java code conventions used in OpenJDK (4-spaces >>>>> line indentation, a space after "if" and before "{", etc.) >>>>> >>>>> Also, please include the bug id and synopsis to the subject line of >>>>> your review requests. Please see other review threads on this mailing >>>>> list for examples. >>>>> >>>>> As for the fix itself, should we add an assertion check to the >>>>> CreateHWnd() method to verify that both peer and m_peerObject refer to >>>>> the same Java object in case the latter is already set? >>>>> >>>>> -- >>>>> best regards, >>>>> Anthony >>>>> >>>>> On 6/26/2014 7:30 PM, artem malinko wrote: >>>>>> Hello, AWT Team. >>>>>> >>>>>> Please review a fix for the issue: >>>>>> https://bugs.openjdk.java.net/browse/JDK-8040076 >>>>>> The fix is available at: >>>>>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.01/ >>>>>> >>>>>> When method "void AwtList::SetMultiSelect" is invoked it invokes "void >>>>>> AwtComponent::CreateHWnd" where m_peerObject initialized. But at this >>>>>> stage m_peerObject already initialized and already holds ref to java >>>>>> List object. So original m_peerObject is lost and ref to java List >>>>>> lost >>>>>> as well. In the fix I've added check whether m_peerObject is >>>>>> initialized >>>>>> or not. >>>>>> >>>>>> Thank you. >>>> >>
