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.
