Bug 82143

Created attachment 134222 [details] Patch

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 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 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 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.

(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.

Created attachment 134516 [details] Patch 2 Use TextFieldDecorationElement::decorate

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 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.

Created attachment 135025 [details] Patch 3 Fix a comment, and make an argument const

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.

Created attachment 135267 [details] Patch for landing Add a limit

(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 on attachment 135267 [details] Patch for landing Clearing flags on attachment: 135267 Committed r112984: <http://trac.webkit.org/changeset/112984>

All reviewed patches have been landed. Closing bug.