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.
<rdar://problem/10104637>
Created attachment 107223 [details] Patch
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?
(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.
(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?
(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.)
Committed this change with revision 95048…and then fixed up the ChangeLog mishap with revision 95051.
(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.
(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.