Bug 41236 - Autocomplete entries are missing when AutoFill entries are shown in the suggestions popup on Chrome
Summary: Autocomplete entries are missing when AutoFill entries are shown in the sugge...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-25 17:06 PDT by David Holloway
Modified: 2010-12-13 13:51 PST (History)
7 users (show)

See Also:


Attachments
Example form. (3.44 KB, text/html)
2010-06-25 17:06 PDT, David Holloway
no flags Details
Proposed patch. (55.98 KB, patch)
2010-06-25 17:51 PDT, David Holloway
no flags Details | Formatted Diff | Diff
Proposed patch, fixed style issues. (55.99 KB, patch)
2010-06-25 18:03 PDT, David Holloway
fishd: review-
Details | Formatted Diff | Diff
Proposed patch, addresses review comments. (56.01 KB, patch)
2010-06-30 10:39 PDT, David Holloway
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Holloway 2010-06-25 17:06:14 PDT
Created attachment 59812 [details]
Example form.

Overview: On Chrome, when user enters AutoFill data (in Preferences) and triggers the display of the suggestion popup in a form field their Autocomplete suggestions are not displayed.

Steps to Reproduce:

1) Launch Chrome.

2) Open a page with a form containing AutoFillable fields.  (See attached form.html file).

3) Type the name "Jay" into the "First Name" field of the form and click the send button.

4) Double-click the "First Name" field, and notice "Jay" appears in the popup.

5) Open Preferences, Open AutoFill settings, and enter a new Address with "David" as the name in the address.

6.)  Double-click the "First Name" field, and notice "David" appears in the popup, but not "Jay".

Actual Results: The suggestion popup does not contained the combined "David" and "Jay" suggestions, only "David".

Expected Results: The suggestions popup should contain "David" followed by "Jay".  That is, the combined results of AutoFill and Autocomplete.
Comment 1 David Holloway 2010-06-25 17:51:08 PDT
Created attachment 59819 [details]
Proposed patch.

Proposed patch.
Comment 2 WebKit Review Bot 2010-06-25 17:53:07 PDT
Attachment 59819 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1914:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/src/AutoFillPopupMenuClient.cpp:98:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/src/AutoFillPopupMenuClient.cpp:111:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebKit/chromium/src/AutoFillPopupMenuClient.cpp:112:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 9 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 David Holloway 2010-06-25 18:03:26 PDT
Created attachment 59820 [details]
Proposed patch, fixed style issues.
Comment 4 David Holloway 2010-06-25 18:07:09 PDT
Reviewed on the Chrome side by jhawkins and (at design level by) jcivelli.
Comment 5 Darin Fisher (:fishd, Google) 2010-06-30 09:04:35 PDT
Comment on attachment 59820 [details]
Proposed patch, fixed style issues.

WebKit/chromium/public/WebFrame.h:490
 +          WebInputElement) = 0;
please pass a WebInputElement using a const ref.  |const WebInputElement&|
WebKit/chromium/public/WebFrame.h:489
 +      virtual void notifiyPasswordListenerOfAutocomplete(
elsewhere you seem to be converging on the term autofill, but here
you still use the term autocomplete.  should this use autofill?

WebKit/chromium/public/WebView.h:243
 +          int separatorIndex) = 0;
was this parameter just misnamed before?  defaultSuggestionIndex
and separatorIndex seem like very different things.

WebKit/chromium/src/WebFrameImpl.cpp:1967
 +      RefPtr<HTMLInputElement> element = inputElement.operator PassRefPtr<HTMLInputElement>();
I don't think we should be invoking an operator this way.  Can't you
just write it like this:

  RefPtr<HTMLInputElement> element(inputElement);

That should cause inputElement to be implicitly converted to
PassRefPtr<HTMLInputElement> to fit the RefPtr constructor.

Overall, looks fine to me.  Please make sure that Jay has
reviewed this patch as well.
Comment 6 David Holloway 2010-06-30 10:39:55 PDT
Created attachment 60132 [details]
Proposed patch, addresses review comments.
Comment 7 David Holloway 2010-06-30 10:47:51 PDT
(In reply to comment #5)
> (From update of attachment 59820 [details])
> WebKit/chromium/public/WebFrame.h:490
>  +          WebInputElement) = 0;
> please pass a WebInputElement using a const ref.  |const WebInputElement&|

Done.

> WebKit/chromium/public/WebFrame.h:489
>  +      virtual void notifiyPasswordListenerOfAutocomplete(
> elsewhere you seem to be converging on the term autofill, but here
> you still use the term autocomplete.  should this use autofill?

It is appropriate to keep this named as Autocomplete.  The distinction remains for certain types of handling, this is one.

> 
> WebKit/chromium/public/WebView.h:243
>  +          int separatorIndex) = 0;
> was this parameter just misnamed before?  defaultSuggestionIndex
> and separatorIndex seem like very different things.

Yes, defaultSuggestionIndex was no longer used by Autocomplete and had been appropriated by Autofill to indicate separatorIndex.  I renamed to reflect actual use in the combined implementation.

> 
> WebKit/chromium/src/WebFrameImpl.cpp:1967
>  +      RefPtr<HTMLInputElement> element = inputElement.operator PassRefPtr<HTMLInputElement>();
> I don't think we should be invoking an operator this way.  Can't you
> just write it like this:
> 
>   RefPtr<HTMLInputElement> element(inputElement);
> 
> That should cause inputElement to be implicitly converted to
> PassRefPtr<HTMLInputElement> to fit the RefPtr constructor.
> 

The RefPtr constructor does not allow for implicit construction from a WebInputElement.  So that doesn't work.  The syntax:

 RefPtr<HTMLInputElement> element = static_cast<PassRefPtr<HTMLInputElement> >(inputElement);

seems a bit cleaner to me.  But I left it with explicit call to cast operator to match local style at line 1963.

> Overall, looks fine to me.  Please make sure that Jay has
> reviewed this patch as well.

I've emailed Jay with link to proposed changes on Chrome side as well to give sense of overall direction here.
Comment 8 Darin Fisher (:fishd, Google) 2010-06-30 11:08:21 PDT
(In reply to comment #7)
> > WebKit/chromium/src/WebFrameImpl.cpp:1967
> >  +      RefPtr<HTMLInputElement> element = inputElement.operator PassRefPtr<HTMLInputElement>();
> > I don't think we should be invoking an operator this way.  Can't you
> > just write it like this:
> > 
> >   RefPtr<HTMLInputElement> element(inputElement);
> > 
> > That should cause inputElement to be implicitly converted to
> > PassRefPtr<HTMLInputElement> to fit the RefPtr constructor.
> > 
> 
> The RefPtr constructor does not allow for implicit construction from a WebInputElement.  So that doesn't work.  The syntax:
> 
>  RefPtr<HTMLInputElement> element = static_cast<PassRefPtr<HTMLInputElement> >(inputElement);
> 
> seems a bit cleaner to me.  But I left it with explicit call to cast operator to match local style at line 1963.

I see.  I'm sad to see that others have been calling the operator explicitly.
That is really not how operators are intended to be used!!

It seems like we should make WebNode's unwrap and constUnwrap methods public.
Comment 9 Darin Fisher (:fishd, Google) 2010-06-30 11:11:07 PDT
Comment on attachment 60132 [details]
Proposed patch, addresses review comments.

OK, r=me with agreement to do the operator cleanup in a separate patch.
Comment 10 Jay Civelli 2010-06-30 14:43:07 PDT
This looks good to me with regards to my upcoming changes to password and field autocomplete.
Comment 11 WebKit Commit Bot 2010-07-01 10:15:07 PDT
Comment on attachment 60132 [details]
Proposed patch, addresses review comments.

Clearing flags on attachment: 60132

Committed r62272: <http://trac.webkit.org/changeset/62272>
Comment 12 WebKit Commit Bot 2010-07-01 10:15:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2010-12-13 13:51:38 PST
http://trac.webkit.org/changeset/73940 might have broken SnowLeopard Intel Release (Tests) and GTK Linux 64-bit Debug
The following tests are not passing:
editing/selection/extend-to-line-boundary.html