[webkit-reviews] review granted: [Bug 89261] [CSS Exclusions] Floats should respect shape-inside on exclusions : [Attachment 179355] Updated Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted Bem Jones-Bey <bjonesbe at adobe.com>'s
request for review:
Bug 89261: [CSS Exclusions] Floats should respect shape-inside on exclusions
https://bugs.webkit.org/show_bug.cgi?id=89261

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=179355&action=review


r=me

> Source/WebCore/rendering/RenderBlock.cpp:3824
> +	   // The segment offsets are relative to the content box.

It's good this comment is here, but I do question why segments are using
content box coordinates. This is not something we do anywhere else in the code.
All other constructs, from positioned elements, to line boxes, to floats, etc.
are in border box coordinates, so why have we made segments different? I
strongly believe you should express segment positions using border box
coordinates instead (and shapes too if they are what led segments to be in
content box coordinates).

I'm not going to r- over this issue, but I think it's something you should
consider. Having exclusions be different than every other layout construct
doesn't seem ideal as far as other people coming into the code and trying to
understand it. You'll be putting clarifying comments like the one above all
over the place as you patch more spots in the code and find you have to do
border<->content conversion math in those spots also.