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.
<rdar://problem/11101108>
Created attachment 134850 [details] Patch
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?
(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.
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.
(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.
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.
Thanks, Darin! I finessed some of these things before checking in: http://trac.webkit.org/changeset/113490