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

[shilpi rastogi]

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