Bug 80322 – Implement image-set

• Component: CSS
• Status: RESOLVED FIXED

[Beth Dakin]

Ted O'Connor recently proposed a new CSS function for the Images module to the CSS working group called image-set: 

http://lists.w3.org/Archives/Public/www-style/2012Feb/1103.html

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. 

This bug tracks implementing an initial version of this feature.

<rdar://problem/10189591>

[Dean Jackson]

You say on webkit-dev that this wouldn't be guarded by an ENABLE flag. Considering it exposes in-progress API to the Web, I wonder if it should be guarded. A port could ship with an unfinished implementation.

[Beth Dakin]

Created attachment 131687 [details]
Patch: image-set for the content property

Here's an initial implementation. This version only hooks up image-set for the content property. I decided to add the ifdef after all. Platforms will have to opt in. If this gets reviewed and checked in, I will be sure to reply to webkit-dev to make sure other ports know that I added the ifdef after all. This implementation leaves the following important follow-up bugs:

-Hook up -webkit-image-set for all other CSS properties that take images (background, border, etc)
-image-set should update dynamically when the device scale factor changes
-image-set should kick in for all other forms of scaling instead of just device scale (page scale, CSS transforms, page zoom)

[WebKit Review Bot]

Attachment 131687 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSStyleSelector.cpp:4182:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 131687 [details]
Patch: image-set for the content property

View in context: https://bugs.webkit.org/attachment.cgi?id=131687&action=review

> Source/WebCore/css/CSSImageSetValue.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

This version of the license is not current.

[Beth Dakin]

Created attachment 132144 [details]
Patch with proper license (i think)

[Beth Dakin]

Created attachment 132728 [details]
Patch that works for all image properties

Here's my latest version of the patch. This hooks up image-set for background, borer-image, and masks as well as the original hook-up for content. There are still a couple of follow-up bugs:

-image-set should kick in dynamically when the device scale factor changes
-image-set should kick in for all other forms of scaling instead of just device scale (page scale, CSS transform, page zoom)
-We need real hi res and low res artwork that would actually be used in the border-image and mask image case to confirm that everything is actually happening properly.

[Beth Dakin]

Created attachment 132729 [details]
Marked a s a patch this time

Oops, forgot to mark that last one as a patch.

[WebKit Review Bot]

Attachment 132729 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSStyleSelector.cpp:4169:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Comment on attachment 132729 [details]
Marked a s a patch this time

View in context: https://bugs.webkit.org/attachment.cgi?id=132729&action=review

> Source/WebCore/css/CSSParser.cpp:6612
> +// FIXME: At this time, this is only hooked up for the content property, but in the future

Oops! This comment is no longer accurate. Removed.

[Early Warning System Bot]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11999025

[Build Bot]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11995025

[Early Warning System Bot]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11994055

[Early Warning System Bot]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12005051

[Philippe Normand]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11994087

[WebKit Review Bot]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12001056

[Gyuyoung Kim]

Comment on attachment 132729 [details]
Marked a s a patch this time

Attachment 132729 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12001130

[Beth Dakin]

Created attachment 132851 [details]
Patch that will hopefully build

[Beth Dakin]

Created attachment 132880 [details]
Patch that will hopefully apply and build

[WebKit Review Bot]

Attachment 132880 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSStyleSelector.cpp:4171:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 132880 [details]
Patch that will hopefully apply and build

Attachment 132880 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12044009

[Beth Dakin]

Created attachment 132889 [details]
Another go for the bots

[Dean Jackson]

Comment on attachment 132851 [details]
Patch that will hopefully build

View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review

I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage.

I also suggest you have some parsing tests, especially for broken input (a set without urls, or multiple scales, or missing commas, etc). e.g. LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js and LayoutTests/css3/filters/script-tests/filter-property-parsing.js

These were really helpful in finding errors in the input processing.

> Source/JavaScriptCore/ChangeLog:8
> +        For the time being, image-set it opt-in since the implementation is 

typo "it"

Also, I never know when you should use Platform.h vs a flag in build-webkit. Platform is definitely easier.

> Source/WebCore/ChangeLog:4
> +        Implement image-set

Maybe this should link to the proposal on www-style? And describe what it is.

> Source/WebCore/ChangeLog:13
> +        CSSImageSetValue inherits from CSSValueList and behaves a lot like 
> +        CSSImageValue.
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * css/CSSImageSetValue.h: Added.

I assume CSSImageSetValue is never seen outside WebCore (i.e. can I get one in JavaScript)?

> Source/WebCore/css/CSSImageSetValue.cpp:86
> +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
> +{
> +    ImageWithScale image;
> +    size_t numberOfImages = m_imagesInSet.size();
> +    for (size_t i = 0; i < numberOfImages; ++i) {
> +        image = m_imagesInSet.at(i);
> +        if (image.scaleFactor >= scaleFactor)
> +            return image;
> +    }
> +    return image;
> +}

What happens if the requested scaleFactor is larger than all the available image scaleFactors?

> Source/WebCore/css/CSSImageSetValue.cpp:100
> +    // FIXME: In the future, we want to take much more than deviceScaleFactor into acount here. 
> +    // All forms of scale should be included: Page::pageScaleFactor(), Frame::pageZoomFactor(), 
> +    // and any CSS transforms.

Link to bug?

> Source/WebCore/css/CSSImageSetValue.cpp:125
> +StyleCachedImageSet* CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image)
> +{
> +    ASSERT(loader);
> +
> +    if (!m_accessedBestFitImage) {
> +        ResourceRequest request(loader->document()->completeURL(image.imageURL));
> +        if (CachedImage* cachedImage = loader->requestImage(request)) {
> +            m_imageSet = StyleCachedImageSet::create(cachedImage, image.scaleFactor, this);
> +            m_accessedBestFitImage = true;
> +        }
> +    }
> +
> +    return (m_imageSet && m_imageSet->isCachedImageSet()) ? static_cast<StyleCachedImageSet*>(m_imageSet.get()) : 0;
> +}
> +
> +StyleImage* CSSImageSetValue::cachedOrPendingImageSet()
> +{
> +    if (!m_imageSet)
> +        m_imageSet = StylePendingImage::create(this);
> +
> +    return m_imageSet.get();
> +}

This is a dumb question, but I don't understand when you need the OrPending version. I was originally wondering if you could simply test for m_imageSet being not null rather than using m_accessedBestFitImage, but then I saw that m_imageSet is also set in the latter function. Maybe this is used later, in which case I forgot to delete this note :) OK! I found it below. I don't have a strong opinion, but I wonder if you can just have the top function here. It would return null in the case of no m_imageSet and you could test for that in the setOrPendingFromValue function. But maybe I haven't got all the logic right!

> Source/WebCore/css/CSSImageSetValue.h:50
> +    StyleCachedImageSet* cachedImageSet(CachedResourceLoader*);
> +    // Returns a StyleCachedImageSet if the best fit image has been cached already, otherwise a StylePendingImage.
> +    StyleImage* cachedOrPendingImageSet();

The comment applies to the top function right? Maybe it should go before or on the same line?

> Source/WebCore/css/CSSParser.cpp:2944
> +#if ENABLE(CSS_IMAGE_SET)
> +              else if (equalIgnoringCase(val->function->name, "-webkit-image-set(")) {
> +                parsedValue = parseImageSet(m_valueList.get());
> +                if (!parsedValue)
> +                    return false;
> +            }
> +#endif
> +              else if (isGeneratedImageValue(val)) {

Indentation on last line.

> Source/WebCore/css/CSSParser.cpp:5577
> +                else
> +                    return false;
> +            }
> +#endif
> +             else if (val->id == CSSValueNone)

Indentation on last line.

> Source/WebCore/css/CSSParser.cpp:6624
> +    if (!functionArgs || !functionArgs->size())
> +        return 0;
> +
> +    if (!functionArgs->current())
> +        return 0;

Merge?

> Source/WebCore/css/CSSParser.cpp:6644
> +        double imageScaleFactor = 0;
> +        const String& string = arg->string;
> +        const UChar* current = string.characters();
> +        const UChar* end = current + string.length();
> +        parseDouble(current, end, 'x', imageScaleFactor);
> +        imageSet->append(cssValuePool()->createValue(imageScaleFactor, CSSPrimitiveValue::CSS_NUMBER));

Should we check for > 0?

> Source/WebCore/css/CSSStyleSelector.cpp:3056
> +#if ENABLE(CSS_IMAGE_SET)
> +              else if (item->isImageSetValue()) {
> +                m_style->setContent(setOrPendingFromValue(CSSPropertyContent, static_cast<CSSImageSetValue*>(item)), didSet);
> +                didSet = true;
> +            }
> +#endif

Indentation on else if line.

> Source/WebCore/platform/graphics/Image.cpp:173
> -void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio)
> +void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio, float scaleFactor)
>  {
>      intrinsicRatio = size();
> +    intrinsicRatio.scale(1 / scaleFactor);

I think we can pass in a scaleFactor of 0 here, unless we've stopped it in the parsing step.

> Source/WebCore/rendering/style/StyleCachedImageSet.cpp:70
> +IntSize StyleCachedImageSet::imageSize(const RenderObject* renderer, float multiplier) const
> +{
> +    IntSize scaledImageSize = m_bestFitImage->imageSizeForRenderer(renderer, multiplier);
> +    scaledImageSize.scale(1 / m_imageScaleFactor);
> +    return scaledImageSize;
> +}

Since we don't test for 0 scales in the parsing method, the image.scaleFactor that's used at creation could be 0, which means we could be dividing by 0 here.

> LayoutTests/fast/hidpi/image-set-as-background.html:11
> +        if (!window.layoutTestController)
> +            return;
> +
> +
> +        if (!window.sessionStorage)
> +            return;
> +

Merge?

[WebKit Review Bot]

Attachment 132889 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSStyleSelector.cpp:4171:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

(In reply to comment #22)
> (From update of attachment 132851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review
> 
> I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage.
> 

Lalalalalalalalalala I can't hear yooooou!!!!!!

(I might consider it if I must for the sake of review though.)

> I also suggest you have some parsing tests, especially for broken input (a set without urls, or multiple scales, or missing commas, etc). e.g. LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js and LayoutTests/css3/filters/script-tests/filter-property-parsing.js
> 
> These were really helpful in finding errors in the input processing.
> 

I will take a look and add one.

> > Source/JavaScriptCore/ChangeLog:8
> > +        For the time being, image-set it opt-in since the implementation is 
> 
> typo "it"
> 

Fixed.

> Also, I never know when you should use Platform.h vs a flag in build-webkit. Platform is definitely easier.
> 

I don't know either, but I do generally favor easy things unless there is a compelling reason to do it the more complicated way.

> > Source/WebCore/ChangeLog:4
> > +        Implement image-set
> 
> Maybe this should link to the proposal on www-style? And describe what it is.
> 

Done.

> > Source/WebCore/ChangeLog:13
> > +        CSSImageSetValue inherits from CSSValueList and behaves a lot like 
> > +        CSSImageValue.
> > +        * WebCore.xcodeproj/project.pbxproj:
> > +        * css/CSSImageSetValue.h: Added.
> 
> I assume CSSImageSetValue is never seen outside WebCore (i.e. can I get one in JavaScript)?
> 

You cannot.

> > Source/WebCore/css/CSSImageSetValue.cpp:86
> > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
> > +{
> > +    ImageWithScale image;
> > +    size_t numberOfImages = m_imagesInSet.size();
> > +    for (size_t i = 0; i < numberOfImages; ++i) {
> > +        image = m_imagesInSet.at(i);
> > +        if (image.scaleFactor >= scaleFactor)
> > +            return image;
> > +    }
> > +    return image;
> > +}
> 
> What happens if the requested scaleFactor is larger than all the available image scaleFactors?
> 

You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation.

> > Source/WebCore/css/CSSImageSetValue.cpp:100
> > +    // FIXME: In the future, we want to take much more than deviceScaleFactor into acount here. 
> > +    // All forms of scale should be included: Page::pageScaleFactor(), Frame::pageZoomFactor(), 
> > +    // and any CSS transforms.
> 
> Link to bug?
> 

Added.

> > Source/WebCore/css/CSSImageSetValue.cpp:125
> > +StyleCachedImageSet* CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image)
> > +{
> > +    ASSERT(loader);
> > +
> > +    if (!m_accessedBestFitImage) {
> > +        ResourceRequest request(loader->document()->completeURL(image.imageURL));
> > +        if (CachedImage* cachedImage = loader->requestImage(request)) {
> > +            m_imageSet = StyleCachedImageSet::create(cachedImage, image.scaleFactor, this);
> > +            m_accessedBestFitImage = true;
> > +        }
> > +    }
> > +
> > +    return (m_imageSet && m_imageSet->isCachedImageSet()) ? static_cast<StyleCachedImageSet*>(m_imageSet.get()) : 0;
> > +}
> > +
> > +StyleImage* CSSImageSetValue::cachedOrPendingImageSet()
> > +{
> > +    if (!m_imageSet)
> > +        m_imageSet = StylePendingImage::create(this);
> > +
> > +    return m_imageSet.get();
> > +}
> 
> This is a dumb question, but I don't understand when you need the OrPending version. I was originally wondering if you could simply test for m_imageSet being not null rather than using m_accessedBestFitImage, but then I saw that m_imageSet is also set in the latter function. Maybe this is used later, in which case I forgot to delete this note :) OK! I found it below. I don't have a strong opinion, but I wonder if you can just have the top function here. It would return null in the case of no m_imageSet and you could test for that in the setOrPendingFromValue function. But maybe I haven't got all the logic right!
> 

Definitely not dumb! I was combing through a lot of this when writing the patch, and I just mimicked what the other image classes do without fully understanding why they do what they do. I think it's good to have all of the image types match each other and return a pending image when they are not yet loaded, but at the same time, it would be good to understand why the existing classes do it that way. If there is not a compelling reason, we could consider changing them all at once.

> > Source/WebCore/css/CSSImageSetValue.h:50
> > +    StyleCachedImageSet* cachedImageSet(CachedResourceLoader*);
> > +    // Returns a StyleCachedImageSet if the best fit image has been cached already, otherwise a StylePendingImage.
> > +    StyleImage* cachedOrPendingImageSet();
> 
> The comment applies to the top function right? Maybe it should go before or on the same line?
> 

Actually it applies to the bottom function. I added a space to make that more clear.

> > Source/WebCore/css/CSSParser.cpp:2944
> > +#if ENABLE(CSS_IMAGE_SET)
> > +              else if (equalIgnoringCase(val->function->name, "-webkit-image-set(")) {
> > +                parsedValue = parseImageSet(m_valueList.get());
> > +                if (!parsedValue)
> > +                    return false;
> > +            }
> > +#endif
> > +              else if (isGeneratedImageValue(val)) {
> 
> Indentation on last line.
> 

Fixed.

> > Source/WebCore/css/CSSParser.cpp:5577
> > +                else
> > +                    return false;
> > +            }
> > +#endif
> > +             else if (val->id == CSSValueNone)
> 
> Indentation on last line.
> 

Fixed.

> > Source/WebCore/css/CSSParser.cpp:6624
> > +    if (!functionArgs || !functionArgs->size())
> > +        return 0;
> > +
> > +    if (!functionArgs->current())
> > +        return 0;
> 
> Merge?
> 

Done.

> > Source/WebCore/css/CSSParser.cpp:6644
> > +        double imageScaleFactor = 0;
> > +        const String& string = arg->string;
> > +        const UChar* current = string.characters();
> > +        const UChar* end = current + string.length();
> > +        parseDouble(current, end, 'x', imageScaleFactor);
> > +        imageSet->append(cssValuePool()->createValue(imageScaleFactor, CSSPrimitiveValue::CSS_NUMBER));
> 
> Should we check for > 0?
> 

I think so. Added.

> > Source/WebCore/css/CSSStyleSelector.cpp:3056
> > +#if ENABLE(CSS_IMAGE_SET)
> > +              else if (item->isImageSetValue()) {
> > +                m_style->setContent(setOrPendingFromValue(CSSPropertyContent, static_cast<CSSImageSetValue*>(item)), didSet);
> > +                didSet = true;
> > +            }
> > +#endif
> 
> Indentation on else if line.
> 

Fixed.

> > Source/WebCore/platform/graphics/Image.cpp:173
> > -void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio)
> > +void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsicHeight, FloatSize& intrinsicRatio, float scaleFactor)
> >  {
> >      intrinsicRatio = size();
> > +    intrinsicRatio.scale(1 / scaleFactor);
> 
> I think we can pass in a scaleFactor of 0 here, unless we've stopped it in the parsing step.
> 

Fixed in parser.

> > Source/WebCore/rendering/style/StyleCachedImageSet.cpp:70
> > +IntSize StyleCachedImageSet::imageSize(const RenderObject* renderer, float multiplier) const
> > +{
> > +    IntSize scaledImageSize = m_bestFitImage->imageSizeForRenderer(renderer, multiplier);
> > +    scaledImageSize.scale(1 / m_imageScaleFactor);
> > +    return scaledImageSize;
> > +}
> 
> Since we don't test for 0 scales in the parsing method, the image.scaleFactor that's used at creation could be 0, which means we could be dividing by 0 here.
> 

Fixed in parser.

> > LayoutTests/fast/hidpi/image-set-as-background.html:11
> > +        if (!window.layoutTestController)
> > +            return;
> > +
> > +
> > +        if (!window.sessionStorage)
> > +            return;
> > +
> 
> Merge?

Fixed.

I will post a new patch once I make a parsing test.

[Dean Jackson]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 132851 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review
> > 
> > I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage.
> > 
> 
> Lalalalalalalalalala I can't hear yooooou!!!!!!
> 
> (I might consider it if I must for the sake of review though.)

I actually wrote that part way through, but once I got to the end I wasn't too zapped. It's not a huge patch.


> 
> > > Source/WebCore/css/CSSImageSetValue.cpp:86
> > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
> > > +{
> > > +    ImageWithScale image;
> > > +    size_t numberOfImages = m_imagesInSet.size();
> > > +    for (size_t i = 0; i < numberOfImages; ++i) {
> > > +        image = m_imagesInSet.at(i);
> > > +        if (image.scaleFactor >= scaleFactor)
> > > +            return image;
> > > +    }
> > > +    return image;
> > > +}
> > 
> > What happens if the requested scaleFactor is larger than all the available image scaleFactors?
> > 
> 
> You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation.

So, won't that mean in this case you'd get an empty ImageWithScale here? Which would then be passed to
CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image)
and there would be no image to load.

Sorry if I'm wrong. I didn't follow through everything carefully.


> > > LayoutTests/fast/hidpi/image-set-as-background.html:11
> > > +        if (!window.layoutTestController)
> > > +            return;
> > > +
> > > +
> > > +        if (!window.sessionStorage)
> > > +            return;
> > > +
> > 
> > Merge?
> 
> Fixed.

I forgot to say that this was in every test.

[Beth Dakin]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > (From update of attachment 132851 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=132851&action=review
> > > 
> > > I wonder if you could split this up into two patches: one that does the parsing of the new syntax (and collects the info) and another that does the rendering side. Or three, if you want to separate the changes to Cached, Style and GeneratedImage.
> > > 
> > 
> > Lalalalalalalalalala I can't hear yooooou!!!!!!
> > 
> > (I might consider it if I must for the sake of review though.)
> 
> I actually wrote that part way through, but once I got to the end I wasn't too zapped. It's not a huge patch.
> 

Hehehe, thanks! I certainly don't want to be disagreeable, so I really would consider splitting it, but of course it's easier not to.

> 
> > 
> > > > Source/WebCore/css/CSSImageSetValue.cpp:86
> > > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
> > > > +{
> > > > +    ImageWithScale image;
> > > > +    size_t numberOfImages = m_imagesInSet.size();
> > > > +    for (size_t i = 0; i < numberOfImages; ++i) {
> > > > +        image = m_imagesInSet.at(i);
> > > > +        if (image.scaleFactor >= scaleFactor)
> > > > +            return image;
> > > > +    }
> > > > +    return image;
> > > > +}
> > > 
> > > What happens if the requested scaleFactor is larger than all the available image scaleFactors?
> > > 
> > 
> > You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation.
> 
> So, won't that mean in this case you'd get an empty ImageWithScale here? Which would then be passed to
> CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image)
> and there would be no image to load.
> 
> Sorry if I'm wrong. I didn't follow through everything carefully.
> 

I don't think you'll get an empty ImageWithScale. The function looks like this:

CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
{
    ImageWithScale image;
    size_t numberOfImages = m_imagesInSet.size();
    for (size_t i = 0; i < numberOfImages; ++i) {
        image = m_imagesInSet.at(i);
        if (image.scaleFactor >= scaleFactor)
            return image;
    }
    return image;
}

So imagine an input parameter of scaleFactor = 3. Also imagine m_imagesInSet has only one image in it with a scale of 1. We'll enter the loop for only one iteration, and our temporary variable will be set to the image with a scale factor of 1. We'll fail the conditional check, but then we will return that image anyway after the loop. 

> 
> > > > LayoutTests/fast/hidpi/image-set-as-background.html:11
> > > > +        if (!window.layoutTestController)
> > > > +            return;
> > > > +
> > > > +
> > > > +        if (!window.sessionStorage)
> > > > +            return;
> > > > +
> > > 
> > > Merge?
> > 
> > Fixed.
> 
> I forgot to say that this was in every test.

Caught that, thanks!

Still working on that parsing test. Hopefully I'll have it soon.

[Beth Dakin]

Created attachment 133140 [details]
Patch with parsing tests

[Dean Jackson]

(In reply to comment #26)
> > > > > Source/WebCore/css/CSSImageSetValue.cpp:86
> > > > > +CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
> > > > > +{
> > > > > +    ImageWithScale image;
> > > > > +    size_t numberOfImages = m_imagesInSet.size();
> > > > > +    for (size_t i = 0; i < numberOfImages; ++i) {
> > > > > +        image = m_imagesInSet.at(i);
> > > > > +        if (image.scaleFactor >= scaleFactor)
> > > > > +            return image;
> > > > > +    }
> > > > > +    return image;
> > > > > +}
> > > > 
> > > > What happens if the requested scaleFactor is larger than all the available image scaleFactors?
> > > > 
> > > 
> > > You will get the largest image we have. We may want to refine this later, but it strikes me as a good starting implementation.
> > 
> > So, won't that mean in this case you'd get an empty ImageWithScale here? Which would then be passed to
> > CSSImageSetValue::cachedImageSet(CachedResourceLoader* loader, ImageWithScale image)
> > and there would be no image to load.
> > 
> > Sorry if I'm wrong. I didn't follow through everything carefully.
> > 
> 
> I don't think you'll get an empty ImageWithScale. The function looks like this:
> 
> CSSImageSetValue::ImageWithScale CSSImageSetValue::bestImageForScaleFactor(float scaleFactor)
> {
>     ImageWithScale image;
>     size_t numberOfImages = m_imagesInSet.size();
>     for (size_t i = 0; i < numberOfImages; ++i) {
>         image = m_imagesInSet.at(i);
>         if (image.scaleFactor >= scaleFactor)
>             return image;
>     }
>     return image;
> }
> 
> So imagine an input parameter of scaleFactor = 3. Also imagine m_imagesInSet has only one image in it with a scale of 1. We'll enter the loop for only one iteration, and our temporary variable will be set to the image with a scale factor of 1. We'll fail the conditional check, but then we will return that image anyway after the loop. 
> 

Duh! I knew I shouldn't drink at work :)

[Beth Dakin]

Created attachment 133150 [details]
Without Platform.h insanity this time

[Dean Jackson]

Comment on attachment 133150 [details]
Without Platform.h insanity this time

View in context: https://bugs.webkit.org/attachment.cgi?id=133150&action=review

> Source/WTF/ChangeLog:10
> +        For the time being, image-set is opt-in since the implementation is 
> +        incomplete.
> +        * wtf/Platform.h:

Missing blank link. Also, after Tim's recent email to webkit-dev, I think this should mention that it adds an ENABLE flag.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13653
> -				FD315FA212B025B100C1A359 /* webaudio */,
> +				FD315FA212B025B100C1A359 /* Modules/webaudio */,

Cruft.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:21184
> -		FD315FA212B025B100C1A359 /* webaudio */ = {
> +		FD315FA212B025B100C1A359 /* Modules/webaudio */ = {

More cruft.

> Source/WebCore/css/CSSParser.cpp:3137
> -            } else if (isGeneratedImageValue(val)) {
> +            }
> +#if ENABLE(CSS_IMAGE_SET)
> +              else if (equalIgnoringCase(val->function->name, "-webkit-image-set(")) {
> +                parsedValue = parseImageSet(m_valueList.get());

Indentation on line after #if.

With this, couldn't you leave the existing "} else if" as is and start the bit inside the #if with "} else if" (but before the existing code)?

> Source/WebCore/css/CSSParser.cpp:5767
> -            } else if (val->id == CSSValueNone)
> +            }
> +#if ENABLE(CSS_IMAGE_SET)
> +            else if (val->unit == CSSParserValue::Function && equalIgnoringCase(val->function->name, "-webkit-image-set(")) {

Same comments on the order and changing here.

> Source/WebCore/css/CSSStyleSelector.cpp:4536
> +        if (current->isImageValue() || current->isImageGeneratorValue()
> +#if ENABLE(CSS_IMAGE_SET)
> +        || current->isImageSetValue()
> +#endif

I *think* that the indentation is supposed to line up with the term in the conditional.

> LayoutTests/ChangeLog:31
> +        * platform/efl/Skipped:
> +        * platform/gtk/Skipped:

What about the test_expectations files for Chrome?

> LayoutTests/fast/css/script-tests/image-set-parsing.js:84
> +testImageSetRule("Single value for content",
> +				"content",
> +	           	"url('#a') 1x", 2,
> +                ["a", "1"]);
> +

some whacky indentation in this test

> LayoutTests/fast/hidpi/image-set-out-of-order.html:35
> +        width:100px;
> +        height:100px;

nit: space

> LayoutTests/fast/hidpi/image-set-simple.html:42
> +    <div>This test passes if the div below is a blue 100px square when the deviceScaleFactor is 1, and if it is a 100px red square when the deviceScaleFactor is 2.</div>

I wonder if we should use green instead of red. red == evil.

[Beth Dakin]

Thanks Dean! I think I addressed all of your comments.

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

[Ojan Vafai]

The new pixel tests added in this patch would be great as reftests. That way when this is enabled for all ports, we wouldn't need a ton of platform specific results.

[Beth Dakin]

Follow-up bugs:

-webkit-image-set should update dynamically when the device scale factor changes
https://bugs.webkit.org/show_bug.cgi?id=81939

-webkit-image-set should consider Page::pageScaleFactor(), Frame::pageZoomFactor(), and CSS transforms
https://bugs.webkit.org/show_bug.cgi?id=81698

-webkit-image-set should support all the image functions WebKit supports, not just url()
https://bugs.webkit.org/show_bug.cgi?id=81941

And I filed one port-specific bug to enable image-set for Chromium since it was required by the test expectations file: https://bugs.webkit.org/show_bug.cgi?id=81859