Bug 102571 - [CSS Exclusions] Support outside-shape layout for shape-inside property
Summary: [CSS Exclusions] Support outside-shape layout for shape-inside property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
Depends on: 101108
Blocks: 89256 109605
  Show dependency treegraph
 
Reported: 2012-11-16 16:23 PST by Bear Travis
Modified: 2013-02-18 08:50 PST (History)
7 users (show)

See Also:


Attachments
Initial patch (9.48 KB, patch)
2013-02-12 13:14 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Fixing typo (9.41 KB, patch)
2013-02-12 13:36 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updating patch (9.40 KB, patch)
2013-02-12 13:58 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Adding resolvedShapeInside to RenderStyle (9.78 KB, patch)
2013-02-14 15:57 PST, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.