-webkit-image-set should update dynamically when the device scale factor changes

• Component: CSS
• Status: RESOLVED FIXED

[Beth Dakin]

https://bugs.webkit.org/show_bug.cgi?id=80322 added a new CSS feature to WebKit called -webkit-image-set. The idea behind the feature is to allow authors to provide multiple variants of the same image at differing resolutions, and to allow the User Agent to choose the resource that is most appropriate at the time. This will allow authors to specify high resolution resources for iPhone's retina display, and it will allow User Agents to choose different resolution resources when content is scaled or zoomed. 

As it's currently implemented, WebKit will only consider the device scale factor when choosing an image. -webkit-image-set should re-access the resource options if the device scale factor changes.

[Radar WebKit Bug Importer]

<rdar://problem/11101108>

[Beth Dakin]

Created attachment 134850 [details]
Patch

[Darin Adler]

Comment on attachment 134850 [details]
Patch

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?

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 134850 [details])
> 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.
> 

Fixed.

> > 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
> 

Fixed.

> > 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?
> 

We could cache the device scale factor in CSSImageSetValue instead. I will change to do that.

> > 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.
> 

m_imageSet will always be either a StyleCachedImageSet or a StylePendingImage. It does seem possible to get to this code and have a StylePendingImage instead, so I will definitely add a check.

> > 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.
> 

This is how StyleCachedImage does it as well. It's not immediately obvious what the well-defined time would be to call an explicit clear.

> > Source/WebCore/rendering/style/StyleCachedImageSet.h:43
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> Why did you need to add this?

This does not need to be added. In my mind, part of this patch was to make StyleCachedImageSet more like StyleCachedImage, and the only part of that that is really required is adding CachedImageClient as a base class and the associated add/remove client bits. This is just something I happened to notice when I was in StyleCachedImage.h. So I thought, if StyleCachedImage is fact allocated, then StyleCachedImageSet should be too! But perhaps that is not enough of a reason.

[Beth Dakin]

Created attachment 135187 [details]
Patch v2

Here's a patch that addresses most of Darin's comments. I think that the only outstanding concern is whether WTF_MAKE_FAST_ALLOCATED is desirable or not. I'm still looking into that.

[Beth Dakin]

(In reply to comment #5)
> Created an attachment (id=135187) [details]
> Patch v2
> 
> Here's a patch that addresses most of Darin's comments. I think that the only outstanding concern is whether WTF_MAKE_FAST_ALLOCATED is desirable or not. I'm still looking into that.

I should mention that Darin and I discussed this in person, and we think this part of the patch is fine as-is for now.

[Darin Adler]

Comment on attachment 135187 [details]
Patch v2

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.

[Beth Dakin]

Thanks, Darin! I finessed some of these things before checking in:

http://trac.webkit.org/changeset/113490