[Webkit-unassigned] [Bug 72584] Support Image Level 3's image() notation

Mon Sep 2 09:15:36 PDT 2013

https://bugs.webkit.org/show_bug.cgi?id=72584

--- Comment #48 from Tim Horton <thorton at apple.com>  2013-09-02 09:14:55 PST ---
(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 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?

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 at 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.