credentials_to_update should own its PasswordForms

credentials_to_update should own its PasswordForms

Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument is used to pass copies of non-best matches with a modified password, in order to be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a pointer to it is passed in |credentials_to_update|. However, |not_best_matches_| will need to become constant as its ownership transfers to the coming FormFetcher class, to avoid potential for surprising side-effects when multiple PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of |credentials_to_update| from a pointer to a PasswordForm to a PasswordForm itself. That way it is possible to modify just the copy inside |credentials_to_update| and not the original in |not_best_matches_|. The fact that |not_best_matches_| fall out of sync with the PasswordStore at that point does not matter: the current PasswordFormManager is currently saving a PasswordForm, so it will be destroyed soon and will not need |not_best_matches_|.

BUG=621355
Committed: https://crrev.com/df4325ec18d2f313f0d36903895c58f294418c58
Cr-Commit-Position: refs/heads/master@{#413436}

[vabr (Chromium), 2016-08-19 18:44:01]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 18:44:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-19 19:37:38]

Description was changed from

==========
Remove FormSaverImpl::UpdatePreferredLoginState

The method was used to update PasswordFormManager's |best_matches_| at the
point of saving a form to the PasswordStore. There seems to be no use of
|best_matches_| at this point inside PasswordFormManager or FormSaverImpl, and
saving is typically the last action performed by a PasswordFormManager (which
owns the FormSaverImpl) before being destroyed. Therefore it seems like an
unnecessary work to update the |best_matches_|.

Furthermore, this was the only edit made to forms in |best_matches_|. Getting
rid of it will make it possible to make forms in |best_matches_| constant. That
will prepare PasswordFormManager for transferring its ownership to the future
class of FormFetcher (hence the association to the BUG mentioned below).

BUG=621355
==========

to

==========
Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355
==========

[vabr (Chromium), 2016-08-19 19:37:54]

Description was changed from

==========
Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355
==========

to

==========
credentials_to_update should own its PasswordForms

Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355
==========

[commit-bot: I haz the power, 2016-08-19 19:53:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 19:53:45]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-08-19 20:58:41]

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

[vabr (Chromium), 2016-08-19 20:58:42]

Hi Vasilii,

PTAL.

Thanks!
Vaclav

[vasilii, 2016-08-22 09:20:01]

lgtm

https://codereview.chromium.org/2263683002/diff/20001/components/password_man...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2263683002/diff/20001/components/password_man...
components/password_manager/core/browser/form_saver_impl.cc:104:
credentials_to_update = nullptr;
It doesn't seem that something breaks without this code.

[vabr (Chromium), 2016-08-22 12:31:07]

Thanks!
Vaclav

https://codereview.chromium.org/2263683002/diff/20001/components/password_man...
File components/password_manager/core/browser/form_saver_impl.cc (right):

https://codereview.chromium.org/2263683002/diff/20001/components/password_man...
components/password_manager/core/browser/form_saver_impl.cc:104:
credentials_to_update = nullptr;
On 2016/08/22 09:20:01, vasilii wrote:
> It doesn't seem that something breaks without this code.

That seems correct. So I removed the unnecessary lines.

[vabr (Chromium), 2016-08-22 12:31:32]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-08-22 12:31:33]

The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2263683002/#ps60001
(title: "No empty->null")

[commit-bot: I haz the power, 2016-08-22 12:31:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 13:24:17]

Description was changed from

==========
credentials_to_update should own its PasswordForms

Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355
==========

to

==========
credentials_to_update should own its PasswordForms

Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355
==========

[commit-bot: I haz the power, 2016-08-22 13:24:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-22 13:26:18]

Description was changed from

==========
credentials_to_update should own its PasswordForms

Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355
==========

to

==========
credentials_to_update should own its PasswordForms

Inside PasswordFormManager and FormSaver, the |credentials_to_update| argument
is used to pass copies of non-best matches with a modified password, in order to
be saved to PasswordStore.

Currently, the PasswordForm in |not_best_matches_| is changed directly, and a
pointer to it is passed in |credentials_to_update|. However, |not_best_matches_|
will need to become constant as its ownership transfers to the coming
FormFetcher class, to avoid potential for surprising side-effects when multiple
PasswordFormManager change the shared object inside one FormFetcher.

This CL makes that possible by changing the element type of
|credentials_to_update| from a pointer to a PasswordForm to a PasswordForm
itself. That way it is possible to modify just the copy inside
|credentials_to_update| and not the original in |not_best_matches_|. The fact
that |not_best_matches_| fall out of sync with the PasswordStore at that point
does not matter: the current PasswordFormManager is currently saving a
PasswordForm, so it will be destroyed soon and will not need
|not_best_matches_|.

BUG=621355

Committed: https://crrev.com/df4325ec18d2f313f0d36903895c58f294418c58
Cr-Commit-Position: refs/heads/master@{#413436}
==========

[commit-bot: I haz the power, 2016-08-22 13:26:19]

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