[Webkit-unassigned] [Bug 86557] Allow WebTextFieldDecoratorClient to see applied decorations.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=86557


Dimitri Glazkov (Google) <dglazkov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #143145|review?                     |review-
               Flag|                            |




--- Comment #21 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2012-05-22 09:48:26 PST ---
(From update of attachment 143145)
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(CSSPropertyVisibility,
> +                                                                     CSSValueVisible);
> +    else
> +        unwrap<TextFieldDecorationElement>()->setInlineStyleProperty(CSSPropertyVisibility,
> +                                                                     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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.