[webkit-reviews] review denied: [Bug 86557] Allow WebTextFieldDecoratorClient to see applied decorations. : [Attachment 143145] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 22 09:49:21 PDT 2012
Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Garrett Casto
<gcasto at chromium.org>'s request for review:
Bug 86557: Allow WebTextFieldDecoratorClient to see applied decorations.
https://bugs.webkit.org/show_bug.cgi?id=86557
Attachment 143145: Patch
https://bugs.webkit.org/attachment.cgi?id=143145&action=review
------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143145&action=review
I think this is good. I would really like for Kent-san to look over the next
iteration of the patch, since you and I came up with this idea together.
Another pair of eyes wouldn't hurt :)
> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:69
> +TextFieldDecorationElement*
TextFieldDecorationElement::fromShadowRoot(ShadowRoot* shadowRoot)
Can you file a bug about making TextFieldDecorationElement _the_ ShadowRoot and
add a FIXME comment referencing it?
// FIXME: After fixing http://bugs.webkit.org/...., this should be a simple
cast.
> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:108
> + box->setInlineStyleProperty(CSSPropertyVisibility, CSSValueHidden);
I don't mind if you expose a flag in TextFieldDecorator that allows controlling
whether the element is hidden or visible by default.
> Source/WebKit/chromium/public/WebTextFieldDecorationElement.h:54
> + // Make visible iff true.
Don't need this comment here.
> Source/WebKit/chromium/src/TextFieldDecoratorImpl.h:46
> + WebTextFieldDecoratorClient* decoratorClient();
Instead of exposing a client here, can we instead let it do the comparison
inside:
WebTextfieldDecoratorClient::isClientFor(WebCore::TextFieldDecorator*);
This way, the casting to TextFieldDecoratorImpl becomes an implementation
detail, not an assumption in WebInputElement.
> Source/WebKit/chromium/src/WebInputElement.cpp:221
> + ShadowRoot* shadowRoot =
unwrap<HTMLInputElement>()->shadow()->youngestShadowRoot();
I think the code just landed that lets you use Node::youngestShadowRoot()
> Source/WebKit/chromium/src/WebInputElement.cpp:228
> + shadowRoot->olderShadowRoot();
shadowRoot = shadowRoot->olderShadowRoot(); !!!
> Source/WebKit/chromium/src/WebTextFieldDecorationElement.cpp:50
> + if (visible)
> +
unwrap<TextFieldDecorationElement>()->setInlineStyleProperty(CSSPropertyVisibil
ity,
> +
CSSValueVisible);
> + else
> +
unwrap<TextFieldDecorationElement>()->setInlineStyleProperty(CSSPropertyVisibil
ity,
> +
CSSValueHidden);
I think we can just use WebElement::setAttribute("style", "display:none|block")
instead. This way, the patch will be smaller and we're not exposing any new
WebCore functionality. WDYT?
More information about the webkit-reviews
mailing list