Basic autofill into password value on user gesture only.

Basic autofill into password value on user gesture only.

BUG=163072
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238561

[jww, 2013-11-23 02:08:47]

gcasto, this is my starting implementation of autofill into passwords only on
user gesture. Note that this is dependent on another CL I'm writing to actually
provide Blink handlers for user gestures:
https://codereview.chromium.org/82693006/

[jww, 2013-11-26 00:47:49]

I got feedback from abarth on my gestures callback API and have updated that CL,
so I changed this CL to match the new API. Additionally, I added some unit
tests. Obviously need to wait until the blink API CL lands, but otherwise I
think this is finished.

[jww, 2013-11-26 20:15:00]

One more update based on some API changes on the Blink side of things.

Additionally, the Blink API got the LGTM, so it should be landing soon.

[Garrett Casto, 2013-12-02 23:49:11]

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:30: #include
"third_party/WebKit/public/web/WebUserGestureToken.h"
Is this leftover from a previous version? Doesn't seem used now.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:201:
blink::WebUserGestureIndicator::setHandler(0);
use NULL for this instead of 0, since we are talking about a pointer.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:741: // for such
a gesture to occur.
Shouldn't this be "If a user gesture hasn't occurred, setup a handler to wait
for such a gesture". That is, the gesture doesn't need to be occurring right
now, just had to have happened at some point in the past.

It also seems like we need to clear this after main frame navigation.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:743:
gestureHandler_->addElement(new blink::WebInputElement(*password_element));
I think that we can just keep a copy of the WebInputElement instead of a new
pointer. The blink/chromium interface just consists of thin wrappers around the
blink objects which are ref-counted, so the overhead from doing this is tiny. It
also frees you from doing the manual memory management that you are doing now.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:820: void
PasswordAutofillAgent::AutofillWebUserGestureHandler::onGesture() {
To be clear, this gets called whenever there is a click, button press, or screen
touch? I tried tracing the code back, but it got a little hairy :).

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:826:
DCHECK(!(*iter)->suggestedValue().isNull());
I don't think that there is much value to DCHECK on something that you check the
line before.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:841:
std::vector<blink::WebInputElement*>::iterator iter;
I don't think that this should be necessary to keep around, but for future
reference if you want to delete pointers in a container, use
STLDeleteContainerPointers()
(https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=...).

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:164: void
setUserGestureOccurred(bool occurred) {
Inline functions like this are generally also done in hacker_case, so
set_user_gesture_occurred(). http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:181:
base::WeakPtrFactory<PasswordAutofillAgent> weak_ptr_factory_;
WeakPtrFactory should always be the last member variable. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:183:
AutofillWebUserGestureHandler* gestureHandler_;
These should be unix_hacker_style_ not mixedCase_.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

[jww, 2013-12-03 06:29:59]

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:30: #include
"third_party/WebKit/public/web/WebUserGestureToken.h"
On 2013/12/02 23:49:12, Garrett Casto wrote:
> Is this leftover from a previous version? Doesn't seem used now.

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:201:
blink::WebUserGestureIndicator::setHandler(0);
On 2013/12/02 23:49:12, Garrett Casto wrote:
> use NULL for this instead of 0, since we are talking about a pointer.

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:741: // for such
a gesture to occur.
On 2013/12/02 23:49:12, Garrett Casto wrote:
> Shouldn't this be "If a user gesture hasn't occurred, setup a handler to wait
> for such a gesture". That is, the gesture doesn't need to be occurring right
> now, just had to have happened at some point in the past.
> 
> It also seems like we need to clear this after main frame navigation.

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:743:
gestureHandler_->addElement(new blink::WebInputElement(*password_element));
On 2013/12/02 23:49:12, Garrett Casto wrote:
> I think that we can just keep a copy of the WebInputElement instead of a new
> pointer. The blink/chromium interface just consists of thin wrappers around
the
> blink objects which are ref-counted, so the overhead from doing this is tiny.
It
> also frees you from doing the manual memory management that you are doing now.

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:820: void
PasswordAutofillAgent::AutofillWebUserGestureHandler::onGesture() {
On 2013/12/02 23:49:12, Garrett Casto wrote:
> To be clear, this gets called whenever there is a click, button press, or
screen
> touch? I tried tracing the code back, but it got a little hairy :).

Yup!

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:826:
DCHECK(!(*iter)->suggestedValue().isNull());
On 2013/12/02 23:49:12, Garrett Casto wrote:
> I don't think that there is much value to DCHECK on something that you check
the
> line before.

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:841:
std::vector<blink::WebInputElement*>::iterator iter;
On 2013/12/02 23:49:12, Garrett Casto wrote:
> I don't think that this should be necessary to keep around, but for future
> reference if you want to delete pointers in a container, use
> STLDeleteContainerPointers()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=...).

Oh, awesome!

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:164: void
setUserGestureOccurred(bool occurred) {
On 2013/12/02 23:49:12, Garrett Casto wrote:
> Inline functions like this are generally also done in hacker_case, so
> set_user_gesture_occurred(). http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:181:
base::WeakPtrFactory<PasswordAutofillAgent> weak_ptr_factory_;
On 2013/12/02 23:49:12, Garrett Casto wrote:
> WeakPtrFactory should always be the last member variable. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

Done.

https://codereview.chromium.org/83023017/diff/70001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:183:
AutofillWebUserGestureHandler* gestureHandler_;
On 2013/12/02 23:49:12, Garrett Casto wrote:
> These should be unix_hacker_style_ not mixedCase_.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

Done.

[Garrett Casto, 2013-12-04 00:25:31]

LGTM with nits

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:829: if
(!(*iter).isNull() && !(*iter).suggestedValue().isNull())
"(*iter)." -> "iter->"

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:185:
AutofillWebUserGestureHandler* gestureHandler_;
gesture_handler_. Also this should be a scoped_ptr<>

[jww, 2013-12-04 00:45:10]

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.cc:829: if
(!(*iter).isNull() && !(*iter).suggestedValue().isNull())
On 2013/12/04 00:25:31, Garrett Casto wrote:
> "(*iter)." -> "iter->"

Done.

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/83023017/diff/90001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:185:
AutofillWebUserGestureHandler* gestureHandler_;
On 2013/12/04 00:25:31, Garrett Casto wrote:
> gesture_handler_. Also this should be a scoped_ptr<>

Done.

[commit-bot: I haz the power, 2013-12-04 00:46:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jww@chromium.org/83023017/110001

[commit-bot: I haz the power, 2013-12-04 04:23:05]

Change committed as 238561