Bug 82143 – [Chromium] Add WebKit API for WebCore::TextFieldDecorator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kent Tamura]

Add Chromium WebKit API for WebCore::TextFieldDecorator

[Kent Tamura]

Created attachment 134222 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Hajime Morrita]

Comment on attachment 134222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134222&action=review

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1009
> +

Shadow DOM part looks OK.
By the way, it would be great if we have some DRT implementation to demonstrate this.
We could have a DRT specific decorator which activates for input elements which has specific id or class.

[Adam Barth]

Comment on attachment 134222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134222&action=review

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1016
> +    RefPtr<ShadowRoot> newRoot = ShadowRoot::create(input, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION);
> +    RefPtr<HTMLDivElement> box = HTMLDivElement::create(input->document());
> +    newRoot->appendChild(box);
> +    box->setInlineStyleProperty(CSSPropertyDisplay, CSSValueWebkitBox);
> +    box->setInlineStyleProperty(CSSPropertyWebkitBoxAlign, CSSValueCenter);
> +    input->containerElement()->setInlineStyleProperty(CSSPropertyWebkitBoxFlex, 1.0, CSSPrimitiveValue::CSS_NUMBER);
> +    box->appendChild(HTMLShadowElement::create(HTMLNames::shadowTag, input->document()));

Drive-by comment: Should this work be done by WebCore?  This seems very detail-oriented for the WebKit layer.

[Dimitri Glazkov (Google)]

Comment on attachment 134222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134222&action=review

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1016
>> +    box->appendChild(HTMLShadowElement::create(HTMLNames::shadowTag, input->document()));
> 
> Drive-by comment: Should this work be done by WebCore?  This seems very detail-oriented for the WebKit layer.

You know, Adam might be onto something here. This does look like a general pattern.

[Kent Tamura]

(In reply to comment #5)
> (From update of attachment 134222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134222&action=review
> 
> >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1016
> >> +    box->appendChild(HTMLShadowElement::create(HTMLNames::shadowTag, input->document()));
> > 
> > Drive-by comment: Should this work be done by WebCore?  This seems very detail-oriented for the WebKit layer.
> 
> You know, Adam might be onto something here. This does look like a general pattern.

ok, I'll add a member function to TextFieldDecorationElement for this.

[Kent Tamura]

Created attachment 134516 [details]
Patch 2

Use TextFieldDecorationElement::decorate

[Dimitri Glazkov (Google)]

Comment on attachment 134516 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=134516&action=review

This looks very exciting. I am sorry to report that I have a few concerns:

> Source/WebKit/chromium/public/WebTextFieldDecoratorClient.h:61
> +    // This is called when the input element looses its renderer.

looses -> loses.

> Source/WebKit/chromium/public/WebTextFieldDecoratorClient.h:62
> +    virtual void willDetach(WebInputElement&) = 0;

I realize I reviewed the change adding this to TextDecorationElement, but now I am not sure this is a good idea. The concept of detaching is intrinsic to WebCore, and it would be extremely easy to create gnarly bugs in this callsite, when written by someone who's not as familiar with WebCore (for instance, WebKit API consumer). Does this have to be a synchronous call? How can we avoid exposing this hook?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:998
> +    const Vector<OwnPtr<TextFieldDecorator> >& decorators = m_webView->textFieldDecorators();
> +    for (unsigned i = 0; i < decorators.size(); ++i) {
> +        if (decorators[i]->willAddDecorationTo(input))
> +            return true;
> +    }
> +    return false;

This is O(NxM) in the worst case, right? Where N is number of inputs on the page and M is the number of decorators. Seems iffy from perf perspective.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1006
> +        if (!decorators[i]->willAddDecorationTo(input))

And this repeats the check again, even though willAddTextFieldDecorationsTo has just been called earlier in TextFieldInputType::createShadowSubtree.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1009
> +        RefPtr<TextFieldDecorationElement> decoration = TextFieldDecorationElement::create(input->document(), decorators[i].get());
> +        decoration->decorate(input);

This is neat :)

However, I just realized that the TextFieldDecorationElement::decorate creates a shadow DOM subtree that is never destroyed. This means that changing the type of the input will leave the icon in place.

[Kent Tamura]

Comment on attachment 134516 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=134516&action=review

>> Source/WebKit/chromium/public/WebTextFieldDecoratorClient.h:62
>> +    virtual void willDetach(WebInputElement&) = 0;
> 
> I realize I reviewed the change adding this to TextDecorationElement, but now I am not sure this is a good idea. The concept of detaching is intrinsic to WebCore, and it would be extremely easy to create gnarly bugs in this callsite, when written by someone who's not as familiar with WebCore (for instance, WebKit API consumer). Does this have to be a synchronous call? How can we avoid exposing this hook?

It's ok to make this asynchronous.  Changing "WebInputElement&" to "const WebInputElement&" solves issues?

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:998
>> +    return false;
> 
> This is O(NxM) in the worst case, right? Where N is number of inputs on the page and M is the number of decorators. Seems iffy from perf perspective.

Right.
I'm not worry about this because a browser doesn't add a lot of decorators and web page authors can't add decorations.
However, we had better add a comment to willAddDecorationTo() like "This function should not take much time ..."

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1006
>> +        if (!decorators[i]->willAddDecorationTo(input))
> 
> And this repeats the check again, even though willAddTextFieldDecorationsTo has just been called earlier in TextFieldInputType::createShadowSubtree.

I think calling it twice is unavoidable.
We need to check if any decorations will be added before constructing the main shadow tree.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1009
>> +        decoration->decorate(input);
> 
> This is neat :)
> 
> However, I just realized that the TextFieldDecorationElement::decorate creates a shadow DOM subtree that is never destroyed. This means that changing the type of the input will leave the icon in place.

Ah, right.  I'll discuss this with Morita-san and Kawanaka-san.

[Kent Tamura]

Created attachment 135025 [details]
Patch 3

Fix a comment, and make an argument const

[Dimitri Glazkov (Google)]

Comment on attachment 135025 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=135025&action=review

> Source/WebKit/chromium/src/WebViewImpl.h:595
> +    Vector<OwnPtr<WebCore::TextFieldDecorator> > m_textFieldDecorators;

I wonder if we could put some sort of an assertion for going over a certain number of decorators. Here's my concern: over time, the consumers of this API may find it useful and start adding more decorators -- and unexpectedly see performance degradation due to NxM complexity of the plumbing. Perhaps if we somehow alerted future us that things are going awry, we would be less surprised about its effects.

[Kent Tamura]

Created attachment 135267 [details]
Patch for landing

Add a limit

[Kent Tamura]

(In reply to comment #11)
> I wonder if we could put some sort of an assertion for going over a certain number of decorators. Here's my concern: over time, the consumers of this API may find it useful and start adding more decorators -- and unexpectedly see performance degradation due to NxM complexity of the plumbing. Perhaps if we somehow alerted future us that things are going awry, we would be less surprised about its effects.

Ok, I have added a limitation of the number of decorators in WebViewImpl::addTextDecoratorClient().

[WebKit Review Bot]

Comment on attachment 135267 [details]
Patch for landing

Clearing flags on attachment: 135267

Committed r112984: <http://trac.webkit.org/changeset/112984>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.