Thanks for the comments Prasanta, here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8208638/webrev02/ <http://cr.openjdk.java.net/~kaddepalli/8208638/webrev02/>
Krishna > On 10-Sep-2018, at 5:20 PM, Prasanta Sadhukhan > <[email protected]> wrote: > > yes, right. fix looks ok to me. Please change comment style in l946 and make > it similar to l956. Also, l960 change "ration" to "ratio" > > Regards > Prasanta > On 9/7/2018 9:00 PM, Krishna Addepalli wrote: >> Hi Prasanta, >> >> I think you are confused by the same name of the variables. >> newHeight/newWidth are in “updateImage” and also “ImageHandler::imageUpdate”. >> imageUpdate can be called from a background thread, with new information >> from the image being loaded, that contains the newHeight and newWidth. Along >> with this, the “flags” variable lets us know what properties of the image >> are updated. For example, ImageObserver.HEIHGT and ImageObserver.WIDTH flags >> being set indicates, that this update call has fetched the image properties >> from the file. >> Based on this, I cache these values, and then update them once the image >> load is complete. >> >> Thanks, >> Krishna >> >> From: Prasanta Sadhukhan >> Sent: Friday, September 7, 2018 7:04 PM >> To: Krishna Addepalli <[email protected]> >> <mailto:[email protected]>; [email protected] >> <mailto:[email protected]> >> Subject: Re: <Swing Dev> RFR: [12] JDK-8208638: Instead of circle rendered >> in appl window, but ellipse is produced JEditor Pane >> >> Hi Krishna, >> >> We obtain newHeight and specifiedHeight by same method >> >> newHeight = getIntAttr(HTML.Attribute.HEIGHT, -1); >> and >> >> specifiedHeight = getIntAttr(HTML.Attribute.HEIGHT, -1); >> so I assume both will return the same value. So, >> >> 968 if (specifiedHeight <= 0) { >> 969 proportion = specifiedWidth / >> ((double)newWidth); >> 970 newHeight = (int)(proportion * newHeight); >> 971 } >> if specifiedHeight is <=0 then newHeight * proportion will also be -ve or 0, >> isn't it? Will it not cause problem? Am I missing something? >> >> Regards >> Prasanta >> On 9/5/2018 4:25 PM, Krishna Addepalli wrote: >> Hi All, >> >> The testfix turned out to be simpler than I thought. I just had to query the >> preferred size before making the frame visible, so that the image is >> rendered on screen properly, and now Robot is able to pick the color. >> Here is the modified webrev: >> http://cr.openjdk.java.net/~kaddepalli/8208638/webrev01/ >> <http://cr.openjdk.java.net/%7Ekaddepalli/8208638/webrev01/> >> >> Thanks, >> Krishna >> >> From: Krishna Addepalli >> Sent: Wednesday, September 5, 2018 9:41 AM >> To: [email protected] <mailto:[email protected]> >> Subject: Re: <Swing Dev> RFR: [12] JDK-8208638: Instead of circle rendered >> in appl window, but ellipse is produced JEditor Pane >> >> Hi All, >> >> I found an issue with the test, and am working on it to make it pass >> reliably. I was setting the size of the frame and then querying the >> preferred size, which makes it a non-test. >> I’m working to address it, meanwhile you could still review the fix itself. >> >> Thanks, >> Krishna >> >> From: Krishna Addepalli >> Sent: Wednesday, September 5, 2018 12:10 AM >> To: [email protected] <mailto:[email protected]> >> Subject: RFR: [12] JDK-8208638: Instead of circle rendered in appl window, >> but ellipse is produced JEditor Pane >> >> Hi All, >> Please review a fix for JDK-8208638: >> https://bugs.openjdk.java.net/browse/JDK-8208638 >> <https://bugs.openjdk.java.net/browse/JDK-8208638> >> Webrev: http://cr.openjdk.java.net/~kaddepalli/8208638/webrev00/ >> <http://cr.openjdk.java.net/%7Ekaddepalli/8208638/webrev00/> >> >> JEditorPane supports loading HTML content, including images, and this bug is >> about image scaling when either of the width/height attributes are not >> specified in the HTML text. >> The problem is that scaling of the images in all cases was not handled >> properly. Here is the behavior that is observed in other browsers: >> 1. If both width and height are specified in html, then use them >> instead. >> 2. If both are not specified, then use the image dimensions. >> 3. If either one is specified, then proportionately adjust the other >> dimension to preserve the aspect ratio of the image. >> ImageView.java implementation was not handling the last case, which is added >> in the fix. >> >> The test is also fixed now, which was earlier relying on >> Robot.getPixelColor, which does not work reliably. A happy alternative was >> found, where in the “RootView” that is rendering the html content is >> retrieved from the JEditorPane, and its preferred span is queried, which >> should match the case specific width/height. >> Tested it on all platforms (Windows, Linux(Ubuntu) and Mac) and now it >> reliably passes. >> >> Thanks, >> Krishna >> > >
