PasswordFormManager supports multiple drivers

PasswordFormManager supports multiple drivers

PFM serves potentially multiple frames by providing (auto)fill
information for each. That happens through drivers, which are
recently one per frame. The PFM only supports one driver so far.

This CL makes it handle a queue of drivers until it retrieves
the credentials from the store, and then serve each of the
associated frames through the drivers.

BUG=440438
Committed: https://crrev.com/283c1f3ea40e007a8b92d314e97b1bffc1910c67
Cr-Commit-Position: refs/heads/master@{#308065}

[vabr (Chromium), 2014-12-11 18:33:41]

vabr@chromium.org changed reviewers:
+ gcasto@chromium.org

[vabr (Chromium), 2014-12-11 18:33:42]

Evan, this is just FYI.

Garrett, PTAL!

Thanks!
Vaclav

https://codereview.chromium.org/795183002/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/795183002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:569:
.WillOnce(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
These two are unrelated, but I squeezed them in when copy-pasting the new test
-- passing result transfers ownership, so returning that more than once will use
in double-free. Better discover it sooner through GMock.

[vabr (Chromium), 2014-12-11 20:12:52]

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:553:
ProcessFrame(driver);
I could actually check driver here, instead of inside ProcessFrame. That would
spare passing it the WeakPtr from the other callsite in PasswordManager.
If ProcessFrame gets called from that other callsite, and |this| is not yet
ready, it would extract the weak pointer from the naked one, and save it in
drivers_. Otherwise it can go on using the naked one.
I'm not sure if it is any better than the current solution. WDYT?

[Garrett Casto, 2014-12-12 08:19:10]

lgtm

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:515:
driver->AllowPasswordGenerationForForm(observed_form_);
Nit: Might want to leave one line explaining that we allow generation for any
form that isn't blacklisted.

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:553:
ProcessFrame(driver);
On 2014/12/11 20:12:52, vabr (Chromium) wrote:
> I could actually check driver here, instead of inside ProcessFrame. That would
> spare passing it the WeakPtr from the other callsite in PasswordManager.
> If ProcessFrame gets called from that other callsite, and |this| is not yet
> ready, it would extract the weak pointer from the naked one, and save it in
> drivers_. Otherwise it can go on using the naked one.
> I'm not sure if it is any better than the current solution. WDYT?

I don't think that it matters much either way. I suppose I slightly prefer it
this way because it's clearer to callers of ProcessFrame() that action may be
taken at a later time.

[vabr (Chromium), 2014-12-12 08:52:48]

Thanks, Garrett!

I addressed your comment, and will send this to the CQ now.

Cheers,
Vaclav

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:515:
driver->AllowPasswordGenerationForForm(observed_form_);
On 2014/12/12 08:19:10, Garrett Casto wrote:
> Nit: Might want to leave one line explaining that we allow generation for any
> form that isn't blacklisted.

Done.

https://codereview.chromium.org/795183002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:553:
ProcessFrame(driver);
On 2014/12/12 08:19:10, Garrett Casto wrote:
> On 2014/12/11 20:12:52, vabr (Chromium) wrote:
> > I could actually check driver here, instead of inside ProcessFrame. That
would
> > spare passing it the WeakPtr from the other callsite in PasswordManager.
> > If ProcessFrame gets called from that other callsite, and |this| is not yet
> > ready, it would extract the weak pointer from the naked one, and save it in
> > drivers_. Otherwise it can go on using the naked one.
> > I'm not sure if it is any better than the current solution. WDYT?
> 
> I don't think that it matters much either way. I suppose I slightly prefer it
> this way because it's clearer to callers of ProcessFrame() that action may be
> taken at a later time.

I agreee! :)

[vabr (Chromium), 2014-12-12 08:52:53]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-12-12 08:54:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/795183002/60001

[commit-bot: I haz the power, 2014-12-12 09:54:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 09:55:05]

Patchset 4 (id:??) landed as
https://crrev.com/283c1f3ea40e007a8b92d314e97b1bffc1910c67
Cr-Commit-Position: refs/heads/master@{#308065}