[webkit-reviews] review denied: [Bug 50949] Add support for unicode-bidi:plaintext CSS property : [Attachment 88417] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 26 16:08:59 PDT 2011
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.
More information about the webkit-reviews
mailing list