Bug 102571 – [CSS Exclusions] Support outside-shape layout for shape-inside property

• Component: CSS
• Status: RESOLVED FIXED

[Bear Travis]

Implement the layout portion of bug 101108

[Bear Travis]

Also shift to use outside-shape as the default value for shape-inside.

[Bear Travis]

Created attachment 187916 [details]
Initial patch

Implementing outside-shape layout, waiting to make it the default value until bug 109605.

[Early Warning System Bot]

Comment on attachment 187916 [details]
Initial patch

Attachment 187916 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16483373

[Bear Travis]

Created attachment 187921 [details]
Fixing typo

[Early Warning System Bot]

Comment on attachment 187921 [details]
Fixing typo

Attachment 187921 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16440425

[Early Warning System Bot]

Comment on attachment 187921 [details]
Fixing typo

Attachment 187921 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16440427

[Bear Travis]

Created attachment 187927 [details]
Updating patch

[Dave Hyatt]

Comment on attachment 187927 [details]
Updating patch

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

r-

> Source/WebCore/rendering/ExclusionShapeInfo.cpp:49
>      ExclusionShapeValue* shapeValue = (m_renderer->style()->*shapeGetter)();
> +    if (shapeValue && shapeValue->type() == ExclusionShapeValue::OUTSIDE)
> +        shapeValue = m_renderer->style()->shapeOutside();

Couldn't you just fold all this into a single getter method rather than having to call another method for OUTSIDE?

> Source/WebCore/rendering/ExclusionShapeInsideInfo.h:64
>          ExclusionShapeValue* shapeValue = renderer->style()->shapeInside();
> +        if (shapeValue && shapeValue->type() == ExclusionShapeValue::OUTSIDE)
> +            shapeValue = renderer->style()->shapeOutside();

Same question here. Really feels like you could create a method that does this so that the call site doesn't have to special case.

> Source/WebCore/rendering/RenderBlock.cpp:1378
> +    const ExclusionShapeValue* shapeInside = newStyle->shapeInside();
> +    if (shapeInside && shapeInside->type() == ExclusionShapeValue::OUTSIDE)
> +        shapeInside = newStyle->shapeOutside();

Same complaint here.  Don't be afraid to add a helper to RenderStyle that gives back the correct value and hides this.

> Source/WebCore/rendering/RenderBlock.cpp:1381
> +    if (oldShapeInside && oldShapeInside->type() == ExclusionShapeValue::OUTSIDE)
> +        oldShapeInside = oldStyle->shapeOutside();

Same here.

[Bear Travis]

Created attachment 188439 [details]
Adding resolvedShapeInside to RenderStyle

Consolidating repeated code into a helper method in RenderStyle.

[Dave Hyatt]

Comment on attachment 188439 [details]
Adding resolvedShapeInside to RenderStyle

r=me

[WebKit Review Bot]

Comment on attachment 188439 [details]
Adding resolvedShapeInside to RenderStyle

Clearing flags on attachment: 188439

Committed r143225: <http://trac.webkit.org/changeset/143225>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.