Bug 41236 – Autocomplete entries are missing when AutoFill entries are shown in the suggestions popup on Chrome

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[David Holloway]

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.

[David Holloway]

Created attachment 59819 [details]
Proposed patch.

Proposed patch.

[WebKit Review Bot]

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.

[David Holloway]

Created attachment 59820 [details]
Proposed patch, fixed style issues.

[David Holloway]

Reviewed on the Chrome side by jhawkins and (at design level by) jcivelli.

[Darin Fisher (:fishd, Google)]

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.

[David Holloway]

Created attachment 60132 [details]
Proposed patch, addresses review comments.

[David Holloway]

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

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 60132 [details]
Proposed patch, addresses review comments.

OK, r=me with agreement to do the operator cleanup in a separate patch.

[Jay Civelli]

This looks good to me with regards to my upcoming changes to password and field autocomplete.

[WebKit Commit Bot]

Comment on attachment 60132 [details]
Proposed patch, addresses review comments.

Clearing flags on attachment: 60132

Committed r62272: <http://trac.webkit.org/changeset/62272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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