Bug 32533

Summary: Add support to set input element's suggestion value from auto-complete menu
Product: WebKit Reporter: Zelidrag Hornung <zelidrag>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: abarth, beng, commit-queue, darin, dglazkov, glen, pkasting, webkit.review.bot
Priority: P2 Keywords: GoogleBug
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 32261    
Bug Blocks:    
Attachments:
Description Flags
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements
none
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.
none
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.
dglazkov: review-, dglazkov: commit-queue-
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.
none
Backout patch
none
Backout patch, for real none

Description Zelidrag Hornung 2009-12-14 16:47:01 PST
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.
Comment 1 Zelidrag Hornung 2009-12-16 11:06:07 PST
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.
Comment 2 Zelidrag Hornung 2009-12-16 12:39:44 PST
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.
Comment 3 WebKit Review Bot 2009-12-16 12:43:27 PST
style-queue ran check-webkit-style on attachment 45006 [details] without any errors.
Comment 4 WebKit Review Bot 2009-12-26 03:23:17 PST
Attachment 45006 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/145685
Comment 5 Zelidrag Hornung 2009-12-29 10:22:05 PST
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.
Comment 6 WebKit Review Bot 2009-12-29 10:26:08 PST
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
Comment 7 Zelidrag Hornung 2009-12-29 10:37:16 PST
Created attachment 45614 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

Fixed minor style issues from the previous patch.
Comment 8 WebKit Review Bot 2009-12-29 10:42:09 PST
style-queue ran check-webkit-style on attachment 45614 [details] without any errors.
Comment 9 Dimitri Glazkov (Google) 2009-12-30 12:17:44 PST
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.
Comment 10 Zelidrag Hornung 2009-12-30 14:51:06 PST
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.
Comment 11 WebKit Review Bot 2009-12-30 14:55:38 PST
style-queue ran check-webkit-style on attachment 45687 [details] without any errors.
Comment 12 Dimitri Glazkov (Google) 2009-12-30 14:59:53 PST
Comment on attachment 45687 [details]
PopupMenuClient changes needed for autocomplete suggestions displaying in input elements.

r=me.
Comment 13 WebKit Commit Bot 2009-12-30 15:09:09 PST
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>
Comment 14 WebKit Commit Bot 2009-12-30 15:09:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Peter Kasting 2010-01-22 13:43:19 PST
Reopening; I'm backing this out due to it causing a number of regressions.
Comment 16 Peter Kasting 2010-01-22 13:44:37 PST
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.
Comment 17 Peter Kasting 2010-01-22 13:54:09 PST
Created attachment 47225 [details]
Backout patch, for real

Whoops, that other patch had other junk from my checkout in it.
Comment 18 Peter Kasting 2010-01-22 14:06:28 PST
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.
Comment 19 Adam Barth 2010-01-22 23:12:44 PST
> 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).