[webkit-reviews] review denied: [Bug 95255] Make RenderBox::computeInlineDirectionMargins const : [Attachment 161073] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Tony Chang <tony at chromium.org>'s
request for review:
Bug 95255: Make RenderBox::computeInlineDirectionMargins const
https://bugs.webkit.org/show_bug.cgi?id=95255

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=161073&action=review


> Source/WebCore/rendering/RenderBox.cpp:1718
> +	       cb->style()->isLeftToRightDirection() ==
style()->isLeftToRightDirection() ? marginValues.m_start : marginValues.m_end,
> +	       cb->style()->isLeftToRightDirection() ==
style()->isLeftToRightDirection() ? marginValues.m_end : marginValues.m_start);


This would be a lot more readable with a local bool, e.g. hasInvertedDirection?


> Source/WebCore/rendering/RenderBox.cpp:1961
> +static bool shouldFlipBeforeAfterMargins(WritingMode containingBlock,
WritingMode child)

I think these should be containingBlockWritingMode and childWritingMode. I know
it's verbose, but otherwise the code below is very confusing.

> Source/WebCore/rendering/RenderBox.cpp:1974
> +    if ((containingBlock == TopToBottomWritingMode && child ==
LeftToRightWritingMode)
> +	   || (containingBlock == BottomToTopWritingMode && child ==
LeftToRightWritingMode)
> +	   || (containingBlock == RightToLeftWritingMode && child ==
TopToBottomWritingMode)
> +	   || (containingBlock == LeftToRightWritingMode && child ==
TopToBottomWritingMode))
> +	   return false;
> +    if ((containingBlock == TopToBottomWritingMode && child ==
RightToLeftWritingMode)
> +	   || (containingBlock == BottomToTopWritingMode && child ==
RightToLeftWritingMode)
> +	   || (containingBlock == RightToLeftWritingMode && child ==
BottomToTopWritingMode)
> +	   || (containingBlock == LeftToRightWritingMode && child ==
BottomToTopWritingMode))
> +	   return true;
> +    ASSERT_NOT_REACHED(); // Writing modes should be perpendicular.
> +    return false;

Did you try writing this as a switch statement? I imagine it'd be much more
readable, e.g.

switch(containingBlockWritingMode) {
case TopToBottomWritingMode:
    return childWritingMode != LeftToRightWritingMode;
...
}

> Source/WebCore/rendering/RenderBox.cpp:2005
> +		       shouldFlipBeforeAfterMargins(cb->style()->writingMode(),
style()->writingMode()) ? marginValues.m_after : marginValues.m_before,
> +		       shouldFlipBeforeAfterMargins(cb->style()->writingMode(),
style()->writingMode()) ? marginValues.m_before : marginValues.m_after);

Ditto. Would be easier to read with a local bool.

> Source/WebCore/rendering/RenderBox.cpp:2060
> +		       shouldFlipBeforeAfterMargins(cb->style()->writingMode(),
style()->writingMode()) ? marginValues.m_after : marginValues.m_before,
> +		       shouldFlipBeforeAfterMargins(cb->style()->writingMode(),
style()->writingMode()) ? marginValues.m_before : marginValues.m_after);

Ditto

> Source/WebCore/rendering/RenderTable.cpp:266
> +	       cb->style()->isLeftToRightDirection() ==
style()->isLeftToRightDirection() ? marginValues.m_start : marginValues.m_end,
> +	       cb->style()->isLeftToRightDirection() ==
style()->isLeftToRightDirection() ? marginValues.m_end : marginValues.m_start);


Ditto

> LayoutTests/fast/block/margins-perpendicular-containing-block.html:15
> +.horizontal, .vertical {
> +    position: relative;
> +}
> +.horizontal div {
> +    margin-left: 10px;
> +    margin-right: 20px;
> +}
> +.vertical div {
> +    margin-top: 10px;
> +    margin-bottom: 20px;
> +}

Could you simplify this by always setting all four margin sides? That would
also give slightly better test coverage.
.container {
    position: relative;
    margin: 10px 20px 20px 10px;
}

> LayoutTests/fast/block/margins-perpendicular-containing-block.html:32
> +<div style="-webkit-writing-mode: horizontal-tb" class="horizontal">
> +<div data-offset-x=10 style="-webkit-writing-mode: vertical-rl">
> +    <tr><td>Hello</td></tr>
> +</div>
> +<div data-offset-x=10 style="-webkit-writing-mode: vertical-lr">
> +    <tr><td>Hello</td></tr>
> +</div>
> +</div>

This is fine. I'd rather see each case generated from script. Then it's easier
to see that each case is covered. But, meh, up to you.

> LayoutTests/fast/table/margins-perpendicular-containing-block.html:1
> +<!DOCTYPE html>

Ditto the comments on the other perpendicular-containing-block.html test.