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

[Anton Litvinov]

Hello Pavel,

Thank you very much for an approval of the last webrev.

Anton

On 31.08.2012 19:41, Pavel Porvatov wrote:
> 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
>>>>>>>>>>
>>>>>>>>
>>>>>
>>>
>