Bug 80322 - Implement image-set
: Implement image-set
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2012-03-05 13:16 PST by
Modified: 2012-03-22 14:38 PST (History)


Attachments
Patch: image-set for the content property (298.56 KB, patch)
2012-03-13 11:55 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Patch with proper license (i think) (298.58 KB, patch)
2012-03-15 16:30 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Patch that works for all image properties (680.44 KB, text/plain)
2012-03-19 17:15 PST, Beth Dakin
no flags Details
Marked a s a patch this time (680.44 KB, patch)
2012-03-19 17:16 PST, Beth Dakin
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch that will hopefully build (680.39 KB, patch)
2012-03-20 10:44 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Patch that will hopefully apply and build (680.39 KB, patch)
2012-03-20 12:57 PST, Beth Dakin
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Another go for the bots (680.41 KB, patch)
2012-03-20 14:01 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Patch with parsing tests (729.83 KB, patch)
2012-03-21 16:31 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Without Platform.h insanity this time (695.84 KB, patch)
2012-03-21 17:19 PST, Beth Dakin
dino: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-05 13:16:19 PST
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>
------- Comment #1 From 2012-03-06 11:47:03 PST -------
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.
------- Comment #2 From 2012-03-13 11:55:54 PST -------
Created an attachment (id=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)
------- Comment #3 From 2012-03-13 12:02:40 PST -------
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 #4 From 2012-03-15 12:07:23 PST -------
(From update of attachment 131687 [details])
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.
------- Comment #5 From 2012-03-15 16:30:28 PST -------
Created an attachment (id=132144) [details]
Patch with proper license (i think)
------- Comment #6 From 2012-03-19 17:15:08 PST -------
Created an attachment (id=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.
------- Comment #7 From 2012-03-19 17:16:55 PST -------
Created an attachment (id=132729) [details]
Marked a s a patch this time

Oops, forgot to mark that last one as a patch.
------- Comment #8 From 2012-03-19 17:20:54 PST -------
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.
------- Comment #9 From 2012-03-19 17:56:31 PST -------
(From update of attachment 132729 [details])
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.
------- Comment #10 From 2012-03-19 20:09:39 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11999025
------- Comment #11 From 2012-03-19 20:28:32 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11995025
------- Comment #12 From 2012-03-19 20:30:48 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11994055
------- Comment #13 From 2012-03-19 21:07:25 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12005051
------- Comment #14 From 2012-03-19 21:36:48 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11994087
------- Comment #15 From 2012-03-19 22:04:18 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12001056
------- Comment #16 From 2012-03-20 01:25:53 PST -------
(From update of attachment 132729 [details])
Attachment 132729 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12001130
------- Comment #17 From 2012-03-20 10:44:40 PST -------
Created an attachment (id=132851) [details]
Patch that will hopefully build
------- Comment #18 From 2012-03-20 12:57:37 PST -------
Created an attachment (id=132880) [details]
Patch that will hopefully apply and build
------- Comment #19 From 2012-03-20 13:00:55 PST -------
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.
------- Comment #20 From 2012-03-20 13:55:24 PST -------
(From update of attachment 132880 [details])
Attachment 132880 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12044009
------- Comment #21 From 2012-03-20 14:01:16 PST -------
Created an attachment (id=132889) [details]
Another go for the bots
------- Comment #22 From 2012-03-20 14:03:28 PST -------
(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.

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?
------- Comment #23 From 2012-03-20 14:04:54 PST -------
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.
------- Comment #24 From 2012-03-20 14:39:18 PST -------
(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 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.
------- Comment #25 From 2012-03-20 17:06:27 PST -------
(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.


> 
> > > 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.
------- Comment #26 From 2012-03-20 17:13:08 PST -------
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > (From update of attachment 132851 [details] [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.
------- Comment #27 From 2012-03-21 16:31:28 PST -------
Created an attachment (id=133140) [details]
Patch with parsing tests
------- Comment #28 From 2012-03-21 17:07:26 PST -------
(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 :)
------- Comment #29 From 2012-03-21 17:19:57 PST -------
Created an attachment (id=133150) [details]
Without Platform.h insanity this time
------- Comment #30 From 2012-03-21 17:46:32 PST -------
(From update of attachment 133150 [details])
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.
------- Comment #31 From 2012-03-21 18:37:20 PST -------
Thanks Dean! I think I addressed all of your comments.

http://trac.webkit.org/changeset/111637
------- Comment #32 From 2012-03-22 10:15:58 PST -------
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.
------- Comment #33 From 2012-03-22 11:42:33 PST -------
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