Bug 102571

Summary: [CSS Exclusions] Support outside-shape layout for shape-inside property
Product: WebKit Reporter: Bear Travis <betravis>
Component: CSSAssignee: Bear Travis <betravis>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, eric, ojan.autocc, rego+ews, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 101108    
Bug Blocks: 89256, 109605    
Attachments:
Description Flags
Initial patch
none
Fixing typo
none
Updating patch
none
Adding resolvedShapeInside to RenderStyle none

Description Bear Travis 2012-11-16 16:23:29 PST
Implement the layout portion of bug 101108
Comment 1 Bear Travis 2012-11-20 11:08:36 PST
Also shift to use outside-shape as the default value for shape-inside.
Comment 2 Bear Travis 2013-02-12 13:14:45 PST
Created attachment 187916 [details]
Initial patch

Implementing outside-shape layout, waiting to make it the default value until bug 109605.
Comment 3 Early Warning System Bot 2013-02-12 13:20:42 PST
Comment on attachment 187916 [details]
Initial patch

Attachment 187916 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16483373
Comment 4 Bear Travis 2013-02-12 13:36:08 PST
Created attachment 187921 [details]
Fixing typo
Comment 5 Early Warning System Bot 2013-02-12 13:41:25 PST
Comment on attachment 187921 [details]
Fixing typo

Attachment 187921 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16440425
Comment 6 Early Warning System Bot 2013-02-12 13:43:41 PST
Comment on attachment 187921 [details]
Fixing typo

Attachment 187921 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16440427
Comment 7 Bear Travis 2013-02-12 13:58:42 PST
Created attachment 187927 [details]
Updating patch
Comment 8 Dave Hyatt 2013-02-14 13:40:13 PST
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.
Comment 9 Bear Travis 2013-02-14 15:57:24 PST
Created attachment 188439 [details]
Adding resolvedShapeInside to RenderStyle

Consolidating repeated code into a helper method in RenderStyle.
Comment 10 Dave Hyatt 2013-02-18 08:09:27 PST
Comment on attachment 188439 [details]
Adding resolvedShapeInside to RenderStyle

r=me
Comment 11 WebKit Review Bot 2013-02-18 08:50:21 PST
Comment on attachment 188439 [details]
Adding resolvedShapeInside to RenderStyle

Clearing flags on attachment: 188439

Committed r143225: <http://trac.webkit.org/changeset/143225>
Comment 12 WebKit Review Bot 2013-02-18 08:50:25 PST
All reviewed patches have been landed.  Closing bug.