[Webkit-unassigned] [Bug 41236] Autocomplete entries are missing when AutoFill entries are shown in the suggestions popup on Chrome

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41236





--- Comment #7 from David Holloway <dhollowa at chromium.org>  2010-06-30 10:47:51 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.