Postpone check if password form is loaded from store

In some cases we encounter situation when in processing onSubmit of password form we didn't receive information from password store yet. For example this can happen when JavaScript creates password form after submission and submit it (as for nytimes.com).

To process such situation this CL postpones checking if password form fetched from store until moment when we are going to decide if we should save it.

In order to process this correctly creating pending credentials in PasswordManager was moved to separate method and it is called only when both events happen - form submission and fetching from store finished.

Tests are not added yet

BUG=470322
Committed: https://crrev.com/f46ff48284131eba449796a932ec32290b03a403
Cr-Commit-Position: refs/heads/master@{#324400}

[dvadym, 2015-04-07 12:51:04]

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

[dvadym, 2015-04-07 12:51:05]

Hi Vaclav and Garrett,

 Could you please review this CL?

Best regards,
Vadym

[vabr (Chromium), 2015-04-07 13:27:15]

LGTM with some comments.
Thanks!
Vaclav

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:262:
DCHECK(state_ == MATCHING_PHASE || state_ == POST_MATCHING_PHASE);
nit: Add
<< state_
before ;, so that it is clear which condition broke the DCHECK.

(Also on line 274.)

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:890: delete
provisionally_saved_form_;
Please do not use delete, use scoped_ptr (and .reset())!

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:344: // Stores
provionally saved form until |pending_credentials_| is created.
typo: provionally

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:345: const
autofill::PasswordForm* provisionally_saved_form_;
Who owns this PasswordForm?
(Looks like it is owned by |this|, in that case it needs to be a scoped_ptr.)

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:348:
OtherPossibleUsernamesAction other_possible_username_action_;
Please initialise this in the constructor as well.

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_manager.cc:504: // We found
some managers, but none finished matching yet. The user has
Please update the comment. This is no longer about the moment just about
submission, this is about not having the store results by the time the next page
is rendered.

[vabr (Chromium), 2015-04-07 13:28:00]

Still, I'd like to review the tests as well, before you land.

Cheers,
Vaclav

[dvadym, 2015-04-07 13:49:10]

Thanks Vaclav!
 I've addressed your comments in PatchSet 6. Sure, I will not land until getting
approval on tests.

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:262:
DCHECK(state_ == MATCHING_PHASE || state_ == POST_MATCHING_PHASE);
On 2015/04/07 13:27:15, vabr (Chromium) wrote:
> nit: Add
> << state_
> before ;, so that it is clear which condition broke the DCHECK.
> 
> (Also on line 274.)

Done.

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:890: delete
provisionally_saved_form_;
On 2015/04/07 13:27:14, vabr (Chromium) wrote:
> Please do not use delete, use scoped_ptr (and .reset())!

Done.

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:344: // Stores
provionally saved form until |pending_credentials_| is created.
On 2015/04/07 13:27:15, vabr (Chromium) wrote:
> typo: provionally

Done.

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:345: const
autofill::PasswordForm* provisionally_saved_form_;
On 2015/04/07 13:27:15, vabr (Chromium) wrote:
> Who owns this PasswordForm?
> (Looks like it is owned by |this|, in that case it needs to be a scoped_ptr.)

It's changed to scoped_ptr

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:348:
OtherPossibleUsernamesAction other_possible_username_action_;
On 2015/04/07 13:27:15, vabr (Chromium) wrote:
> Please initialise this in the constructor as well.

Done.

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_manager.cc:504: // We found
some managers, but none finished matching yet. The user has
On 2015/04/07 13:27:15, vabr (Chromium) wrote:
> Please update the comment. This is no longer about the moment just about
> submission, this is about not having the store results by the time the next
page
> is rendered.

I've changed to
    // We have a provisional save manager, but it didn't finish matching yet.
    // We just give up.

[vabr (Chromium), 2015-04-07 13:54:40]

LGTM so far, looking forward to the tests. :)

Cheers,
Vaclav

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/70001/components/password_man...
components/password_manager/core/browser/password_manager.cc:504: // We found
some managers, but none finished matching yet. The user has
On 2015/04/07 13:49:10, dvadym wrote:
> On 2015/04/07 13:27:15, vabr (Chromium) wrote:
> > Please update the comment. This is no longer about the moment just about
> > submission, this is about not having the store results by the time the next
> page
> > is rendered.
> 
> I've changed to
>     // We have a provisional save manager, but it didn't finish matching yet.
>     // We just give up.

Acknowledged.

[dvadym, 2015-04-08 15:31:30]

Hi Vaclav and Garrett,

 I've added a test and fixed a bug that I found with this test. Could you please
review these changes

Best regards,
Vadym

[vabr (Chromium), 2015-04-08 16:22:40]

Thanks for the test, LGTM!
Vaclav

https://codereview.chromium.org/1050903002/diff/110001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1050903002/diff/110001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1101: //
Make that action called on GetLogins call does not send form to
grammar: Please rephrase the first two sentences to, e.g.:
Do not call back from store after GetLogins is called. Instead, save the pointer
to the form manager for calling back later.

[dvadym, 2015-04-08 16:26:21]

Thanks Vaclav!

https://codereview.chromium.org/1050903002/diff/110001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1050903002/diff/110001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1101: //
Make that action called on GetLogins call does not send form to
On 2015/04/08 16:22:40, vabr (Chromium) wrote:
> grammar: Please rephrase the first two sentences to, e.g.:
> Do not call back from store after GetLogins is called. Instead, save the
pointer
> to the form manager for calling back later.

Done.

[vabr (Chromium), 2015-04-09 07:21:30]

LGTM, thanks!
Vaclav

[dvadym, 2015-04-09 07:45:21]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-04-09 07:45:38]

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

[commit-bot: I haz the power, 2015-04-09 08:41:51]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-04-09 08:42:48]

Patchset 9 (id:??) landed as
https://crrev.com/f46ff48284131eba449796a932ec32290b03a403
Cr-Commit-Position: refs/heads/master@{#324400}

[battre, 2017-07-13 13:30:50]

battre@chromium.org changed reviewers:
+ battre@chromium.org

[battre, 2017-07-13 13:30:51]

Vadym, I have a question regarding a very old CL of yours.

https://codereview.chromium.org/1050903002/diff/150001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/150001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:525:
CreatePendingCredentials();
Do you remember why you decided not to return here but continue to process the
frames by the drivers? - Just curious... (The code still exists today in a
similar way)

[vabr (Chromium), 2017-07-13 14:01:27]

https://codereview.chromium.org/1050903002/diff/150001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1050903002/diff/150001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:525:
CreatePendingCredentials();
On 2017/07/13 13:30:51, battre wrote:
> Do you remember why you decided not to return here but continue to process the
> frames by the drivers? - Just curious... (The code still exists today in a
> similar way)

The PasswordFormManager still needs to ensure saved forms are filled. The added
bit of code just adds a different functionality of creating pending credentials
in a special case of store response coming later than expected. There seems to
be no reason to suspend the first functionality because of executing the second
one.