Bug 67885 – Outline for the high-resolution broken image icon draws at 2x

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

https://bugs.webkit.org/show_bug.cgi?id=67819 added high resolution platform images to WebCore. The outline for the high-resolution broken image icon draws at 2x, but it should not.

[Beth Dakin]

<rdar://problem/10104637>

[Beth Dakin]

Created attachment 107223 [details]
Patch

[Beth Dakin]

Also: <rdar://problem/10104637>. I added that the the Changelog as well.

Comment on attachment 107223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107223&action=review

> Source/WebCore/rendering/RenderImage.cpp:92
> +        if (deviceScaleFactor >= 2)
> +            imageSize.scale(0.5f);        

It seems strange to always multiply by 1/2 rather than by 1/deviceScaleFactor. Is this relying on some knowledge of which sizes the broken image is available at?

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 107223 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107223&action=review
> 
> > Source/WebCore/rendering/RenderImage.cpp:92
> > +        if (deviceScaleFactor >= 2)
> > +            imageSize.scale(0.5f);        
> 
> It seems strange to always multiply by 1/2 rather than by 1/deviceScaleFactor. Is this relying on some knowledge of which sizes the broken image is available at?

This matter came up in https://bugs.webkit.org/show_bug.cgi?id=67819 

In that bug, Darin felt strongly that code similar to this in RenderLayer should use 0.5 instead of 1/deviceScaleFactor which was how I had coded it at one point. His argument was that currently we only have 2x images so (to quote him directly): "If the scale factor was 4 we would load the 2x image and then scale its size down by 4, so so display a half-sized resized. Not good! It would just get worse at higher resolutions."

Comment on attachment 107223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107223&action=review

>>> Source/WebCore/rendering/RenderImage.cpp:92
>>> +            imageSize.scale(0.5f);        
>> 
>> It seems strange to always multiply by 1/2 rather than by 1/deviceScaleFactor. Is this relying on some knowledge of which sizes the broken image is available at?
> 
> This matter came up in https://bugs.webkit.org/show_bug.cgi?id=67819 
> 
> In that bug, Darin felt strongly that code similar to this in RenderLayer should use 0.5 instead of 1/deviceScaleFactor which was how I had coded it at one point. His argument was that currently we only have 2x images so (to quote him directly): "If the scale factor was 4 we would load the 2x image and then scale its size down by 4, so so display a half-sized resized. Not good! It would just get worse at higher resolutions."

I see. Dividing by the scale factor would be incorrect, and doing this is correct, but the reason why it’s correct is nowhere near this code. A comment here could explain why this is the right thing to do, but still won’t help us remember to change this when we add more versions of the image.

A good way to have the code do the right thing would be to have a way to ask an image for its resolution (or scale factor), but that is outside the scope of this bug.

[Beth Dakin]

(In reply to comment #6)
> 
> I see. Dividing by the scale factor would be incorrect, and doing this is correct, but the reason why it’s correct is nowhere near this code. A comment here could explain why this is the right thing to do, but still won’t help us remember to change this when we add more versions of the image.

Thanks for the review.

I agree this code is problematic in that it will be tricky to find in the future, unlike the RenderLayer code which is just a few lines below the call that actually loads the image. I will definitely add a comment here. Perhaps we should make 0.5 a global constant somewhere? Called something along the lines of highResolutionArtworkScaleFactor? When we have more then one scale factor for high-resolution artwork, the global constant will be insufficient, but it would at least make all instances more find-able? What do you think?

[Beth Dakin]

(In reply to comment #7)
> A good way to have the code do the right thing would be to have a way to ask an image for its resolution (or scale factor), but that is outside the scope of this bug.

Oh that's a good idea too. (I didn't see this comment before I left my last comment.)

[Beth Dakin]

Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051.

[Beth Dakin]

(In reply to comment #10)
> Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051.

Doh! And in revision 95053 I added a comment. Feeling spacey today.

[Darin Adler]

(In reply to comment #7)
> A good way to have the code do the right thing would be to have a way to ask an image for its resolution (or scale factor), but that is outside the scope of this bug.

We don’t even have to go that far. Just returning a scale factor along with the image from the brokenImage function would do for this case.