<Swing Dev> RFR: [12] JDK-8208638: Instead of circle rendered in appl window, but ellipse is produced JEditor Pane

[Sergey Bylokhov]

Looks fine.

On 10/09/2018 05:35, Prasanta Sadhukhan wrote:
> +1
> 
> Regards
> Prasanta
> On 9/10/2018 5:56 PM, Krishna Addepalli wrote:
>> Thanks for the comments Prasanta, here is the updated webrev: 
>> http://cr.openjdk.java.net/~kaddepalli/8208638/webrev02/ 
>> <http://cr.openjdk.java.net/%7Ekaddepalli/8208638/webrev02/>
>>
>> Krishna
>>
>>> On 10-Sep-2018, at 5:20 PM, Prasanta Sadhukhan 
>>> <prasanta.sadhukhan at oracle.com 
>>> <mailto:prasanta.sadhukhan at oracle.com>> 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<krishna.addepalli at oracle.com>;swing-dev at openjdk.java.net
>>>> *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:*swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>>>>     *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:*swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>>>>     *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
>>>>     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
>>>>
>>>
>>>
>>
> 


-- 
Best regards, Sergey.