[webkit-reviews] review granted: [Bug 89993] [CSS Exclusions] Enable css exclusions for multiple blocks per element : [Attachment 165463] Incorporating feedback

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Bear Travis
<betravis at adobe.com>'s request for review:
Bug 89993: [CSS Exclusions] Enable css exclusions for multiple blocks per
element
https://bugs.webkit.org/show_bug.cgi?id=89993

Attachment 165463: Incorporating feedback
https://bugs.webkit.org/attachment.cgi?id=165463&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165463&action=review


> Source/WebCore/rendering/LayoutState.cpp:116
> +	   RenderBlock* renderBlock = toRenderBlock(renderer);
> +	   m_wrapShapeInfo = renderBlock->wrapShapeInfo();

Unless you are sure you need |renderBlock| in a follow-up patch, I would write
it on one line (as it is not used outside this scope):

m_wrapShapeInfo = toRenderBlock(renderer)->wrapShapeInfo();

> LayoutTests/fast/exclusions/shape-inside/shape-inside-multiple-blocks.html:12

> +	   -webkit-shape-inside: rectangle(10px, 10px, 180px, 280px);

It's usually better to select different values for each direction (as you did
for the vertical writing mode test). That makes it easier to spot the wrong
size being used.