Hi Anton,
Hello Pavel,
Thank you for the response with remarks. Yes, one additional "catch
(RuntimeException re)" block was added in my previous webrev. I
decided to add such a block, because of two following reasons:
1. To make an output stack trace shorter, but still containing a stack
trace of the expected NullPointerException, in case if such exception
is caught. Otherwise the final stack trace will contain 3 exceptions,
where only the third one is informational. This change simplifies
manual testing by diminishing a number of lines necessary for reading.
2. To eliminate extra folding of RuntimeException into a new
RuntimeException, which did not happen at all.
Why don't you catch just one IOException? I think it will solve both the
problem described above and will look shorter and clearer for other people?
Regards, Pavel
A similar approach was applied to "try/catch" block in "serializeGUI"
method for making the whole test consistent in the current webrev.
Thank you,
Anton
On 31.08.2012 14:48, Pavel Porvatov wrote:
Hi Anton,
I see you added
57 } catch (RuntimeException re) {
58 throw re;
in serializeGUI method. Is it necessary? I think lines 72 and 73 can
be removed as well... I didn't noticed that code in the previous
webrev, unfortunately.
Regards, Pavel
Hello Pavel,
Could you review a new webrev with code changes reflecting your last
remark concerning a substitution of File streams for Byte streams in
the test class. The test was checked using "jtreg". It fails on JDK
7 without this fix, and passes on JDK 7 with fix. URL of the new
webrev is the following.
Webrev: http://cr.openjdk.java.net/~alitvinov/7193219/webrev.03
Thank you,
Anton
On 29.08.2012 20:54, Anton Litvinov wrote:
Hello Pavel,
Yes, the test fails on JDK 7 without fix and does not fail on JDK 7
with fix. Yes, I will definitely prefer to substitute the code
writing data to files for a code writing data to byte streams.
Thank you,
Anton
On 29.08.2012 19:04, Pavel Porvatov wrote:
Hi Anton,
Does the test fail without the fix?
All code look good except one thing: could you please replace
FileStreams by Byte streams? Regression tests should avoid to
change file system, and if there is such need tests should restore
file system in initial state
Hello Pavel,
Thank you very much for a positive review of the fix and detailed
remarks for the test. Could you review a corrected version of the
test in a new webrev. I would like to clarify that a previous
version of the test contained some irrelevant components, because
I was not sure if I could significantly modify the original test
case provided with the bug itself. URL of a webrev with the test
class corrected according to your remarks is provided below.
Webrev: http://cr.openjdk.java.net/~alitvinov/7193219/webrev.02
The modified test was checked using "jtreg" tool on JDK with and
without this fix, and the test behaved properly. The following
changes were made:
1. Setting visibility of frames was removed, because it does not
influence reproducibility of the test.
2. All components except JPanel and JComboBox were eliminated
from the test.
3. Frames' titles were corrected.
4. Method "test" was renamed and moved into EDT execution block.
5. Method "createAndShowGUI" was renamed.
6. Calls to JFrame's "dispose" method were added.
Thank you,
Anton
On 28.08.2012 21:45, Pavel Porvatov wrote:
Hi Anton,
The fix looks good but the test should be updated:
1. Is the following line needed for the test:
63 frame.setVisible(true);
?
If yes then you should use SunToolkit.realSync to wait until
frame become visible.
The same comment for the line
85 frame.setVisible(true);
2. Are label and layout and layout needed for the test purpose?
If no, can you remove unused components?
3. The following code looks strange (two titles?)
45 JFrame frame = new JFrame("HelloWorldSwing");
...
47 frame.setTitle("why why why");
4. The test method should be on the EDT
5. There is no need to use the mainPanel field. Logically it
should be local variable
BTW: can you start with the fix for jdk8 and only then backport
it to jdk7?
Regards, Pavel
Hello,
Please review the following fix for a bug.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7193219
Webrev: http://cr.openjdk.java.net/~alitvinov/7193219/webrev.01
For details on this bug please look at "Evaluation" field on a
web page of this bug. The provided webrev contains both a fix
and a corresponding unit-test. Before publishing this webrev
all unit-tests from the "java.awt" and
"javax.swing" related to serialization were run and no negative
changes were observed comparing the results of tests' runs on
JDK with and without patch represented by this webrev.
This is the second version of the fix. The first version was
submitted to the AWT development group through a review request
with the same subject and after discussion with engineers and
additional investigation of the problem it was decided to apply
the fix in Swing part of JDK.
Thank you,
Anton
