Bug 101108 – [CSS Exclusions] Support outside-shape value on shape-inside

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

[Bear Travis]

Make sure values outside-shape and auto work properly

[Bear Travis]

Created attachment 174764 [details]
Initial patch

Patch adding CSS support for the 'outside-shape' value of shape-inside. Layout support to follow in a subsequent patch.

[Build Bot]

Comment on attachment 174764 [details]
Initial patch

Attachment 174764 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14872382

[Bear Travis]

Created attachment 175009 [details]
Fixing windows build failure

Changing ExclusionShapeValue to not conditionally compile, as RenderStyle::shapeInside/Outside must return a value regardless of whether CSS Exclusions are enabled.

[Dirk Schulze]

Comment on attachment 175009 [details]
Fixing windows build failure

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

I think OutsideExclusionShapeValue does not make any sense. It is adding a new class without any sense IMO.

> Source/WebCore/ChangeLog:11
> +        auto (null), basic shape, or outside-shape. Eventually there will be a url

Well, it is <shape> (a Basic Shape), 'outside-shape' and 'auto'. It should be more clear here.

> Source/WebCore/css/CSSValueKeywords.in:917
>  // -webkit-wrap-shape

this is not called wrap-shape anymore, is it? Can you update this comment please?

> Source/WebCore/css/StyleBuilder.cpp:1734
> +                setValue(styleResolver->style(), OutsideExclusionShapeValue::create());

There should be an FIXME here, with a link to a bug report.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:155
> +    const BasicShape* fromShape = static_cast<ShapeExclusionShapeValue*>(from)->shape();
> +    const BasicShape* toShape = static_cast<ShapeExclusionShapeValue*>(to)->shape();
> +
> +    if (!fromShape->canBlend(toShape))
> +        return to;

You may want to blend with outside shape in the future. Please add an FIXME here.

> Source/WebCore/rendering/RenderBlock.h:525
> +    void updateExclusionShapeInsideInfoAfterStyleChange(const ExclusionShapeValue*, const ExclusionShapeValue* oldWrapShape);

Can you rename it to oldExclusionShape?

> Source/WebCore/rendering/style/ExclusionShapeValue.h:69
> +    : ExclusionShapeValue(SHAPE)
> +    , m_shape(shape)

indentation looks wrong here.

> Source/WebCore/rendering/style/ExclusionShapeValue.h:83
> +class OutsideExclusionShapeValue : public ExclusionShapeValue {
> +public:
> +    static PassRefPtr<OutsideExclusionShapeValue> create()
> +    {
> +        return adoptRef(new OutsideExclusionShapeValue());
> +    }
> +
> +private:
> +    OutsideExclusionShapeValue() : ExclusionShapeValue(OUTSIDE) { }
> +};

I don't think that you need to create this class at this point. It doesn't have any information. Just use ExclusionShapeValue for everything and remove OutsideExclusionShapeValue and ShapeExclusionShapeValue for now. Then we don't have unnecessary vtables and inheritance.

[Bear Travis]

Created attachment 175074 [details]
Incorporating Dirk's feedback

[Bear Travis]

Created attachment 175249 [details]
Updating patch

[Dirk Schulze]

Comment on attachment 175249 [details]
Updating patch

LGTM. r=me

[WebKit Review Bot]

Comment on attachment 175249 [details]
Updating patch

Clearing flags on attachment: 175249

Committed r135314: <http://trac.webkit.org/changeset/135314>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.