[webkit-reviews] review denied: [Bug 102571] [CSS Exclusions] Support outside-shape layout for shape-inside property : [Attachment 187927] Updating patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Bear Travis <betravis at adobe.com>'s
request for review:
Bug 102571: [CSS Exclusions] Support outside-shape layout for shape-inside
property
https://bugs.webkit.org/show_bug.cgi?id=102571

Attachment 187927: Updating patch
https://bugs.webkit.org/attachment.cgi?id=187927&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.