[webkit-reviews] review granted: [Bug 218087] [css-logical] Add logical values for 'float' and 'clear' : [Attachment 425028] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Nguyen (:ntim) <ntim at apple.com>'s
request for review:
Bug 218087: [css-logical] Add logical values for 'float' and 'clear'
https://bugs.webkit.org/show_bug.cgi?id=218087

Attachment 425028: Patch

https://bugs.webkit.org/attachment.cgi?id=425028&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 425028
  --> https://bugs.webkit.org/attachment.cgi?id=425028
Patch

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

> Source/WebCore/layout/layouttree/LayoutBox.cpp:198
> +    bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL;

Given we only need this for start/end, is it OK that we are computing it
unconditionally? Will the extra work have a measurable performance impact?

> Source/WebCore/layout/layouttree/LayoutBox.cpp:212
> +    bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL;

Ditto.

> Source/WebCore/rendering/RenderObject.cpp:2103
> +    bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL;

Is there a way to avoid having the code be so repetitive? This is the third
almost-identical function of four.

> LayoutTests/ChangeLog:14
> +	   *
imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-1.ht
ml: Added.
> +	   *
imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-2.ht
ml: Added.
> +	   *
imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-3.ht
ml: Added.
> +	   *
imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-4.ht
ml: Added.

ChangeLog does not mention TestExpectations, nor explain the changes in that
file.

> LayoutTests/TestExpectations:2947
> +webkit.org/b/224104
imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-reft
est.html [ Skip ]

I understand that this test fails. But I don’t understand why we need to skip
this test instead of expecting a failure now. Why is that?