Hi Vadim, Thanks for the review. I have made suggested changes. Updated Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/
Please review. Thanks, Jay -----Original Message----- From: Vadim Pakhnushev Sent: Friday, October 16, 2015 8:15 PM To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing Hi Jay, What's the point of using Float.compare in the test? Why not just check if (horizontalNodeValue.equals(expectedHorizontalValue)) ? Also please capitalize Attr in the declaration of horizontalattr and verticalattr. Thanks, Vadim On 16.10.2015 17:36, Jayathirth D V wrote: > Hello All, > > Can I get one more review please. > > Thanks, > Jay > > -----Original Message----- > From: Sergey Bylokhov > Sent: Thursday, October 15, 2015 6:05 PM > To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race > Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-7182758: > BMPMetadata returns invalid PhysicalPixelSpacing > > The fix looks fine. The test can be improved a little bit to simplify the > int->Integer boxing, but it is not necessary for now. > > Thanks. > > On 15.10.15 13:17, Jayathirth D V wrote: >> Hi Sergey, >> >> I thought you suggested to check for tighter "true" condition instead of >> "false" condition. >> >> I have made changes to map horizontalNodeValue and verticalNodeValue to >> expected values. Please find updated Webrev link: >> >> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/ >> >> Please review. >> >> Thanks, >> Jay >> >> -----Original Message----- >> From: Sergey Bylokhov >> Sent: Wednesday, October 14, 2015 7:34 PM >> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race >> Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-7182758: >> BMPMetadata returns invalid PhysicalPixelSpacing >> >> Hi, Jay. >>> I suggest to check correct/expected result in the test instead of previous >>> incorrect zero. >> Here, I suggested to check that the resulted horizontalNodeValue and >> verticalNodeValue are equal to some expected value. Because if it bigger >> than zero does not mean it is correct(note to use Float.compare(f1, f2) >> instead of "=="). >> >>> Previously I forgot to mention, that it will be good to name the test by >>> some useful name instead of some bug number(see examples in >>> javax/imageio/plugins). >>> >>> On 13.10.15 11:12, Jayathirth D V wrote: >>>> Hello All, >>>> >>>> Removed Trailing whitespace present in code. >>>> >>>> Please find updated webrev link: >>>> >>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/ >>>> >>>> Thanks, >>>> >>>> Jay >>>> >>>> *From:* Jayathirth D V >>>> *Sent:* Monday, October 12, 2015 2:15 PM >>>> *To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov >>>> *Subject:* [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for >>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing >>>> >>>> Hello All, >>>> >>>> Made small change on how we need to represent floating point >>>> constant in >>>> Java(1000.0 -> 1000.0F). >>>> >>>> Please find updated Webrev link: >>>> >>>> Webrev : >>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/ >>>> >>>> Please review. >>>> >>>> Thanks, >>>> >>>> Jay >>>> >>>> *From:* Jayathirth D V >>>> *Sent:* Thursday, October 08, 2015 2:20 PM >>>> *To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>; >>>> Philip Race; Sergey Bylokhov >>>> *Subject:* [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for >>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing >>>> >>>> Hello All, >>>> >>>> Please review following fix in jdk9: >>>> >>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7182758 >>>> >>>> Webrev : >>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/ >>>> >>>> Bug : BMPMetadata returns invalid PhysicalPixelSpacing >>>> >>>> Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more >>>> than value 1 in BMP header. Horizontal & Vertical Physical pixel >>>> spacing were returned as zero. >>>> >>>> In getStandardDimensionNode() method >>>> of BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter. >>>> When >>>> >>>> XPixelsPerMter/ YPixelsPerMter is >>>> more than 1. Resulted value is stored without decimal part, which resulted >>>> in zero. >>>> >>>> Solution : Made changes to how Horizontal & Vertical Physical pixel >>>> spacing is calculated so that decimal value is not truncated. >>>> >>>> Thanks, >>>> >>>> Jay >>>> >>> >>> -- >>> Best regards, Sergey. >>> >> >> -- >> Best regards, Sergey. >> > > -- > Best regards, Sergey.
