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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 134850: Patch
https://bugs.webkit.org/attachment.cgi?id=134850&action=review

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


review- because of what seems to be a bad cast

> Source/WebCore/ChangeLog:13
> +	    cachedOrPendingImageSet() now takes a Document as a parameter so
that it can 
> +	    access the deviceScaleFactor. If there is a cached image already
but its scale 
> +	    factor does not match the scale factor of the best fit image, then
m_imageSet is 
> +	    set to a pending image so that the best fit image will be loaded.

These comments seem to be indented one space too many.

> Source/WebCore/css/CSSImageSetValue.cpp:130
> +	   // If bestImageForScaleFactor() returns and image with a different
scale factor than the one we have loaded,

and image -> an image

> Source/WebCore/css/CSSImageSetValue.cpp:132
> +	   ImageWithScale bestImage =
bestImageForScaleFactor(deviceScaleFactor);

This seems like a lot of work to do every time we call cachedOrPendingImageSet.
Is there a way to avoid this work for the common case where the scale factor is
the same as last time?

> Source/WebCore/css/CSSImageSetValue.cpp:133
> +	   if (bestImage.scaleFactor !=
static_cast<StyleCachedImageSet*>(m_imageSet.get())->scaleFactorForBestFitImage
()) {

What guarantees that m_imageSet is a StyleCachedImageSet? Don’t we have to
check isCachedImageSet first? I think this is potentially a bad cast.

> Source/WebCore/rendering/style/StyleCachedImageSet.cpp:49
> +    m_bestFitImage->removeClient(this);

Is there a time other than destruction time that we can remove this as a
client? For reference counted objects it’s usually a bad pattern to clean up
the object only when the last reference goes away. It’s usually a better
pattern to have an explicit clear at some well-defined time.

> Source/WebCore/rendering/style/StyleCachedImageSet.h:43
> +    WTF_MAKE_FAST_ALLOCATED;

Why did you need to add this?