[webkit-reviews] review denied: [Bug 82143] [Chromium] Add WebKit API for WebCore::TextFieldDecorator : [Attachment 134516] Patch 2

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 82143: [Chromium] Add WebKit API for WebCore::TextFieldDecorator
https://bugs.webkit.org/show_bug.cgi?id=82143

Attachment 134516: Patch 2
https://bugs.webkit.org/attachment.cgi?id=134516&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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.