<Swing Dev> REM: Re: [9-client] Review request for bug 7124238 : [TEST_BUG] Font in BasicHTML document is bigger than it should be

[Alexander Zvegintsev]

Hi Shilpi,
the fix looks good to me

Thanks,

Alexander.

On 08/31/2015 12:00 PM, shilpi rastogi wrote:
> Hi All,
>
> Please review updated webrev
>
> open: 
> http://cr.openjdk.java.net/~psadhukhan/shilpi/webrev-7124238/open-7124238/webrev/ 
>
> closed: 
> http://cr.openjdk.java.net/~psadhukhan/shilpi/webrev-7124238/closed-7124238/webrev/ 
>
>
>
> Thanks
> Shilpi
>
> On 8/24/2015 7:44 PM, Alexander Zvegintsev wrote:
>> Hi Shilpi,
>>
>>   56         test();
>>   57         f.dispose();
>>
>>
>> In case of test failure runtime exception will be thrown, thus 
>> f.dispose() will not be called.
>> So it would be better to call it from a finally block.
>>
>> The fix contains mixed tab/spaces indentation(we are using spaces), 
>> trailing whitespaces,
>> this will prevent push to openjdk repository.
>>
>> Also there are a lot of empty lines at the end of file.
>>
>> Thanks,
>>
>> Alexander.
>>
>> On 08/24/2015 02:47 PM, shilpi rastogi wrote:
>>> Hi all,
>>>
>>> Gentle reminder.
>>>
>>> Thanks
>>> Shilpi
>>> On 8/14/2015 1:23 PM, Alexander Scherbatiy wrote:
>>>>
>>>>   The fix looks good to me.
>>>>
>>>>   Thanks,
>>>>   Alexandr.
>>>>
>>>> On 8/14/2015 10:33 AM, shilpi rastogi wrote:
>>>>> Hi All,
>>>>>
>>>>> Thanks Alexander for comments!
>>>>>
>>>>> I tried to incorporate all changes suggested by you.
>>>>>
>>>>> Please have a look
>>>>>
>>>>> http://cr.openjdk.java.net/~sgupta/7124238/webrev.05/
>>>>> http://cr.openjdk.java.net/~sgupta/7124238/webrev.04/
>>>>>
>>>>>
>>>>> Thanks
>>>>> Shilpi
>>>>>
>>>>> On 8/13/2015 7:37 PM, Alexander Scherbatiy wrote:
>>>>>> On 8/13/2015 2:51 PM, shilpi rastogi wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>> Please review updated webrev
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~sgupta/7124238/webrev.03/
>>>>>>> http://cr.openjdk.java.net/~sgupta/7124238/webrev.02/
>>>>>>
>>>>>>   - I forgot to mention that the rule is to split line if it 
>>>>>> longer than 80 characters. Long lines  are annoying for 
>>>>>> side-by-side views.
>>>>>>    IDEs usually allow to configure right margin settings in a 
>>>>>> editor for this purposes.
>>>>>>   - It is better to define that createAndShowGUI() method throws 
>>>>>> Exception. In this case the try/catch block for L&F setting is 
>>>>>> unnecessary.
>>>>>>   - SwingUtilities.invokeAndWait() does not throw AWTException so 
>>>>>> its declaration can be removed.
>>>>>>
>>>>>>   Thanks,
>>>>>>   Alexandr.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Shilpi
>>>>>>>
>>>>>>>
>>>>>>> On 8/13/2015 12:56 PM, Alexander Scherbatiy wrote:
>>>>>>>> On 8/13/2015 8:16 AM, shilpi rastogi wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Please review a test bug fix
>>>>>>>>>
>>>>>>>>> TEST : 
>>>>>>>>> closed/javax/swing/plaf/basic/BasicHTML/4960629/bug4960629.java
>>>>>>>>> BUG ID - https://bugs.openjdk.java.net/browse/JDK-7124238
>>>>>>>>>
>>>>>>>>> Pleasemove  the test from closed repo to open repo.
>>>>>>>>>
>>>>>>>>> The webrev is: 
>>>>>>>>> http://cr.openjdk.java.net/~sgupta/7124238/webrev.01 add to 
>>>>>>>>> open repo
>>>>>>>>
>>>>>>>>     - The long lines should be split so they fit to a page
>>>>>>>>     - Exceptions should be re-thrown. In other case they are 
>>>>>>>> just printed but jtreg decides that a test is passed.
>>>>>>>>
>>>>>>>>   69                 try {
>>>>>>>>      70                     createAndShowGUI();
>>>>>>>>      71                 } catch (AWTException e) {
>>>>>>>>      72                     e.printStackTrace();
>>>>>>>>      73                 }
>>>>>>>>
>>>>>>>>    - The test methods can throw a general Exceptioninstead of 
>>>>>>>> several concrete ones. This usually makes a test more readable.
>>>>>>>>       It also allows to omit try/catch block for the the 
>>>>>>>> UIManager.setLookAndFeel() call.
>>>>>>>>
>>>>>>>>    Thanks,
>>>>>>>>    Alexandr.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~sgupta/7124238/webrev.00/ 
>>>>>>>>> <http://cr.openjdk.java.net/%7Ekshefov/8017187/webrev.diff/> - 
>>>>>>>>> diff with previous version of the  closed test.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Shilpi
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>