Password autofill: Check for user gesture switches to new Blink backend

Password autofill: Check for user gesture switches to new Blink backend

After landing https://codereview.chromium.org/235983016/ in Blink, there is a better way to check for user gesture, which does also work for gesture events on Android. The new way is a callback from Blink through WebAutofillAgent::firstUserGestureObserved, this CL switches to the new way, and also cleans up unneeded additions to cotent/renderer from https://codereview.chromium.org/163843002

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

[vabr (Chromium), 2014-04-17 17:43:30]

Hi Ilya,
Could you please review components/autofill/* ?

Hi Antoine,
Could you please review content/* ?
(It removes what I added in https://codereview.chromium.org/163843002.)

Thanks,
Vaclav

https://codereview.chromium.org/241193002/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/241193002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:93: virtual void
firstUserGestureObserved() OVERRIDE;
It looks like all methods in this block should have OVERRIDE.
Should I add that to the other methods, or is it not appropriate for the new one
either?

[Ilya Sherman, 2014-04-17 21:28:47]

Spiffy.  Could you please also add some test coverage?  Or is this already
covered by existing tests?

https://codereview.chromium.org/241193002/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/241193002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:93: virtual void
firstUserGestureObserved() OVERRIDE;
On 2014/04/17 17:43:30, vabr (OOO until 22 Apr) wrote:
> It looks like all methods in this block should have OVERRIDE.
> Should I add that to the other methods, or is it not appropriate for the new
one
> either?

Actually, per request from jamesr@, none of them should have OVERRIDE, so as to
reduce the number of N-sided changes necessary when making changes that cross
the repository boundary.  We'll be able to restore the OVERRIDE annotations once
the Chromium and Blink repositories are merged (woohoo!).

[piman, 2014-04-17 23:33:59]

lgtm

[vabr (Chromium), 2014-04-18 18:23:53]

Thanks Antoine!

Thanks Ilya!
I answered your questions in the comments.

Note: the Blink backend has some fixes to be landed:
https://codereview.chromium.org/239963005/.
I will not land this CL before https://codereview.chromium.org/239963005/ makes
it to the Blink revision used by Chromium.

Cheers,
Vaclav

https://codereview.chromium.org/241193002/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/241193002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:93: virtual void
firstUserGestureObserved() OVERRIDE;
On 2014/04/17 21:28:47, Ilya Sherman wrote:
> On 2014/04/17 17:43:30, vabr (OOO until 22 Apr) wrote:
> > It looks like all methods in this block should have OVERRIDE.
> > Should I add that to the other methods, or is it not appropriate for the new
> one
> > either?
> 
> Actually, per request from jamesr@, none of them should have OVERRIDE, so as
to
> reduce the number of N-sided changes necessary when making changes that cross
> the repository boundary.  We'll be able to restore the OVERRIDE annotations
once
> the Chromium and Blink repositories are merged (woohoo!).

Makes sense, thanks!
OVERRIDE removed.

https://codereview.chromium.org/241193002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/241193002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:603:
MAYBE_PasswordValueAccessible) {
Thanks for reminding me of the test, I needed to enable it for Android. The
existing PasswordValueAccessible should cover all the functionality (the Blink
backend has also its own tests). For flakiness the Windows tests are not enabled
yet, but that's tracked as a bug as noted. I can test the result manually on
Windows as soon as I'm back in the office.

[Ilya Sherman, 2014-04-18 18:25:12]

LGTM, thanks.

[vabr (Chromium), 2014-04-23 11:41:38]

The fixes in Blink landed yesterday
(https://codereview.chromium.org/239963005/), and I just verified on a locally
compiled Clank with patch set 2 from here, that tap and long-press make password
element .value available to JavaScript.

Therefore I'm sending this to the CQ.

Cheers,
Vaclav

[vabr (Chromium), 2014-04-23 11:41:44]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-04-23 11:42:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/241193002/60001

[commit-bot: I haz the power, 2014-04-23 13:48:07]

Change committed as 265632