Bug 32533 – Add support to set input element's suggestion value from auto-complete menu

• Status: UNCONFIRMED
• Component: WebCore Misc.

[Zelidrag Hornung]

AutoComplete pop-up menu needs to be able to set suggested value of HTTPInputElement. This will allow displaying proposed autocomplete value in Chromium as user will a) type value prefix and b) scroll through other autocomplete suggestions in the menu. 

This bug is Chromium-specific part of the issue originally described in https://bugs.webkit.org/show_bug.cgi?id=32261.

[Zelidrag Hornung]

Little correction to the original description of this bug:

This bug includes PopupMenu and Chromium-specific part of the issue originally describedin https://bugs.webkit.org/show_bug.cgi?id=32261.

[Zelidrag Hornung]

Created attachment 45006 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements

Added ability for PopupMenuClient to signal when menu selection changed and weather selected value should be accepted when popup menu closes. The CL also includes Chromium implementation of autocomplete that now also show matching suggestion in input elements without having their value exposed to JS.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45006 [details] without any errors.

[WebKit Review Bot]

Attachment 45006 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/145685

[Zelidrag Hornung]

Created attachment 45613 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

This patch includes changes for PopupMenuClient and Chromium-specific implementation of autocomplete suggestion mechanism.

[WebKit Review Bot]

Attachment 45613 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/AutocompletePopupMenuClient.cpp:64:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/chromium/src/AutocompletePopupMenuClient.cpp:190:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2

[Zelidrag Hornung]

Created attachment 45614 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

Fixed minor style issues from the previous patch.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45614 [details] without any errors.

[Dimitri Glazkov (Google)]

Comment on attachment 45614 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

Looks fine, except for nits below:

It looks like setInitialAutocompleteValue is only used in one place. Do we need it as a separate method?

> +void AutocompletePopupMenuClient::setInitialAutocompleteValue()
> +{
> +    if (!m_suggestions.size() || !m_textField->name().length())
> +        return;
> +    int newIndex = m_selectedIndex >= 0 ? m_selectedIndex : 0;
> +    WebCore::String suggestion = m_suggestions[newIndex];

No need for namespace prefix here.

> +    bool hasPreviousValue = m_lastFieldValues->contains(m_textField->name());
> +    WebCore::String prevValue;

Ditto.

> +          if (suggestion.length() > m_typedFieldValue.length()) {
> +              newSuggestion.append(suggestion.characters() + m_typedFieldValue.length(),
> +                  suggestion.length() - m_typedFieldValue.length());

Since we're dealing w/UTF16 here, is this still valid for weird characters? I am a UTF ignoramus, btw.

[Zelidrag Hornung]

Created attachment 45687 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

Fixed minor issues from Dimitry's comments:
- minor namespace issues, and
- made string concatenation little bit safer from UTF16 perspective - now I check for prefix match explicitly even though that probably could not have been an issue with extended UTF16 glyphs since both sides of the comparison come from typed values.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45687 [details] without any errors.

[Dimitri Glazkov (Google)]

Comment on attachment 45687 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

r=me.

[WebKit Commit Bot]

Comment on attachment 45687 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

Clearing flags on attachment: 45687

Committed r52673: <http://trac.webkit.org/changeset/52673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

Reopening; I'm backing this out due to it causing a number of regressions.

[Peter Kasting]

Created attachment 47224 [details]
Backout patch

Backout patch.  Posting so I can get the EWS bots to run it, since I want to make sure I didn't break Qt.

[Peter Kasting]

Created attachment 47225 [details]
Backout patch, for real

Whoops, that other patch had other junk from my checkout in it.

[Peter Kasting]

Comment on attachment 47225 [details]
Backout patch, for real

Not sure the bots will try something without r?, so I went ahead and landed this as 53716, and will watch the builders.

[Adam Barth]

> Not sure the bots will try something without r?

The bots only try things that are up for review.  Also, they're not good at reviewing build fixes because they don't test patches when they can't build (otherwise, they'd reject a lot of stuff).