New take at implementing autofill using the editor client API

A new implementation of the autofill using the editor client API.
This simplifies code as we don't need to listen for events on input elements, the editor client API is only triggered when the text changes.
The only quirk we have to work around is that when the editor client API notifies us that the text has changed, the selection is not set properly, preventing us from reliably finding out if the caret is at the end of the text.
To work around that issue, we post a task that does the autofill after the text change callback.

BUG=None
TEST=Trigger the autofill behavior with form and passwords.



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

[darin (slow to review), 2008-11-19 18:33:45]

http://codereview.chromium.org/11479/diff/1/10
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/11479/diff/1/10#newcode278
Line 278: RefPtr<WebCore::HTMLInputElement> input_element,
this looks like an improper usage of RefPtr.  you should probably be using
PassRefPtr.  please see http://webkit.org/coding/RefPtr.html for details.

http://codereview.chromium.org/11479/diff/1/10#newcode457
Line 457: struct CmpRefPtrs {
nit: google style is to spell out words, so CompareRefPtrs would be better

[tim (not reviewing), 2008-11-19 19:59:27]

http://codereview.chromium.org/11479/diff/1/12
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/11479/diff/1/12#newcode623
Line 623: // Don't attempt to autofill with too large values.
nit- 'Don't attempt to autofill with values that are too large.'

BTW, how did this come up? How'd you pick 1000? What happens?

http://codereview.chromium.org/11479/diff/1/12#newcode624
Line 624: if (input_element->value().length() > 1000)
nit - use constant

http://codereview.chromium.org/11479/diff/1/12#newcode635
Line 635: backspace_pressed_));
I'm still confused about this because I don't see how backspace_pressed_ is
reliable given that the user can type or hit backspace between now and when the
task is run

http://codereview.chromium.org/11479/diff/1/12#newcode642
Line 642: // Only autofill when there is some text and the caret is at the end.
I think we want to autofill a username (at least in the password manager case)
even if the value is empty... ?

http://codereview.chromium.org/11479/diff/1/6
File webkit/glue/password_autocomplete_listener.h (right):

http://codereview.chromium.org/11479/diff/1/6#newcode61
Line 61: // (username/password) and if so, match them up by autocompleting the
edit
nit- indents

http://codereview.chromium.org/11479/diff/1/8
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/11479/diff/1/8#newcode1895
Line 1895: delete iter->second;
do we have to deref the keys?

http://codereview.chromium.org/11479/diff/1/10
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/11479/diff/1/10#newcode286
Line 286: WebCore::HTMLInputElement* input_element);
is |input_element| meant to be the username element? or the password element? 
also a nit- fix indenting.

http://codereview.chromium.org/11479/diff/1/10#newcode466
Line 466: typedef std::map<RefPtr<WebCore::HTMLInputElement>,
hm.. looks like other code in glue uses the WebCore HashMap for stashing WebCore
types (editor_client_impl, webplugin_impl). I'm not familiar enough with it to
know the differences or if it has something to do with how std::map operations
make internal copies, etc. In any case, you may want to use HashMap to be
consistent with glue?

[jcampan, 2008-11-19 20:29:34]

http://codereview.chromium.org/11479/diff/1/12
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/11479/diff/1/12#newcode623
Line 623: // Don't attempt to autofill with too large values.
On 2008/11/19 19:59:27, timsteele wrote:
> nit- 'Don't attempt to autofill with values that are too large.'
> 
> BTW, how did this come up? How'd you pick 1000? What happens?

OK, I kind of pulled 1000 out of thin air.
The goal here is to prevent sending to the browser large IPC messages when the
user types really long strings, since most likely the autocomplete is not going
to be useful in that case.
Let me know if you think that's overkill.

http://codereview.chromium.org/11479/diff/1/12#newcode624
Line 624: if (input_element->value().length() > 1000)
On 2008/11/19 19:59:27, timsteele wrote:
> nit - use constant

Done.

http://codereview.chromium.org/11479/diff/1/12#newcode635
Line 635: backspace_pressed_));
On 2008/11/19 19:59:27, timsteele wrote:
> I'm still confused about this because I don't see how backspace_pressed_ is
> reliable given that the user can type or hit backspace between now and when
the
> task is run
The calls to autofill_factory_.RevokeAll() ensures that we discard any pending
execution of the task.
So if a task executes, it is guaranteed that the text did not change since it
was posted.

http://codereview.chromium.org/11479/diff/1/12#newcode642
Line 642: // Only autofill when there is some text and the caret is at the end.
On 2008/11/19 19:59:27, timsteele wrote:
> I think we want to autofill a username (at least in the password manager case)
> even if the value is empty... ?
I am not sure how that could happen. Here we have been called because the text
has changed. So if the value is empty, it means either backspace was pressed and
the last char was deleted or the entire value was selected and then cut/deleted.
Do we really want to do autofill for passwords in that case?

http://codereview.chromium.org/11479/diff/1/6
File webkit/glue/password_autocomplete_listener.h (right):

http://codereview.chromium.org/11479/diff/1/6#newcode61
Line 61: // (username/password) and if so, match them up by autocompleting the
edit
On 2008/11/19 19:59:27, timsteele wrote:
> nit- indents

Done.

http://codereview.chromium.org/11479/diff/1/8
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/11479/diff/1/8#newcode1895
Line 1895: delete iter->second;
On 2008/11/19 19:59:27, timsteele wrote:
> do we have to deref the keys?
No, the key is a RefPtr which automatically derefs when deleted (when the entry
is removed).

http://codereview.chromium.org/11479/diff/1/10
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/11479/diff/1/10#newcode278
Line 278: RefPtr<WebCore::HTMLInputElement> input_element,
On 2008/11/19 18:33:45, darin wrote:
> this looks like an improper usage of RefPtr.  you should probably be using
> PassRefPtr.  please see http://webkit.org/coding/RefPtr.html for details.

Done.

http://codereview.chromium.org/11479/diff/1/10#newcode286
Line 286: WebCore::HTMLInputElement* input_element);
On 2008/11/19 19:59:27, timsteele wrote:
> is |input_element| meant to be the username element? or the password element? 
> also a nit- fix indenting.
Yes, it is meant to be the username element, I clarified as this was confusing.

http://codereview.chromium.org/11479/diff/1/10#newcode457
Line 457: struct CmpRefPtrs {
On 2008/11/19 18:33:45, darin wrote:
> nit: google style is to spell out words, so CompareRefPtrs would be better

Done.

http://codereview.chromium.org/11479/diff/1/10#newcode466
Line 466: typedef std::map<RefPtr<WebCore::HTMLInputElement>,
On 2008/11/19 19:59:27, timsteele wrote:
> hm.. looks like other code in glue uses the WebCore HashMap for stashing
WebCore
> types (editor_client_impl, webplugin_impl). I'm not familiar enough with it to
> know the differences or if it has something to do with how std::map operations
> make internal copies, etc. In any case, you may want to use HashMap to be
> consistent with glue?

Sorry, I greped but somehow could not find HashMap used in glue (I don't seem to
find it in editor_client_impl and webplugin_impl either). Can you point me to a
specific file and line number where you see this being used?

[tim (not reviewing), 2008-11-19 21:04:38]

http://codereview.chromium.org/11479/diff/1/12
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/11479/diff/1/12#newcode642
Line 642: // Only autofill when there is some text and the caret is at the end.
Ah, right. The case I was talking about is still handled in dom_operations.cc. I
think this is fine.
On 2008/11/19 20:29:34, jcampan wrote:
> On 2008/11/19 19:59:27, timsteele wrote:
> > I think we want to autofill a username (at least in the password manager
case)
> > even if the value is empty... ?
> I am not sure how that could happen. Here we have been called because the text
> has changed. So if the value is empty, it means either backspace was pressed
and
> the last char was deleted or the entire value was selected and then
cut/deleted.
> Do we really want to do autofill for passwords in that case?
>

http://codereview.chromium.org/11479/diff/1/10
File webkit/glue/webframe_impl.h (right):

http://codereview.chromium.org/11479/diff/1/10#newcode466
Line 466: typedef std::map<RefPtr<WebCore::HTMLInputElement>,
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/editor_client_imp...

On 2008/11/19 20:29:34, jcampan wrote:
> On 2008/11/19 19:59:27, timsteele wrote:
> > hm.. looks like other code in glue uses the WebCore HashMap for stashing
> WebCore
> > types (editor_client_impl, webplugin_impl). I'm not familiar enough with it
to
> > know the differences or if it has something to do with how std::map
operations
> > make internal copies, etc. In any case, you may want to use HashMap to be
> > consistent with glue?
> 
> Sorry, I greped but somehow could not find HashMap used in glue (I don't seem
to
> find it in editor_client_impl and webplugin_impl either). Can you point me to
a
> specific file and line number where you see this being used?

[jcampan, 2008-11-19 22:06:25]

Tim,
I see why I missed the hashmap, I was looking for specifically a
HashMap<RefPtr<...
Yes, it is better to use a HashMap since they have a specialization for keys
that are RefPtr.
Done in new snapshot.

[tim (not reviewing), 2008-11-19 23:33:23]

LGTM