[webkit-reviews] review granted: [Bug 81939] -webkit-image-set should update dynamically when the device scale factor changes : [Attachment 135187] Patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 81939: -webkit-image-set should update dynamically when the device scale
factor changes
https://bugs.webkit.org/show_bug.cgi?id=81939

Attachment 135187: Patch v2
https://bugs.webkit.org/attachment.cgi?id=135187&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=135187&action=review


This patch is OK. CSSImageSetValue seems to have an unusual separation of
responsibilities between its various functions that could be improved.

> Source/WebCore/css/CSSImageSetValue.cpp:94
> -    float deviceScaleFactor = 1;
>      if (Page* page = document->page())
> -	   deviceScaleFactor = page->deviceScaleFactor();
> +	   m_scaleFactor = page->deviceScaleFactor();

What guarantees m_scaleFactor will be 1 when we enter this function? Might it
have a leftover scale factor from earlier? Shouldn’t we set it to 1 explicitly
if page is 0? I’d prefer to see us use a helper function to return the scale
factor given a document that consistently returns a 1.

> Source/WebCore/css/CSSImageSetValue.cpp:102
> +    return cachedImageSet(loader, bestImageForScaleFactor(m_scaleFactor));

This code seems a bit bizarre. If we m_accessedBestFitImage is already true,
then we do the work of calling bestImageForScaleFactor even though the image
argument to cachedImageSet won’t even be used.

It is not good style that the m_scaleFactor setting is done at this level, but
the m_imageSet creation is done down a level from here.

> Source/WebCore/css/CSSImageSetValue.h:76
> +    // This represents the scale factor that we use to find the best fit
image. It does not necessarily

I would say “that we used” rather than “that we use” here.