[webkit-reviews] review denied: [Bug 50949] Add support for unicode-bidi:plaintext CSS property : [Attachment 88417] 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 88417: Patch
https://bugs.webkit.org/attachment.cgi?id=88417&action=review

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

I'm concerned this is not the only place you want this code.  Otherwise seems
fine.

> Source/WebCore/html/HTMLElement.cpp:170
> +	   int bidiAttribute;
> +	   if (hasLocalName(bdoTag))
> +	       bidiAttribute = CSSValueBidiOverride;
> +	   else if (hasLocalName(preTag) || hasLocalName(textareaTag))
> +	       bidiAttribute = CSSValueWebkitPlaintext;
> +	   else
> +	       bidiAttribute = CSSValueEmbed;
> +	   addCSSProperty(attr, CSSPropertyUnicodeBidi, bidiAttribute);

This is a super-weird place for this code!  I guess since I we have eto leave
his here we should abstract this into a unicodeBidFromDirAuto() helper.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:87
> +static void determineParagraphDirection(Direction& dir, InlineIterator iter)

> +{

So this is basically "first strong direction in paragarph"?  I'm not sure where
this logic goes.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1266
> +	   if (style()->unicodeBidi() == Plaintext)
> +	       determineParagraphDirection(direction, InlineIterator(this,
bidiFirst(this, 0), pos));

I think we're going to want to start passing unicodeBidi() to BidiContexts :(

Are we sure this is the only place this needs to be done?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1270
>	   resolver.setLastStrongDir(direction);
>	   resolver.setLastDir(direction);
>	   resolver.setEorDir(direction);
> -	   resolver.setContext(BidiContext::create(ltr ? 0 : 1, direction,
style()->unicodeBidi() == Override, FromStyleOrDOM));
> +	   resolver.setContext(BidiContext::create(direction == LeftToRight ? 0
: 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM));

I think I have an outstanding conflicting patch here.