[webkit-reviews] review denied: [Bug 67819] Use high resolution platform images when the deviceScaleFactor > 1 : [Attachment 106818] Patch with style fix

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 67819: Use high resolution platform images when the deviceScaleFactor > 1
https://bugs.webkit.org/show_bug.cgi?id=67819

Attachment 106818: Patch with style fix
https://bugs.webkit.org/attachment.cgi?id=106818&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106818&action=review


Do we have code that will cause us to update to the right image when the page's
device scale factor changes?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-575
> -		1C14E76B0AD8C81C00B6158B /* deleteButtonPressed.tiff in
Resources */ = {isa = PBXBuildFile; fileRef = 1C14E7690AD8C81C00B6158B /*
deleteButtonPressed.tiff */; };
> -		1C14E76C0AD8C81C00B6158B /* deleteButton.tiff in Resources */ =
{isa = PBXBuildFile; fileRef = 1C14E76A0AD8C81C00B6158B /* deleteButton.tiff
*/; };

Are these files still used by other ports? Or can we delete them?

> Source/WebCore/editing/DeleteButtonController.cpp:244
> +    unsigned deviceScaleFactor = 1;

Since we use float everywhere else, I'd recommend using it here, too.

> Source/WebCore/editing/DeleteButtonController.cpp:252
> +    RefPtr<Image> buttonImage;
> +    if (deviceScaleFactor == 1)
> +	   buttonImage = Image::loadPlatformResource("deleteButton");
> +    else
> +	   buttonImage = Image::loadPlatformResource("deleteButton at 2x");

I think our normal practice has been to use @2x images when the scale factor is
>= 2. So I'd write it like this:

if (deviceScaleFactor >= 2)
    use @2x
else
    use normal image

> Source/WebCore/loader/cache/CachedImage.cpp:117
> +static Image* brokenImage(unsigned deviceScaleFactor)

Again, I think you should use float here.

> Source/WebCore/loader/cache/CachedImage.cpp:122
> +    // If deviceScaleFactor is 0, that means we couldn't access the Page
through m_request this time,
> +    // so use the lastDeviceScaleFactor to determine the appropriate
resolution.

In what kinds of cases will we be unable to get to a Page?

I think you can make the logic in this function simpler by doing this:

static float lastDeviceScaleFactor = 1;

if (!deviceScaleFactor)
    deviceScaleFactor = lastDeviceScaleFactor;
else
    lastDeviceScaleFactor = deviceScaleFactor;

// From here on you just use deviceScaleFactor and never mention
lastDeviceScaleFactor again.

> Source/WebCore/loader/cache/CachedImage.cpp:123
> +    if (deviceScaleFactor == 1 || (!deviceScaleFactor &&
lastDeviceScaleFactor == 1)) {

Again, I think you should use the >= 2 logic as above.

> Source/WebCore/loader/cache/CachedImage.cpp:125
> +	   DEFINE_STATIC_LOCAL(RefPtr<Image>, brokenImageLoRes,
(Image::loadPlatformResource("missingImage")));

You can make this a bare pointer if you add a .leakRef() to the
loadPlatformResource expression.

> Source/WebCore/rendering/RenderLayer.cpp:2458
> +    if (deviceScaleFactor == 1) {

Again, I recommend you use the >= 2 logic as above.

>>> Source/WebCore/rendering/RenderLayer.cpp:2467
>>> +	     cornerResizerHeight = resizeCornerImageHiRes->height() / 2;
>> 
>> Isn't this assuming that a non-1 deviceScaleFactor is always 2?
> 
> Ah, yes that's dumb. I can just divide by the devicePixelRatio and that will
actually be the correct thing. I will post a new patch that does that and
attempts to fix the broken builds shortly.

If we were using IntSize instead of two ints you could even use
cornerResizerSize.scale(1 / devicePixelRatio).