Bug 95255 – Make RenderBox::computeInlineDirectionMargins const

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Make RenderBox::computeInlineDirectionMargins const

[Tony Chang]

Created attachment 161073 [details]
Patch

[Ojan Vafai]

Comment on attachment 161073 [details]
Patch

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.

[Tony Chang]

Created attachment 161099 [details]
Patch

[Tony Chang]

I did all the suggestions except those mentioned below.

(In reply to comment #2)
> (From update of attachment 161073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161073&action=review
> 
> > 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;
> ...
> }

That's not quite correct. There are 16 possible combinations, but only 8 are valid (there are 8 perpendicular cases). I wanted the ASSERT_NOT_REACHED() to make sure that this function is only used in the valid cases.

> > 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.

Generating doesn't save much since it's only testing perpendicular cases, not all 16 cases.

[Tony Chang]

Comment on attachment 161099 [details]
Patch

Looks like fast/writing-mode/inline-direction-positioning.html is failing on the ews bot.

[WebKit Review Bot]

Comment on attachment 161099 [details]
Patch

Attachment 161099 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13680449

New failing tests:
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scrolled.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe-composited.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-layout-change.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div.html
fast/loader/unload-form-post-about-blank.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe-scrolled.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-composited-outer.html
fullscreen/video-controls-timeline.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe.html
fast/canvas/webgl/shader-precision-format.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe-composited-scrolled-late-composite.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe-composited-scrolled-clipped.html
http/tests/xmlhttprequest/zero-length-response.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner-late-composite.html
fast/repaint/block-selection-gap-in-composited-layer.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-composited-scrolled.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe-composited-scrolled.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner.html
fast/frames/cached-frame-counter.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-composited-inner.html
fast/writing-mode/inline-direction-positioning.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scrolled-late-noncomposite.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-composited.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scrolled-late-composite.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-layout-change-2.html
fast/loader/local-CSS-from-local.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-composited-scroll-clip.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-iframe-composited-scrolled-late-noncomposite.html

[Ojan Vafai]

View in context: https://bugs.webkit.org/attachment.cgi?id=161099&action=review

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

How about changing this function to take the containingblock and the child itself then doing the following?:
ASSERT(containingBlock->isHorizontalWritingMode() != child->isHorizontalWritingMode());
WritingMode childWritingMode = child->style()->writingMode();
switch (containingBlock->style()->writingMode()) {
    case TopToBottomWritingMode:
        return childWritingMode != LeftToRightWritingMode;
    ...
}

I think that's cleaner. It makes the calling sites smaller and asserts that the writing modes are perpendicular instead of having a comment to that affect.

Not a big deal, I just find this really hard to read.

> Source/WebCore/rendering/RenderBox.cpp:2004
> +                bool hasInvertedDirection = shouldFlipBeforeAfterMargins(cb->style()->writingMode(), style()->writingMode());

s/hasInvertedDirection/shouldFlipBeforeAfter? Since these aren't actually directions.

[Tony Chang]

Created attachment 161355 [details]
Patch

[Tony Chang]

I forgot that we need to swap before/after if the containing block is rtl.  Added some tests cases for that too.

I also made use of a switch and renamed the local variable to shouldFlipBeforeAfter.

[WebKit Review Bot]

Comment on attachment 161355 [details]
Patch

Clearing flags on attachment: 161355

Committed r127157: <http://trac.webkit.org/changeset/127157>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.