<Swing Dev> [7u8] Review request for 7193219: JComboBox serialization fails in JDK 1.7

[Pavel Porvatov]

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
>>>>>>>
>>>>>
>>