[webkit-reviews] review granted: [Bug 106927] [CSS Exclusions] Handle shape-outside changing a float's overhang behavior : [Attachment 187189] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Bem Jones-Bey
<bjonesbe at adobe.com>'s request for review:
Bug 106927: [CSS Exclusions] Handle shape-outside changing a float's overhang
behavior
https://bugs.webkit.org/show_bug.cgi?id=106927

Attachment 187189: Patch
https://bugs.webkit.org/attachment.cgi?id=187189&action=review

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


> Source/WebCore/rendering/RenderBoxModelObject.cpp:544
> +	   ExclusionShapeOutsideInfo* shapeOutside =
toRenderBox(this)->exclusionShapeOutsideInfo();
> +	   if (shapeOutside)

This should be on one line.

> Source/WebCore/rendering/RenderLayer.h:1127
>      // Our current relative position offset.

Let's update this comment as it is stale now.

// Paint time offset only, it is used for properly paint relative / sticky
positioned elements and exclusion boxes on floats.

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-overhang.
html:31
> +	 You should only be able to see part of this text. If you can read all
of this, then it isn't working.

I would capitalize the second sentence and make it more prominent that this is
the failure:

You should only be able to see this sentence. <span style="color: red">IF YOU
SEE THIS TEXT, THE TEST HAS FAILED.</span>

If you expect only to see part of the second sentence, this test should defined
a fixed font-size (probably Ahem) and use a text that ensures the proper layout
(like XXXXXXXXXX which is 100px if font-size: 10px)