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
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Beth Dakin
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-09 23:35 PDT by Beth Dakin
Modified: 2011-09-13 16:17 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-09-09 23:35:00 PDT
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 Beth Dakin 2011-09-09 23:36:43 PDT
<rdar://problem/10104637>
Comment 2 Beth Dakin 2011-09-13 14:09:19 PDT
Created attachment 107223 [details]
Patch
Comment 3 Beth Dakin 2011-09-13 14:21:40 PDT
Also: <rdar://problem/10104637>. I added that the the Changelog as well.
Comment 4 mitz@webkit.org 2011-09-13 14:31:41 PDT
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?
Comment 5 Beth Dakin 2011-09-13 14:37:04 PDT
(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 6 mitz@webkit.org 2011-09-13 14:44:55 PDT
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.
Comment 7 mitz@webkit.org 2011-09-13 14:46:18 PDT
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 Beth Dakin 2011-09-13 14:49:05 PDT
(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 Beth Dakin 2011-09-13 14:50:35 PDT
(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 Beth Dakin 2011-09-13 15:33:56 PDT
Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051.
Comment 11 Beth Dakin 2011-09-13 15:40:59 PDT
(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 Darin Adler 2011-09-13 16:17:14 PDT
(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.