Bug 67885 - Outline for the high-resolution broken image icon draws at 2x
: Outline for the high-resolution broken image icon draws at 2x
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2011-09-09 23:35 PST by
Modified: 2011-09-13 16:17 PST (History)


Attachments
Patch (2.12 KB, patch)
2011-09-13 14:09 PST, Beth Dakin
mitz: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-09 23:35:00 PST
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.
------- Comment #1 From 2011-09-09 23:36:43 PST -------
<rdar://problem/10104637>
------- Comment #2 From 2011-09-13 14:09:19 PST -------
Created an attachment (id=107223) [details]
Patch
------- Comment #3 From 2011-09-13 14:21:40 PST -------
Also: <rdar://problem/10104637>. I added that the the Changelog as well.
------- Comment #4 From 2011-09-13 14:31:41 PST -------
(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?
------- Comment #5 From 2011-09-13 14:37:04 PST -------
(In reply to comment #4)
> (From update of attachment 107223 [details] [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 #6 From 2011-09-13 14:44:55 PST -------
(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
>>> +            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.
------- Comment #7 From 2011-09-13 14:46:18 PST -------
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.
------- Comment #8 From 2011-09-13 14:49:05 PST -------
(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?
------- Comment #9 From 2011-09-13 14:50:35 PST -------
(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.)
------- Comment #10 From 2011-09-13 15:33:56 PST -------
Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051.
------- Comment #11 From 2011-09-13 15:40:59 PST -------
(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.
------- Comment #12 From 2011-09-13 16:17:14 PST -------
(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.