[webkit-reviews] review denied: [Bug 72584] Support Image Level 3's image() notation : [Attachment 210177] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Sep 1 22:32:46 PDT 2013


Dirk Schulze <krit at webkit.org> has denied Tim Horton <thorton at apple.com>'s
request for review:
Bug 72584: Support Image Level 3's image() notation
https://bugs.webkit.org/show_bug.cgi?id=72584

Attachment 210177: patch
https://bugs.webkit.org/attachment.cgi?id=210177&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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 at 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 at 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();


More information about the webkit-reviews mailing list