Bug 82143 - [Chromium] Add WebKit API for WebCore::TextFieldDecorator
Summary: [Chromium] Add WebKit API for WebCore::TextFieldDecorator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Other
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 82142 82572 82693
Blocks: 80479
  Show dependency treegraph
 
Reported: 2012-03-25 03:53 PDT by Kent Tamura
Modified: 2012-04-02 21:24 PDT (History)
7 users (show)

See Also:


Attachments
Patch (23.16 KB, patch)
2012-03-27 23:07 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (20.78 KB, patch)
2012-03-29 00:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (20.88 KB, patch)
2012-04-01 22:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (21.26 KB, patch)
2012-04-02 20:35 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-03-25 03:53:39 PDT
Add Chromium WebKit API for WebCore::TextFieldDecorator
Comment 1 Kent Tamura 2012-03-27 23:07:42 PDT
Created attachment 134222 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-27 23:10:23 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Hajime Morrita 2012-03-27 23:55:59 PDT
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.
Comment 4 Adam Barth 2012-03-27 23:56:17 PDT
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.
Comment 5 Dimitri Glazkov (Google) 2012-03-28 09:07:53 PDT
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.
Comment 6 Kent Tamura 2012-03-28 19:05:27 PDT
(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.
Comment 7 Kent Tamura 2012-03-29 00:26:55 PDT
Created attachment 134516 [details]
Patch 2

Use TextFieldDecorationElement::decorate
Comment 8 Dimitri Glazkov (Google) 2012-03-29 09:47:25 PDT
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.
Comment 9 Kent Tamura 2012-03-29 21:50:07 PDT
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.
Comment 10 Kent Tamura 2012-04-01 22:26:26 PDT
Created attachment 135025 [details]
Patch 3

Fix a comment, and make an argument const
Comment 11 Dimitri Glazkov (Google) 2012-04-02 09:34:20 PDT
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.
Comment 12 Kent Tamura 2012-04-02 20:35:10 PDT
Created attachment 135267 [details]
Patch for landing

Add a limit
Comment 13 Kent Tamura 2012-04-02 20:36:24 PDT
(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().
Comment 14 WebKit Review Bot 2012-04-02 21:24:54 PDT
Comment on attachment 135267 [details]
Patch for landing

Clearing flags on attachment: 135267

Committed r112984: <http://trac.webkit.org/changeset/112984>
Comment 15 WebKit Review Bot 2012-04-02 21:24:59 PDT
All reviewed patches have been landed.  Closing bug.