Bug 72584

Created attachment 116678 [details] Patch to add CSS image() This patch implements the parsing for image()

Comment on attachment 116678 [details] Patch to add CSS image() View in context: https://bugs.webkit.org/attachment.cgi?id=116678&action=review > Source/WebCore/ChangeLog:8 > + Parse CSS3's image() notation. Please explain what kind of changes you're making and also add a url to the spec where the notation is specified. > Source/WebCore/ChangeLog:706 > - > + Please revert these changes to old changelog entries. > Source/WebCore/css/CSSImageFallbackValue.cpp:25 > + No blank line here. > Source/WebCore/css/CSSImageFallbackValue.cpp:33 > + size_t n = this->m_images.size(); Please don't use one-letter variable line n. > Source/WebCore/css/CSSParser.cpp:6289 > + const CSSParserString& fname = val->function->name; Please don't use abbreviations like "f". > Source/WebCore/css/CSSParser.cpp:6378 > + CSSParserValue* a = args->current(); Ditto about one-letter variable.

Created attachment 116687 [details] Fixed review issues Fixed the issues mentioned by Ryosuke Niwa. Fix whitespace changes in ChangeLog, rename variables, add link to spec to the ChangeLog. The relevant spec URL is http://www.w3.org/TR/css3-images

Created attachment 116737 [details] Update patch so it applies to current trunk

Comment on attachment 116737 [details] Update patch so it applies to current trunk Attachment 116737 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10679158

Created attachment 116756 [details] Fix dyslexic gypi build break

http://www.w3.org/TR/2011/WD-css3-images-20110908/#image-notation is pretty old. You should follow http://dev.w3.org/csswg/css3-images/#image (but I'm not sure if anything changed).

(In reply to comment #7) > http://www.w3.org/TR/2011/WD-css3-images-20110908/#image-notation is pretty old. You should follow http://dev.w3.org/csswg/css3-images/#image (but I'm not sure if anything changed). Looking only at the grammar (since the patch so far only implements parsing), they appear identical. Not sure about functionality-wise.

Created attachment 120056 [details] Update patch to apply to trunk

Created attachment 120084 [details] Fix backwards diff

Comment on attachment 120084 [details] Fix backwards diff View in context: https://bugs.webkit.org/attachment.cgi?id=120084&action=review Commenting on a few things I couldn't help but notice when skimming over the patch. I did't not attempt to verify the logic, or even to comprehensively check the style. > LayoutTests/fast/css/getComputedStyle/computed-style-image.html:27 > +shouldBe('testImage("background-image: -webkit-image(\'example1.png\' ltr)", "background-image")', '"-webkit-image(\'example1.png\' ltr)"') > +shouldBe('testImage("background-image: -webkit-image(\'example1.png\' ltr)", "background-image")', '"-webkit-image(\'example1.png\' ltr)"') Given that you're testing one aspect of this already, it would seem useful to test no space between URL and "ltr", and then tabs and other whitespace characters. Test that "-webkit-image" is case insensitive? > Source/WebCore/css/CSSImageFallbackValue.cpp:7 > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. Is it intentional that you are adding new code under LGPL? Generally, we prefer BSD style on new files. > Source/WebCore/css/CSSImageFallbackValue.cpp:21 > +#include "config.h" > + > +#include "CSSImageFallbackValue.h" No space between these. > Source/WebCore/css/CSSImageFallbackValue.cpp:32 > + size_t nImages = this->m_images.size(); Please don't abbreviate. > Source/WebCore/css/CSSImageFallbackValue.h:34 > + > + > + Too much whitespace. > Source/WebCore/css/CSSImageFallbackValue.h:40 > + NONE, > + LTR, > + RTL We only use ALL_CAPS for macros and abbreviations. So, the first value should be None. > Source/WebCore/css/CSSImageFallbackValue.h:61 > + , m_images(Vector<ImageFallbackPair>()) > + , m_color(Color()) Please don't spell out initializers that just perform the default action. > Source/WebCore/css/CSSImageFallbackValue.h:70 > + String customCssText() const; s/Css/CSS/. > Source/WebCore/css/CSSImageFallbackValue.h:71 > + PassRefPtr<Image> image(RenderObject*, const IntSize&); If this function just finds an image (as opposed to creating it), raw pointer seems preferable. > Source/WebCore/css/CSSParser.cpp:35 > #include "CSSCrossfadeValue.h" > +#include "CSSImageFallbackValue.h" > #include "CSSCursorImageValue.h" Please maintain alphabetic order. I'm surprised that style bot isn't yelling. > Source/WebCore/css/CSSParser.cpp:176 > +static inline bool isComma(const CSSParserValue* a) Why "a"? I'd suggest "value". > Source/WebCore/css/CSSParser.cpp:6325 > + const CSSParserString& funcName = val->function->name; s/funcName/functionName/ > Source/WebCore/css/CSSParser.cpp:6344 > + const CSSParserString& fname = val->function->name; s/fname/functionName/ > Source/WebCore/css/CSSParser.cpp:6427 > + CSSParserValueList* args = valueList->current()->function->args.get(); s/args/arguments/. > Source/WebCore/css/CSSParser.cpp:6433 > + CSSParserValue* arg = args->current(); s/arg/argument/

Comment on attachment 120084 [details] Fix backwards diff View in context: https://bugs.webkit.org/attachment.cgi?id=120084&action=review > Source/WebCore/css/CSSImageFallbackValue.cpp:59 > + UNUSED_PARAM(renderer); You can, if you want, leave off the argument name above, and remove UNUSED_PARAM. >> Source/WebCore/css/CSSImageFallbackValue.h:70 >> + String customCssText() const; > > s/Css/CSS/. That would be inconsistent with the other CSSImageGeneratorValue subclasses. Perhaps they should all be changed? >> Source/WebCore/css/CSSImageFallbackValue.h:71 >> + PassRefPtr<Image> image(RenderObject*, const IntSize&); > > If this function just finds an image (as opposed to creating it), raw pointer seems preferable. At least in the pure color case, it'll be creating a Generator subclass (I assume, I've vaguely talked to Matt about this), and returning a new GeneratorGeneratedImage here. >> Source/WebCore/css/CSSParser.cpp:35 >> #include "CSSCursorImageValue.h" > > Please maintain alphabetic order. I'm surprised that style bot isn't yelling. Yeah, that's pretty bizarre. It's yelled at me before on numerous occasions.

> That would be inconsistent with the other CSSImageGeneratorValue subclasses. Perhaps they should all be changed? Hmm, that's weird. WebKit style says that they should all be changed. It's not exported via any IDL, so it doesn't look like we're matching an externally mandated name here. Seems acceptable to keep the prevailing style for this patch, although a person doing actual review may have a strong option one way or another.

Created attachment 120233 [details] Style fixes

(In reply to comment #11) > > Source/WebCore/css/CSSParser.cpp:6427 > > + CSSParserValueList* args = valueList->current()->function->args.get(); > > s/args/arguments/. > > > Source/WebCore/css/CSSParser.cpp:6433 > > + CSSParserValue* arg = args->current(); > > s/arg/argument/ This already comes from something named "args.get()" so I don't think calling it something different really makes sense.

Attachment 120233 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Inform the scrolling coordinator when scrollbar layers come and go Using index info to reconstruct a base tree... <stdin>:474806: trailing whitespace. [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread <stdin>:474827: trailing whitespace. Nothing to test, just removing redundant code. Correct behavior tested by <stdin>:475346: trailing whitespace. warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 167254 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/chromium/test_expectations.txt CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/page/ScrollingCoordinator.h CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm Auto-merging Source/WebCore/platform/ScrollView.cpp Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Auto-merging Tools/Scripts/build-webkit Auto-merging Tools/Scripts/webkitdirs.pm CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm Failed to merge in the changes. Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #16) > Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go Madness. Has the style bot gone nuts?

I think svn.webkit.org and git.webkit.org may have been having some trobule today which has caused some bot-sickness.

> This already comes from something named "args.get()" so I don't think calling it something different really makes sense. It's kind of an edge case, but I think that we should be improving overall adherence to WebKit style, not propagate old mistakes whenever we touch old uncorrected code.

Comment on attachment 120233 [details] Style fixes Attachment 120233 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10986259 New failing tests: fast/css/getComputedStyle/computed-style-image.html

Created attachment 121937 [details] Update patch

Attachment 121937 [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/CSSImageFallbackValue.h:57: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 121939 [details] Fix style error

Attachment 121939 [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/CSSImageFallbackValue.h:57: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 121942 [details] Actually attach the new patch

>33>c:\cygwin\home\buildbot\WebKit\Tools\TestWebKitAPI\config.h(47) : fatal error C1083: Cannot open include file: 'WebKit2/WebKit2.h': No such file or directory The windows build error doesn't look like my problem.

The level 3 spec for this is in Last Call now, so it's probably (finally) safe to get review for the parsing bits and move on to the actual implementation.

Created attachment 123592 [details] Fix merge conflict markers in changelog

FYI, The ltr and rtl keywords have now been dropped from CSS3 images due to a design issue. The feature has been deferred to Level 4 (but may wind up with a different syntax than originally suggested). The design issue (see http://lists.w3.org/Archives/Public/www-style/2012Mar/0243 and replies in "Next in thread" chain) is that it is not clear that there is a use case for specifying different modes (non-flipping, ltr, rtl) for different images in the fallback chain. That the syntax allows this may indicate that it is suboptimal. Nevertheless, it would still be useful if browsers prototype this feature in the current syntax (suitably prefixed as necessary).

*** Bug 68007 has been marked as a duplicate of this bug. ***

This functionality is now back in Level 4 (http://dev.w3.org/csswg/css4-images/#bidi-images), with a slight modification.

Created attachment 210097 [details] rebase, implement basic functionality Not for review yet, but basic functionality (solid color fallback, fallback for unavailable images, etc.) works. rtl/ltr stuff was removed from level 3 so I pulled that out.

Attachment 210097 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/css/getComputedStyle/computed-style-image-expected.txt', u'LayoutTests/fast/css/getComputedStyle/computed-style-image.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSImageFallbackValue.cpp', u'Source/WebCore/css/CSSImageFallbackValue.h', u'Source/WebCore/css/CSSImageGeneratorValue.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSValue.cpp', u'Source/WebCore/css/CSSValue.h']" exit_code: 1 Source/WebCore/css/CSSValue.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/css/CSSImageFallbackValue.h:23: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/css/CSSImageFallbackValue.h:54: The parameter name "color" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/css/CSSImageFallbackValue.h:57: The parameter name "cachedImage" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 210097 [details] rebase, implement basic functionality Attachment 210097 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1650097 New failing tests: fast/css/getComputedStyle/computed-style-image.html

Created attachment 210106 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Comment on attachment 210097 [details] rebase, implement basic functionality Attachment 210097 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1646103 New failing tests: fast/css/getComputedStyle/computed-style-image.html

Created attachment 210108 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Comment on attachment 210097 [details] rebase, implement basic functionality Attachment 210097 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1635580 New failing tests: fast/css/getComputedStyle/computed-style-image.html

Created attachment 210117 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Comment on attachment 210097 [details] rebase, implement basic functionality Attachment 210097 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1588518

Created attachment 210166 [details] revised patch

Comment on attachment 210166 [details] revised patch Attachment 210166 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1641097 New failing tests: css3/images/image-fallback.html

Created attachment 210170 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Created attachment 210177 [details] patch Ready for a very preliminary review; I have a few things (some noted in the changelog) that I need to double check.

Comment on attachment 210177 [details] patch Attachment 210177 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1582710

Comment on attachment 210177 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=210177&action=review I have a couple of questions but inline, like do you just load the images as needed, or do you load all images? (The latter is not the intention of image().) Just one opinion to image(). I think as specified it is not so useful. But it has a lot of potential. I for instance would be interested to have arguments after each STRING/URL. Thinks like type of image: just load the image if you think you support the type (see WebP). image() could also solve a problem with responsive images in CSS, just like srcset on <img>. I would have things in mind like image('image.webp' image/webp 2x, 'image.png' 2x, 'image-low.webp' image/png, 'image.png'); > LayoutTests/ChangeLog:10 > + Add a test that ensures that -webkit-image() is parsed correctly and invalid > + values are correctly identified. I thought image() was in CSS3 Images, but looks like it was dropped and delayed to CSS4. > LayoutTests/css3/images/image-fallback.html:16 > +<div style="background-image: -webkit-image(rgba(255, 255, 255, 0.8)), url('resources/green-10.png');"></div> This is not valid according to the spec > LayoutTests/css3/images/image-fallback.html:19 > + > +<br/><br/><br/> Can you add a test with a list of images that all fail? And another one where all fail and you have a fallback color? I would also like to see negative tests where you have gradients, -webkit-filter and -webkit-cross-fade as input. You seem not to have one test where you have an URL instead of a string. > LayoutTests/fast/css/getComputedStyle/computed-style-image.html:40 > +shouldBe('testImage("background-image: -webkit-image(\'dummy://hello.jpg\', #FF0000)", "background-image")', '"-webkit-image(\'dummy://hello.jpg\', #ff0000)"') I think this is wrong: image() = image( <image-tags>? [ <image-src> , ]* [ <image-src> | <color> ] ) Color must be the last value and it must not be comma separated from the last specified image. > Source/WebCore/css/CSSImageFallbackValue.cpp:17 > + * Copyright (C) 2012 Matthew Arsenault <arsenm2@rpi.edu> > + * Copyright (C) 2013 Apple Inc. All rights reserved. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA Did you refactor the code from somewhere? Otherwise, where does the copy right come from and why LGPL? > Source/WebCore/css/CSSImageFallbackValue.cpp:53 > + if (i != imageCount - 1 || m_hasColor) > + result.append(", "); Color is not comma separated. > Source/WebCore/css/CSSImageFallbackValue.h:3 > + * Copyright (C) 2012 Matthew Arsenault <arsenm2@rpi.edu> > + * Copyright (C) 2013 Apple Inc. All rights reserved. Ditto. > Source/WebCore/css/CSSParser.cpp:8141 > +bool CSSParser::parseImage(CSSParserValueList* valueList, RefPtr<CSSValue>& resultFallback) I think the name could be a bit more self descriptive. What about parseCSSImageFunction? Or just parseImageFunction? > Source/WebCore/css/CSSParser.cpp:8147 > + RefPtr<CSSImageFallbackValue> fallback = CSSImageFallbackValue::create(); Don't create this until the very last moment. > Source/WebCore/css/CSSParser.cpp:8154 > + while (argument) { > + RGBA32 color; Why not initialize it before the while loop? There should just be one color. > Source/WebCore/css/CSSParser.cpp:8156 > + if (argument->unit == CSSPrimitiveValue::CSS_STRING) { URL is missing but should be supported. You can also make that an early return. Then check if the next argument is a comma, if yes continue to next loop iteration. If not, it must be a color, or return false. > Source/WebCore/css/CSSParser.cpp:8157 > + fallback->addImage(CSSImageValue::create(completeURL(argument->string))); The thing that I do not understand it... Does creating and image already cause loading, or is CacheImageResource (and the image loading) initialized later? If every CSSImageValue that you already starts loading, then the implementation dosn't get the point of image(). The next fallback image should just be loaded when the current in row doesn't load. > Source/WebCore/css/CSSParser.cpp:8165 > + if (!isComma(argument)) > + return false; > + } else if (parseColorFromValue(argument, color)) { // Anything not an image name should be a color. well, as described above, no comma between image and color. > Source/WebCore/css/CSSParser.cpp:8169 > + argument = arguments->next(); > + if (argument) // A color can only be specified as the last fallback. > + return false; Oh, you do support color just as the last entry. > Source/WebCore/platform/graphics/SolidColorImage.cpp:45 > +void SolidColorImage::draw(GraphicsContext* context, const FloatRect& dstRect, const FloatRect&, ColorSpace styleColorSpace, CompositeOperator compositeOperator, BlendMode) > +{ > + fillWithSolidColor(context, dstRect, m_color, styleColorSpace, compositeOperator); > +} I like the concept. Simple and logical. > Source/WebCore/platform/graphics/SolidColorImage.h:45 > + virtual bool isSolidColorImage() const OVERRIDE { return true; } Is this really the case? What abour rgba(0,0,0,0) ? That would be transparent. You should check the alpha channel of the color and return the result: return m_color.hasAlpha();

Started a threat about image() function capabilities. Apparently the spec is still a bit on flux.

(In reply to comment #46) > (From update of attachment 210177 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210177&action=review > > I have a couple of questions but inline, like do you just load the images as needed, or do you load all images? (The latter is not the intention of image().) They're loaded as needed (I hope! I should check with an HTTP test!). > Just one opinion to image(). I think as specified it is not so useful. But it has a lot of potential. I for instance would be interested to have arguments after each STRING/URL. Thinks like type of image: just load the image if you think you support the type (see WebP). image() already covers the just-load-the-image-if-you-support-the-type usecase, like with WebP. If you specify a WebP image and a JPEG fallback, and the browser doesn't support WebP, it should fall back to the JPEG version. No unnecessary keywords required. Now, Level 4 does have support for ltr rtl keywords following the image, as that is something you can't infer from the image, and that does seem useful, but that's for later. > image() could also solve a problem with responsive images in CSS, just like srcset on <img>. > > I would have things in mind like > > image('image.webp' image/webp 2x, 'image.png' 2x, 'image-low.webp' image/png, 'image.png'); Sort of like -webkit-image-set? > > LayoutTests/ChangeLog:10 > > + Add a test that ensures that -webkit-image() is parsed correctly and invalid > > + values are correctly identified. > > I thought image() was in CSS3 Images, but looks like it was dropped and delayed to CSS4. Actually I'm pretty sure (see the spec I mentioned in the changelog) it's still in CSS3 images, the part that was deferred to CSS4 images is the rtl/ltr stuff. But maybe I've got that wrong because I find the standards process completely incomprehensible. > > LayoutTests/css3/images/image-fallback.html:16 > > +<div style="background-image: -webkit-image(rgba(255, 255, 255, 0.8)), url('resources/green-10.png');"></div> > > This is not valid according to the spec I certainly don't see why not. This is an image with two background image layers, one which is a color, and one which is an image. In fact, the spec explicitly endorses this example, in the "Solid-color Images" section. > > LayoutTests/css3/images/image-fallback.html:19 > > + > > +<br/><br/><br/> > > Can you add a test with a list of images that all fail? And another one where all fail and you have a fallback color? > > I would also like to see negative tests where you have gradients, -webkit-filter and -webkit-cross-fade as input. Sure. > You seem not to have one test where you have an URL instead of a string. Good point. > > LayoutTests/fast/css/getComputedStyle/computed-style-image.html:40 > > +shouldBe('testImage("background-image: -webkit-image(\'dummy://hello.jpg\', #FF0000)", "background-image")', '"-webkit-image(\'dummy://hello.jpg\', #ff0000)"') > > I think this is wrong: > > image() = image( <image-tags>? [ <image-src> , ]* [ <image-src> | <color> ] ) > > Color must be the last value and it must not be comma separated from the last specified image. Hmm? The last element has an OR in it, and is preceded by a comma. So, definitely a comma before the color. Also all the examples have commas before the colors. > > Source/WebCore/css/CSSImageFallbackValue.cpp:17 > > + * Copyright (C) 2012 Matthew Arsenault <arsenm2@rpi.edu> > > + * Copyright (C) 2013 Apple Inc. All rights reserved. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > > Did you refactor the code from somewhere? Otherwise, where does the copy right come from and why LGPL? See the history of this patch, in this bug. > > > Source/WebCore/css/CSSImageFallbackValue.cpp:53 > > + if (i != imageCount - 1 || m_hasColor) > > + result.append(", "); > > Color is not comma separated. I don't understand why not, as above. > > Source/WebCore/css/CSSImageFallbackValue.h:3 > > + * Copyright (C) 2012 Matthew Arsenault <arsenm2@rpi.edu> > > + * Copyright (C) 2013 Apple Inc. All rights reserved. > > Ditto. And ditto. > > Source/WebCore/css/CSSParser.cpp:8141 > > +bool CSSParser::parseImage(CSSParserValueList* valueList, RefPtr<CSSValue>& resultFallback) > > I think the name could be a bit more self descriptive. What about parseCSSImageFunction? Or just parseImageFunction? Sure. > > Source/WebCore/css/CSSParser.cpp:8147 > > + RefPtr<CSSImageFallbackValue> fallback = CSSImageFallbackValue::create(); > > Don't create this until the very last moment. Ok. > > Source/WebCore/css/CSSParser.cpp:8154 > > + while (argument) { > > + RGBA32 color; > > Why not initialize it before the while loop? There should just be one color. Indeed. > > Source/WebCore/css/CSSParser.cpp:8156 > > + if (argument->unit == CSSPrimitiveValue::CSS_STRING) { > > URL is missing but should be supported. You can also make that an early return. Then check if the next argument is a comma, if yes continue to next loop iteration. If not, it must be a color, or return false. Yep, on the URL bit. > > Source/WebCore/css/CSSParser.cpp:8157 > > + fallback->addImage(CSSImageValue::create(completeURL(argument->string))); > > The thing that I do not understand it... Does creating and image already cause loading, or is CacheImageResource (and the image loading) initialized later? Image loading happens later, in CSSImageFallbackValue's loadSubimages and friends. > If every CSSImageValue that you already starts loading, then the implementation dosn't get the point of image(). The next fallback image should just be loaded when the current in row doesn't load. Indeed. > > Source/WebCore/css/CSSParser.cpp:8165 > > + if (!isComma(argument)) > > + return false; > > + } else if (parseColorFromValue(argument, color)) { // Anything not an image name should be a color. > > well, as described above, no comma between image and color. I continue to disagree :D > > Source/WebCore/css/CSSParser.cpp:8169 > > + argument = arguments->next(); > > + if (argument) // A color can only be specified as the last fallback. > > + return false; > > Oh, you do support color just as the last entry. As it's specified. > > Source/WebCore/platform/graphics/SolidColorImage.cpp:45 > > +void SolidColorImage::draw(GraphicsContext* context, const FloatRect& dstRect, const FloatRect&, ColorSpace styleColorSpace, CompositeOperator compositeOperator, BlendMode) > > +{ > > + fillWithSolidColor(context, dstRect, m_color, styleColorSpace, compositeOperator); > > +} > > I like the concept. Simple and logical. > > > Source/WebCore/platform/graphics/SolidColorImage.h:45 > > + virtual bool isSolidColorImage() const OVERRIDE { return true; } > > Is this really the case? What abour rgba(0,0,0,0) ? That would be transparent. You should check the alpha channel of the color and return the result: > > return m_color.hasAlpha(); What? This is the "is this Image subclass a SolidColorImage class" fake rtti nonsense. It's not dependent at all on the color. In any case, a "solid" color could still have alpha, that would just make it a *non-opaque* color :D

> > Just one opinion to image(). I think as specified it is not so useful. But it has a lot of potential. I for instance would be interested to have arguments after each STRING/URL. Thinks like type of image: just load the image if you think you support the type (see WebP). > > image() already covers the just-load-the-image-if-you-support-the-type usecase, like with WebP. If you specify a WebP image and a JPEG fallback, and the browser doesn't support WebP, it should fall back to the JPEG version. No unnecessary keywords required. Do note that I'm not sure this works as-is, but it is supposed to work.

(In reply to comment #48) > (In reply to comment #46) > > (From update of attachment 210177 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=210177&action=review > > > > I have a couple of questions but inline, like do you just load the images as needed, or do you load all images? (The latter is not the intention of image().) > > They're loaded as needed (I hope! I should check with an HTTP test!). > > > Just one opinion to image(). I think as specified it is not so useful. But it has a lot of potential. I for instance would be interested to have arguments after each STRING/URL. Thinks like type of image: just load the image if you think you support the type (see WebP). > > image() already covers the just-load-the-image-if-you-support-the-type usecase, like with WebP. If you specify a WebP image and a JPEG fallback, and the browser doesn't support WebP, it should fall back to the JPEG version. No unnecessary keywords required. Well, you need to fetch the resource to get the information. Not the downloading is the problem nowadays, but the requesting of resources. With this keyword you can avoid requesting the image. > > Now, Level 4 does have support for ltr rtl keywords following the image, as that is something you can't infer from the image, and that does seem useful, but that's for later. > > > image() could also solve a problem with responsive images in CSS, just like srcset on <img>. > > > > I would have things in mind like > > > > image('image.webp' image/webp 2x, 'image.png' 2x, 'image-low.webp' image/png, 'image.png'); > > Sort of like -webkit-image-set? Right, this is part of the linked discussion on www-style. ... like -webkit-image-set but combined with image() seems more useful for me. > > > > LayoutTests/ChangeLog:10 > > > + Add a test that ensures that -webkit-image() is parsed correctly and invalid > > > + values are correctly identified. > > > > I thought image() was in CSS3 Images, but looks like it was dropped and delayed to CSS4. > > Actually I'm pretty sure (see the spec I mentioned in the changelog) it's still in CSS3 images, the part that was deferred to CSS4 images is the rtl/ltr stuff. But maybe I've got that wrong because I find the standards process completely incomprehensible. Oh you are right. In this case r- for prefixing the function!!!! The spec is in CR and should be implemented unprefixed. You may can add a compiler flag if you want to. IIRC you did not announce implementing image() on webkit-dev anyway. > > > > LayoutTests/css3/images/image-fallback.html:16 > > > +<div style="background-image: -webkit-image(rgba(255, 255, 255, 0.8)), url('resources/green-10.png');"></div> > > > > This is not valid according to the spec > > I certainly don't see why not. This is an image with two background image layers, one which is a color, and one which is an image. In fact, the spec explicitly endorses this example, in the "Solid-color Images" section. You are absolutely right and I am wrong. Ditto for the other comments about the comma. Misunderstood the grammar. > > > > LayoutTests/css3/images/image-fallback.html:19 > > > + > > > +<br/><br/><br/> > > > > Can you add a test with a list of images that all fail? And another one where all fail and you have a fallback color? > > > > I would also like to see negative tests where you have gradients, -webkit-filter and -webkit-cross-fade as input. > > Sure. > > > You seem not to have one test where you have an URL instead of a string. > > Good point. > > > > LayoutTests/fast/css/getComputedStyle/computed-style-image.html:40 > > > +shouldBe('testImage("background-image: -webkit-image(\'dummy://hello.jpg\', #FF0000)", "background-image")', '"-webkit-image(\'dummy://hello.jpg\', #ff0000)"') > > > > I think this is wrong: > > > > image() = image( <image-tags>? [ <image-src> , ]* [ <image-src> | <color> ] ) > > > > Color must be the last value and it must not be comma separated from the last specified image. > > Hmm? The last element has an OR in it, and is preceded by a comma. So, definitely a comma before the color. Also all the examples have commas before the colors. Yes, see comment above. > > > > Source/WebCore/css/CSSImageFallbackValue.cpp:17 > > > + * Copyright (C) 2012 Matthew Arsenault <arsenm2@rpi.edu> > > > + * Copyright (C) 2013 Apple Inc. All rights reserved. > > > + * > > > + * This library is free software; you can redistribute it and/or > > > + * modify it under the terms of the GNU Lesser General Public > > > + * License as published by the Free Software Foundation; either > > > + * version 2.1 of the License, or (at your option) any later version. > > > + * > > > + * This library is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > + * Lesser General Public License for more details. > > > + * > > > + * You should have received a copy of the GNU Lesser General Public > > > + * License along with this library; if not, write to the Free Software > > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > > > > Did you refactor the code from somewhere? Otherwise, where does the copy right come from and why LGPL? > > See the history of this patch, in this bug. I couldn't see it from the patch but trust you. > > > > > > Source/WebCore/css/CSSImageFallbackValue.cpp:53 > > > + if (i != imageCount - 1 || m_hasColor) > > > + result.append(", "); > > > > Color is not comma separated. > > I don't understand why not, as above. > > > > Source/WebCore/css/CSSImageFallbackValue.h:3 > > > + * Copyright (C) 2012 Matthew Arsenault <arsenm2@rpi.edu> > > > + * Copyright (C) 2013 Apple Inc. All rights reserved. > > > > Ditto. > > And ditto. > > > > Source/WebCore/css/CSSParser.cpp:8141 > > > +bool CSSParser::parseImage(CSSParserValueList* valueList, RefPtr<CSSValue>& resultFallback) > > > > I think the name could be a bit more self descriptive. What about parseCSSImageFunction? Or just parseImageFunction? > > Sure. > > > > Source/WebCore/css/CSSParser.cpp:8147 > > > + RefPtr<CSSImageFallbackValue> fallback = CSSImageFallbackValue::create(); > > > > Don't create this until the very last moment. > > Ok. > > > > Source/WebCore/css/CSSParser.cpp:8154 > > > + while (argument) { > > > + RGBA32 color; > > > > Why not initialize it before the while loop? There should just be one color. > > Indeed. > > > > Source/WebCore/css/CSSParser.cpp:8156 > > > + if (argument->unit == CSSPrimitiveValue::CSS_STRING) { > > > > URL is missing but should be supported. You can also make that an early return. Then check if the next argument is a comma, if yes continue to next loop iteration. If not, it must be a color, or return false. > > Yep, on the URL bit. > > > > Source/WebCore/css/CSSParser.cpp:8157 > > > + fallback->addImage(CSSImageValue::create(completeURL(argument->string))); > > > > The thing that I do not understand it... Does creating and image already cause loading, or is CacheImageResource (and the image loading) initialized later? > > Image loading happens later, in CSSImageFallbackValue's loadSubimages and friends. > > > If every CSSImageValue that you already starts loading, then the implementation dosn't get the point of image(). The next fallback image should just be loaded when the current in row doesn't load. > > Indeed. > > > > Source/WebCore/css/CSSParser.cpp:8165 > > > + if (!isComma(argument)) > > > + return false; > > > + } else if (parseColorFromValue(argument, color)) { // Anything not an image name should be a color. > > > > well, as described above, no comma between image and color. > > I continue to disagree :D Damn pigheaded fellow! :D > > > > Source/WebCore/css/CSSParser.cpp:8169 > > > + argument = arguments->next(); > > > + if (argument) // A color can only be specified as the last fallback. > > > + return false; > > > > Oh, you do support color just as the last entry. > > As it's specified. > > > > Source/WebCore/platform/graphics/SolidColorImage.cpp:45 > > > +void SolidColorImage::draw(GraphicsContext* context, const FloatRect& dstRect, const FloatRect&, ColorSpace styleColorSpace, CompositeOperator compositeOperator, BlendMode) > > > +{ > > > + fillWithSolidColor(context, dstRect, m_color, styleColorSpace, compositeOperator); > > > +} > > > > I like the concept. Simple and logical. > > > > > Source/WebCore/platform/graphics/SolidColorImage.h:45 > > > + virtual bool isSolidColorImage() const OVERRIDE { return true; } > > > > Is this really the case? What abour rgba(0,0,0,0) ? That would be transparent. You should check the alpha channel of the color and return the result: > > > > return m_color.hasAlpha(); > > What? This is the "is this Image subclass a SolidColorImage class" fake rtti nonsense. It's not dependent at all on the color. In any case, a "solid" color could still have alpha, that would just make it a *non-opaque* color :D Ok.

> > > I thought image() was in CSS3 Images, but looks like it was dropped and delayed to CSS4. > > > > Actually I'm pretty sure (see the spec I mentioned in the changelog) it's still in CSS3 images, the part that was deferred to CSS4 images is the rtl/ltr stuff. But maybe I've got that wrong because I find the standards process completely incomprehensible. > > Oh you are right. In this case r- for prefixing the function!!!! The spec is in CR and should be implemented unprefixed. You may can add a compiler flag if you want to. IIRC you did not announce implementing image() on webkit-dev anyway. Just because something is in CR doesn't mean we implement it without a prefix? We should only do that if we are extremely certain it won't change (which I don't think we are). This is an experimental feature, and should be treated as such.

(In reply to comment #51) > > > > I thought image() was in CSS3 Images, but looks like it was dropped and delayed to CSS4. > > > > > > Actually I'm pretty sure (see the spec I mentioned in the changelog) it's still in CSS3 images, the part that was deferred to CSS4 images is the rtl/ltr stuff. But maybe I've got that wrong because I find the standards process completely incomprehensible. > > > > Oh you are right. In this case r- for prefixing the function!!!! The spec is in CR and should be implemented unprefixed. You may can add a compiler flag if you want to. IIRC you did not announce implementing image() on webkit-dev anyway. > > Just because something is in CR doesn't mean we implement it without a prefix? We should only do that if we are extremely certain it won't change (which I don't think we are). This is an experimental feature, and should be treated as such. The CSS WG charta is clear about that. If a specification is in CR, it shall be (might even be a must) implemented without prefix. It is unlikely that a specification that is in CR will change drastically. This was not the case so far. CR is requiring implementations and implementation feedback. A future spec might add functionality to features but this must be in a compatible way. If you worry that we reveal problems with image() on the way of implementing it, we should do it behind a compile time flag.

(In reply to comment #52) > > The CSS WG charta is clear about that. If a specification is in CR, it shall be (might even be a must) implemented without prefix. Nope. The CSS WG's prefixing policy defines when UAs MAY ship an unprefixed version, but cannot constrain a UA from choosing to ship a prefixed version if, in that UA's view, the feature is immature.

(In reply to comment #53) > (In reply to comment #52) > > > > The CSS WG charta is clear about that. If a specification is in CR, it shall be (might even be a must) implemented without prefix. > > Nope. The CSS WG's prefixing policy defines when UAs MAY ship an unprefixed version, but cannot constrain a UA from choosing to ship a prefixed version if, in that UA's view, the feature is immature. I checked with Ted. The CSS WG has a wiki page where it is explicitly stated that the implemented MUST NOT implement a feature from a CSS specification in CR. [1] However, this is a guidance and the Charter and official process do not define the usage of prefixes at all. [2] [1] http://wiki.csswg.org/spec/vendor-prefixes#candidate-recommendation-features [2] http://www.w3.org/Style/2011/CSS-process

(In reply to comment #54) > > The CSS WG has a wiki page where it is explicitly stated that the implemented MUST NOT implement a feature from a CSS specification in CR. [1] That wiki page doesn't reflect the consensus of the WG, and really should carry a disclaimer to that effect.

Prefixed or not, is there any update on this patch? Especially with things like background-blend-mode image(<color>) can be very helpful.

<rdar://problem/56518920>

https://drafts.csswg.org/css-images-4/#funcdef-image is the current spec. Unclear how much interest there is cross-browser in this. See https://bugzilla.mozilla.org/show_bug.cgi?id=703217 for a Mozilla bug for this; can't see any Chromium bug for this.