Implementation of the UI part of the autofill

This CL adds the autofill UI in forms.
When the user types text in a text field in a form, the renderer queries the browser for suggestion based on the entered text and displays the suggestions in a popup.

Listeners are set on the form text field in a similar fashion than for password save.

The popup showing the suggestion uses the same mechanism as the select popup. Note that a difference between the select and the autofill popup is that the autofill should not take focus, so the page still has focus and the user can still type in while it shows.
The creation of the render widget was modified for that purpose so we can specify the popup should not be focused when shown.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=4804

[jcampan, 2008-10-29 19:35:59]

Tim, could you look at:
autocomplete_input_listener.h
password_autocomplete_listener.cc
webframeloaderclient_impl.[h,cc]
form_autocomplete_listener.[h, cc]
glue.vcproj
base/string_util.]h,cc]
base/string_util_unittest.cc

Matt, could you look at the rest (sorry for the long CL, you seemed like a good
reviewer since you worked on the select popup):
webview.h
webview_delegate.h
webview_impl.[h, cc]
webwidget_impl.[h, cc]
port.vcproj
WidgetClientChromium.h
render_messages_internal.h
render_widget.[h,cc]
render_view.[h,cc]
render_widget_unittest.cc
render_view_host.[h,cc]
render_widget_helper.[h,cc]
render_widget_host_view_win.[h,cc]
resource_message_filter.[h,cc]
web_contents_view.[h,cc]
web_contents_view_win.[h,cc]
(it is a lot of files but most of the changes are small, they are caused by the
new focus_on_show parameter for RenderWidgets).

[Matt Perry, 2008-10-29 21:41:59]

Looks good for the most part.  I'm surprised the PopupMenu code didn't need more
changes to work.

I'm not too happy with exposing PopupContainer to external users since its more
of an implementation detail, but it does seem like the least evil alternative.

http://codereview.chromium.org/8885/diff/76/97
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/8885/diff/76/97#newcode1220
Line 1220: /*
nit: replace the comment with an #ifdef TEST_AUTOFILL or something of the sort,
to make it clear why it's there.

http://codereview.chromium.org/8885/diff/76/85
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/8885/diff/76/85#newcode902
Line 902: HideAutoCompletePopup();
Could we test if the popup has focus, and only hide it if it doesn't?  That
might solve this problem, and we could hide on the page losing focus.

http://codereview.chromium.org/8885/diff/76/85#newcode1485
Line 1485: HideAutoCompletePopup();
This seems strange... for every set of suggestions we get back, we hide and
reshow the popup.  Does that result in any flickering?

http://codereview.chromium.org/8885/diff/76/77
File webkit/port/platform/chromium/PopupMenuChromium.h (right):

http://codereview.chromium.org/8885/diff/76/77#newcode23
Line 23: // NOTE: this class is exposed so it can be instanciated direcly for
the
nit: instantiated

http://codereview.chromium.org/8885/diff/76/77#newcode36
Line 36: static void show(PopupContainer* popup, const IntRect& r,
Can't this be turned into a non-static method, since its first parameter is a
PopupContainer*?

[tim (not reviewing), 2008-10-29 22:51:25]

mostly nits/suggestions.
can't wait to try this out!

http://codereview.chromium.org/8885/diff/76/94
File base/string_util.cc (right):

http://codereview.chromium.org/8885/diff/76/94#newcode668
Line 668: if (search.size() > str.size())
this is really regardless of case sensitivity

http://codereview.chromium.org/8885/diff/76/94#newcode671
Line 671: CaseInsensitiveCompare<wchar_t>());
nit- looks mis-indented

http://codereview.chromium.org/8885/diff/76/92
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/8885/diff/76/92#newcode1240
Line 1240: EXPECT_EQ(true, StartsWith(L"java", L"", true));
suggestion- add a test for 
EXPECT_FALSE(StartsWith(L"JavaScript:url", L"javascript", true))

http://codereview.chromium.org/8885/diff/76/115
File webkit/glue/form_autocomplete_listener.cc (right):

http://codereview.chromium.org/8885/diff/76/115#newcode35
Line 35: }
this could go in the header

http://codereview.chromium.org/8885/diff/76/115#newcode39
Line 39: // The input_element_ pointer is simply used as an id for the node.
I think there is a somewhat subtle fact that the input_element_ pointer won't
change because it's referenced by the superclass due to wrapping it in an
HTMLInputDelegate, and it only drops the reference when this
FormAutocompleteListener gets deleted. You could remove the member from this
class and use edit_delegate() (returns a pointer to the element that will be
autofilled), but it might make the most sense to change the member type to an
HTMLInputDelegate (sticking with the AutocompleteInputListener style which is to
abstain from explicitly needing WebCore types, to simplify testing). At the very
least you'd probably want to Ref/Deref the HtmlInputElement yourself.

http://codereview.chromium.org/8885/diff/76/83
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/8885/diff/76/83#newcode55
Line 55: #include "webkit/glue/webframeloaderclient_impl.h"
doesn't this go on top?

http://codereview.chromium.org/8885/diff/76/83#newcode374
Line 374: // Now let's register any text input.  Note that at some point the
nit- register for any text input

http://codereview.chromium.org/8885/diff/76/83#newcode375
Line 375: // password and form autofill should be merged.
hm.. do we want any other form of tracking for that? bug/todo? just a
suggestion.

http://codereview.chromium.org/8885/diff/76/83#newcode724
Line 724: input_element->isPasswordField() || !input_element->autoComplete()) {
what is the isPasswordField check intended for? if you moved the excluded_fields
check to before this, then I don't think you need it? (or it could be a dcheck)?

http://codereview.chromium.org/8885/diff/76/85
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/8885/diff/76/85#newcode124
Line 124: class AutocompletePopupMenuClient : public
RefCounted<AutocompletePopupMenuClient>,
 80 lines

[jcampan, 2008-10-30 00:39:57]

On 2008/10/29 21:41:59, mpcomplete wrote:
> Looks good for the most part.  I'm surprised the PopupMenu code didn't need
more
> changes to work.
> 
> I'm not too happy with exposing PopupContainer to external users since its
more
> of an implementation detail, but it does seem like the least evil alternative.
Yeah, it is not great to have to expose PopupContainer.
Darin made a great suggestion of in the future creating the popup by adding
element to the render tree. I guess after 1.0 is released I could look into
that.

[jcampan, 2008-10-30 00:40:17]

http://codereview.chromium.org/8885/diff/76/94
File base/string_util.cc (right):

http://codereview.chromium.org/8885/diff/76/94#newcode668
Line 668: if (search.size() > str.size())
On 2008/10/29 22:51:25, timsteele wrote:
> this is really regardless of case sensitivity
I am assuming most std::wstring.compare() implementations do that check, so it
might end up being done twice if we do it for the case sensitive case.
I am no stl expert though, so I might be wrong.

http://codereview.chromium.org/8885/diff/76/94#newcode671
Line 671: CaseInsensitiveCompare<wchar_t>());
On 2008/10/29 22:51:25, timsteele wrote:
> nit- looks mis-indented

Done.

http://codereview.chromium.org/8885/diff/76/92
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/8885/diff/76/92#newcode1240
Line 1240: EXPECT_EQ(true, StartsWith(L"java", L"", true));
On 2008/10/29 22:51:25, timsteele wrote:
> suggestion- add a test for 
> EXPECT_FALSE(StartsWith(L"JavaScript:url", L"javascript", true))
Good point. Done.

http://codereview.chromium.org/8885/diff/76/97
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/8885/diff/76/97#newcode1220
Line 1220: /*
On 2008/10/29 21:41:59, mpcomplete wrote:
> nit: replace the comment with an #ifdef TEST_AUTOFILL or something of the
sort,
> to make it clear why it's there.

Done.

http://codereview.chromium.org/8885/diff/76/115
File webkit/glue/form_autocomplete_listener.cc (right):

http://codereview.chromium.org/8885/diff/76/115#newcode35
Line 35: }
On 2008/10/29 22:51:25, timsteele wrote:
> this could go in the header

Done.

http://codereview.chromium.org/8885/diff/76/115#newcode39
Line 39: // The input_element_ pointer is simply used as an id for the node.
On 2008/10/29 22:51:25, timsteele wrote:
> I think there is a somewhat subtle fact that the input_element_ pointer won't
> change because it's referenced by the superclass due to wrapping it in an
> HTMLInputDelegate, and it only drops the reference when this
> FormAutocompleteListener gets deleted. You could remove the member from this
> class and use edit_delegate() (returns a pointer to the element that will be
> autofilled), but it might make the most sense to change the member type to an
> HTMLInputDelegate (sticking with the AutocompleteInputListener style which is
to
> abstain from explicitly needing WebCore types, to simplify testing). At the
very
> least you'd probably want to Ref/Deref the HtmlInputElement yourself.

The only reason I need this pointer is to keep track of the node that is making
the request (as an ID), so when the suggestions combe back from the browser, I
can check that the focused node is still the node for which the query
originated. I just save the node id now, so there is no life management
ambiguity.

http://codereview.chromium.org/8885/diff/76/83
File webkit/glue/webframeloaderclient_impl.cc (right):

http://codereview.chromium.org/8885/diff/76/83#newcode55
Line 55: #include "webkit/glue/webframeloaderclient_impl.h"
On 2008/10/29 22:51:25, timsteele wrote:
> doesn't this go on top?

Actually I don't know. Looks like webwidget_impl.cc and webframe_impl.cc put it
in alphabetical order.

http://codereview.chromium.org/8885/diff/76/83#newcode374
Line 374: // Now let's register any text input.  Note that at some point the
On 2008/10/29 22:51:25, timsteele wrote:
> nit- register for any text input

Done.

http://codereview.chromium.org/8885/diff/76/83#newcode375
Line 375: // password and form autofill should be merged.
On 2008/10/29 22:51:25, timsteele wrote:
> hm.. do we want any other form of tracking for that? bug/todo? just a
> suggestion.

Done.

http://codereview.chromium.org/8885/diff/76/83#newcode724
Line 724: input_element->isPasswordField() || !input_element->autoComplete()) {
On 2008/10/29 22:51:25, timsteele wrote:
> what is the isPasswordField check intended for? if you moved the
excluded_fields
> check to before this, then I don't think you need it? (or it could be a
dcheck)?
I guess my thought was that this would prevent a lookup in the set for password
fields since we know we never want the password fields anyway.

http://codereview.chromium.org/8885/diff/76/85
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/8885/diff/76/85#newcode124
Line 124: class AutocompletePopupMenuClient : public
RefCounted<AutocompletePopupMenuClient>,
On 2008/10/29 22:51:25, timsteele wrote:
>  80 lines

Done.

http://codereview.chromium.org/8885/diff/76/85#newcode902
Line 902: HideAutoCompletePopup();
On 2008/10/29 21:41:59, mpcomplete wrote:
> Could we test if the popup has focus, and only hide it if it doesn't?  That
> might solve this problem, and we could hide on the page losing focus.
The render view is notified that it lost focus before the popup is notified it
got focus. So that won't work. The windows WM_KILLFOCUS message indicates which
HWND is getting focused. I guess the right fix would be to change the SETFOCUS
message we send to the RW to somehow include the RW (if any) that is going to be
focused.
I filed a bug and TODOed for another CL if you have no objections.

http://codereview.chromium.org/8885/diff/76/85#newcode1485
Line 1485: HideAutoCompletePopup();
On 2008/10/29 21:41:59, mpcomplete wrote:
> This seems strange... for every set of suggestions we get back, we hide and
> reshow the popup.  Does that result in any flickering?
I don't see any flickering.
I see your point, I could just update the currently showing popup. The thing is
that I am currently hidding the popup everytime a new key is pressed, so I would
also have to be smart on which keys (cursor, home, end, back-space...) I want to
hide it or not. Not sure if I would get a lot out of it.

http://codereview.chromium.org/8885/diff/76/77
File webkit/port/platform/chromium/PopupMenuChromium.h (right):

http://codereview.chromium.org/8885/diff/76/77#newcode23
Line 23: // NOTE: this class is exposed so it can be instanciated direcly for
the
On 2008/10/29 21:41:59, mpcomplete wrote:
> nit: instantiated

Done.

http://codereview.chromium.org/8885/diff/76/77#newcode36
Line 36: static void show(PopupContainer* popup, const IntRect& r,
On 2008/10/29 21:41:59, mpcomplete wrote:
> Can't this be turned into a non-static method, since its first parameter is a
> PopupContainer*?
You are right, this is dumb. Done.

[Matt Perry, 2008-10-30 00:44:50]

LGTM

[tim (not reviewing), 2008-10-30 01:30:04]

LGTM