[webkit-reviews] review denied: [Bug 67437] support -webkit-flex-flow: horizontal-rtl : [Attachment 106029] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 67437: support -webkit-flex-flow: horizontal-rtl
https://bugs.webkit.org/show_bug.cgi?id=67437

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

------- Additional Comments from Tony Chang <tony at chromium.org>
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?