Bug 67437 – support -webkit-flex-flow: horizontal-rtl

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

[Ojan Vafai]

support -webkit-flex-flow: horizontal-rtl

[Ojan Vafai]

Created attachment 106029 [details]
Patch

[Tony Chang]

Comment on attachment 106029 [details]
Patch

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

Just some minor style nits.

> LayoutTests/ChangeLog:10
> +        * platform/mac/Skipped:

Nit: This file doesn't appear to have changed.

> LayoutTests/css3/flexbox/right-to-left.html:36
> +<div class="horizontal-box">
> +  <div data-offset-x="400" style="width: -webkit-flex(1)"></div>

Can you add a test with different flex values?  Also, some tests with horizontal margins or paddings on the flex items?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:257
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return FlowLeftToRight;

I think we can remove the default case since you enumerated all the cases.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:261
> +LayoutUnit RenderFlexibleBox::startOffset()

Maybe add MainAxis to this function name?

[Ojan Vafai]

Created attachment 106076 [details]
Patch

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 106029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106029&action=review
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:257
> > +    default:
> > +        ASSERT_NOT_REACHED();
> > +        return FlowLeftToRight;
> 
> I think we can remove the default case since you enumerated all the cases.

Compiler's not smart enough to realize it isn't lacking a return statement. In either case, I moved the assert and return statement out of the switch. I think that'll be better as this function expands to include the other cases.

[Tony Chang]

Comment on attachment 106076 [details]
Patch

LGTM, but I defer to Hyatt.

[Ojan Vafai]

Comment on attachment 106076 [details]
Patch

As per discussion with Hyatt on IRC, all this code needs to be reworked to operate in logical, not physical coordiantes.