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