Bug 131444 – [CSS Shapes] Add support for shape-outside gradient values

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Zoltan Horvath]

As the CSS Shapes Level 1 spec says [1] shape-outside can take image values. Image values can be gradients [2]. Thus we need to add support for gradients [3].

[1] http://www.w3.org/TR/2014/CR-css-shapes-1-20140320/#shapes-from-image
[2] http://www.w3.org/TR/css3-images/#image-type
[3] http://www.w3.org/TR/css3-images/#gradients

[Hans Muller]

Created attachment 229558 [details]
Patch

Added support for shape-outside gradient values.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/style/ShapeValue.h:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:151:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 9 files


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

[Hans Muller]

Created attachment 229561 [details]
Patch

Fixed some styleOs.

[Hans Muller]

Created attachment 229578 [details]
Patch

Removed a spurious ShapeOutsideInfo.cpp include.

[Bem Jones-Bey]

Comment on attachment 229578 [details]
Patch

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

> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:123
> +static inline bool checkShapeImageOrigin(Document& document, const ShapeValue& shapeValue)

Why take a ShapeValue instead of a StyleImage?

> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:128
> +    CachedImage& cachedImage = *(shapeValue.image()->cachedImage());

I'd ASSERT(shapeValue.image()->cachedImage()) first, just to be safe.

> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:151
> +        image = styleImage->image(const_cast<RenderBox*>(&renderBox), imageSize).get();

The const_cast makes me very uncomfortable. There isn't a better way to do this? Maybe we need to refactor StyleImage so it can take a const?

[Hans Muller]

(In reply to comment #5)
> (From update of attachment 229578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229578&action=review
> 
> > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:123
> > +static inline bool checkShapeImageOrigin(Document& document, const ShapeValue& shapeValue)
> 
> Why take a ShapeValue instead of a StyleImage?

I've changed the parameter to StyleImage*.


> > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:128
> > +    CachedImage& cachedImage = *(shapeValue.image()->cachedImage());
> 
> I'd ASSERT(shapeValue.image()->cachedImage()) first, just to be safe.

OK.

> 
> > Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:151
> > +        image = styleImage->image(const_cast<RenderBox*>(&renderBox), imageSize).get();
> 
> The const_cast makes me very uncomfortable. There isn't a better way to do this? Maybe we need to refactor StyleImage so it can take a const?

I did take a shot at that. The overall change would be much bigger than this patch.  There are about 10 classes that implement this image() method and in some cases the not-const RenderElement is passed along to other methods.  If it's worth tracking down the complete transitive closure of the change, it might be better to do it as a separate patch.

[Hans Muller]

Created attachment 229662 [details]
Patch

Made the suggested changes.

[Bem Jones-Bey]

Comment on attachment 229662 [details]
Patch

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

> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:125
> +    ASSERT(styleImage);

if this can't be null, why not pass a reference? (even better, can it be a const ref?)

> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:153
> +        image = styleImage->image(const_cast<RenderBox*>(&renderBox), imageSize).get();

It would be nice to see a follow-up patch that removes the need for the const_cast here.

[Hans Muller]

Created attachment 229682 [details]
Patch

I've made the checkShapeImageOrigin() StyleImage* parameter a const StyleImage&.

I'll attempt to make this StyleImage function's RenderElement* parameter, const RenderElement*, in a separate patch:

    virtual PassRefPtr<Image> image(RenderElement*, const FloatSize&) const = 0;

[Bem Jones-Bey]

Comment on attachment 229682 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 229682 [details]
Patch

Clearing flags on attachment: 229682

Committed r167518: <http://trac.webkit.org/changeset/167518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.