[webkit-reviews] review granted: [Bug 108168] Implement the -webkit-margin-collapse properties correct rendering : [Attachment 186898] Patch v3

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 108168: Implement the -webkit-margin-collapse properties correct rendering
https://bugs.webkit.org/show_bug.cgi?id=108168

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

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


r=me

> Source/WebCore/rendering/RenderBlock.cpp:6921
> +bool RenderBlock::mustDiscardMarginBeforeForChild(const RenderBox* child)
const

You should probably assert that the child does not need a layout here, since
you are asking for a RenderBlock bit that is set by the layout process.  Same
for all four of these methods.

> Source/WebCore/rendering/RenderBlock.cpp:6927
> +    if (child->isHorizontalWritingMode() == isHorizontalWritingMode())
> +	   return child->isRenderBlock() ?
toRenderBlock(child)->mustDiscardMarginAfter() :
(child->style()->marginAfterCollapse() == MDISCARD);
> +    return false;

This warrants commenting I think. It's pretty subtle that you just return false
for orthogonal blocks. Really the only reason we do so is because the
properties are only specified for "before" and "after". If we did implement
physical properties correctly (and this is something I could see us doing),
then you would need to check the correct physical side. For now this is
correct, though, but I think it warrants a comment.