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

[Pavel Porvatov]

Hi Anton,

Looks good for me

Regards, Pavel
> Hello Pavel,
>
> Please look at a new webrev with corrections reflecting your remarks. 
> Extra "catch (RuntimeException)" blocks were eliminated. On line 70, 
> only "IOException" cannot be used, because "ClassNotFoundException" 
> should be handled also.
>
> Webrev: http://cr.openjdk.java.net/~alitvinov/7193219/webrev.04
>
> Thank you,
> Anton
>
> On 31.08.2012 17:49, Pavel Porvatov wrote:
>> 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
>>>>>>>>>
>>>>>>>
>>>>
>>