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.
