[webkit-reviews] review denied: [Bug 50949] Add support for unicode-bidi:plaintext CSS property : [Attachment 98211] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 50949: Add support for unicode-bidi:plaintext CSS property
https://bugs.webkit.org/show_bug.cgi?id=50949

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review

r- for the cr-linux failure.  Happy to look again after your replies.

> Source/WebCore/html/HTMLElement.cpp:129
> +static inline int unicodeBidiAttributeForDirAuto(HTMLElement* element)

So sad that these are ints, and not an enum.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:967
> +	   bool isNewUBAParagraph = lineInfo.previousLineBrokeCleanly();

I wonder if this shouldn't be an accessor on lineInfo.	So that at some point
later we might distinguish between nextLineIsNewUBAParagraph and
previousLineBrokeCleanly (since I think they're used for separate things a
times).

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:991
> +	       if (isNewUBAParagraph && style()->unicodeBidi() == Plaintext &&
!resolver.context()->parent()) {
> +		   TextDirection direction = style()->direction();
> +		   determineParagraphDirection(direction, resolver.position());

> +		   resolver.setStatus(BidiStatus(direction,
style()->unicodeBidi() == Override));
> +	       }

Why is this needed here?  I would think you would only need this in
determinedStart... below.