support -webkit-flex-flow: horizontal-rtl
Created attachment 106029 [details] Patch
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?
Created attachment 106076 [details] Patch
(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.
Comment on attachment 106076 [details] Patch LGTM, but I defer to Hyatt.
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.