[Password Manager] Convert |pending_login_managers_| to an array of scoped_refptr

[Password Manager] Convert |pending_login_managers_| to an array of scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for password saving is implemented, both PasswordManager and PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: the matched PasswordFormManager is removed from |pending_login_managers_| on form submission (https://cs.chromium.org/chromium/src/components/password_manager/core/browser/password_manager.cc?rcl=2f63ef02c274ccc73241c9723ffc3edb94f7529e&l=356). If the login has failed, we might still need that PasswordFormManager in |pending_login_managers_|  and have to re-create it.

BUG=725883
TEST=PasswordManagerTest.InPageNavigation

Review-Url: https://codereview.chromium.org/2900693002
Cr-Commit-Position: refs/heads/master@{#475889}
Committed: https://chromium.googlesource.com/chromium/src/+/b31c9aa77f4ab68acbfc4b2a01216ec0f42e07c9

[kolos1, 2017-05-24 12:39:12]

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

[commit-bot: I haz the power, 2017-05-24 12:39:29]

Dry run: 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, 2017-05-24 12:49:29]

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

[commit-bot: I haz the power, 2017-05-24 12:49:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kolos1, 2017-05-24 12:49:32]

Description was changed from

==========
o


o


o


o

BUG=
==========

to

==========
[Password Manager] Conver |pending_login_managers_| to an array of scoped_refptr


Before this CL, |pending_login_managers_| is an array of unique_ptr. So, we have
to pass ownership to | PasswordsClientUIDelegate|.

When manual fallback for password saving is implemented, both PasswordManager
and PasswordsClientUIDelegate should have access to the PasswordFormManager.

Anothere issue that this CL fixes. If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
==========

[kolos1, 2017-05-24 13:41:35]

Description was changed from

==========
[Password Manager] Conver |pending_login_managers_| to an array of scoped_refptr


Before this CL, |pending_login_managers_| is an array of unique_ptr. So, we have
to pass ownership to | PasswordsClientUIDelegate|.

When manual fallback for password saving is implemented, both PasswordManager
and PasswordsClientUIDelegate should have access to the PasswordFormManager.

Anothere issue that this CL fixes. If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
==========

to

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| is an array of unique_ptr. So, we have
to pass ownership to | PasswordsClientUIDelegate|.

When manual fallback for password saving is implemented, both PasswordManager
and PasswordsClientUIDelegate should have access to the PasswordFormManager.

Anothere issue that this CL fixes. If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
==========

[kolos1, 2017-05-24 15:23:41]

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

[commit-bot: I haz the power, 2017-05-24 15:24:13]

Dry run: 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, 2017-05-24 15:32:54]

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

[commit-bot: I haz the power, 2017-05-24 15:32:55]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[kolos1, 2017-05-24 16:07:47]

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

[commit-bot: I haz the power, 2017-05-24 16:08:20]

Dry run: 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, 2017-05-24 16:19:13]

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

[commit-bot: I haz the power, 2017-05-24 16:19:14]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kolos1, 2017-05-24 18:44:49]

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

[commit-bot: I haz the power, 2017-05-24 18:45:35]

Dry run: 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, 2017-05-24 19:56:44]

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

[commit-bot: I haz the power, 2017-05-24 19:56:45]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[kolos1, 2017-05-26 10:09:34]

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

[commit-bot: I haz the power, 2017-05-26 10:09:42]

Dry run: 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, 2017-05-26 10:33:01]

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

[commit-bot: I haz the power, 2017-05-26 10:33:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kolos1, 2017-05-26 11:08:42]

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

[commit-bot: I haz the power, 2017-05-26 11:08:54]

Dry run: 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, 2017-05-26 11:57:55]

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

[commit-bot: I haz the power, 2017-05-26 11:57:56]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-05-29 08:36:13]

Patchset #3 (id:40001) has been deleted

[kolos1, 2017-05-29 08:45:45]

Patchset #2 (id:20001) has been deleted

[kolos1, 2017-05-29 08:45:56]

Patchset #1 (id:1) has been deleted

[kolos1, 2017-05-29 08:47:53]

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

[commit-bot: I haz the power, 2017-05-29 08:48:09]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-05-29 09:22:45]

Description was changed from

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| is an array of unique_ptr. So, we have
to pass ownership to | PasswordsClientUIDelegate|.

When manual fallback for password saving is implemented, both PasswordManager
and PasswordsClientUIDelegate should have access to the PasswordFormManager.

Anothere issue that this CL fixes. If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
==========

to

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
==========

[kolos1, 2017-05-29 09:23:40]

Patchset #2 (id:80001) has been deleted

[kolos1, 2017-05-29 09:23:51]

Patchset #2 (id:80002) has been deleted

[kolos1, 2017-05-29 09:24:06]

Patchset #2 (id:100001) has been deleted

[kolos1, 2017-05-29 09:24:23]

Patchset #2 (id:120001) has been deleted

[kolos1, 2017-05-29 09:24:34]

Patchset #2 (id:140001) has been deleted

[kolos1, 2017-05-29 09:24:45]

Patchset #2 (id:160001) has been deleted

[kolos1, 2017-05-29 09:24:55]

Patchset #2 (id:180001) has been deleted

[kolos1, 2017-05-29 09:25:11]

Patchset #2 (id:200001) has been deleted

[kolos1, 2017-05-29 09:25:27]

Patchset #2 (id:220001) has been deleted

[kolos1, 2017-05-29 09:25:41]

Patchset #2 (id:240001) has been deleted

[commit-bot: I haz the power, 2017-05-29 09:37:22]

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

[commit-bot: I haz the power, 2017-05-29 09:37:23]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-05-29 09:47:03]

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

[commit-bot: I haz the power, 2017-05-29 09:47:13]

Dry run: 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, 2017-05-29 09:49:49]

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

[commit-bot: I haz the power, 2017-05-29 09:49:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kolos1, 2017-05-29 10:09:27]

Description was changed from

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
==========

to

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
TEST=PasswordManagerTest.InPageNavigation
==========

[kolos1, 2017-05-29 10:13:48]

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

[commit-bot: I haz the power, 2017-05-29 10:13:53]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-05-29 10:14:12]

Patchset #3 (id:280001) has been deleted

[commit-bot: I haz the power, 2017-05-29 10:16:22]

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

[commit-bot: I haz the power, 2017-05-29 10:16:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kolos1, 2017-05-29 10:19:34]

kolos@chromium.org changed reviewers:
+ dvadym@chromium.org, melandory@chromium.org, vasilii@chromium.org

[kolos1, 2017-05-29 10:19:35]

vasilii@chromium.org: Please take primary review of the whole CL except ios/*
files.

melandory@chromium.org: Please review changes in ios/* files

dvadym@chromium.org: Please double check changes, especially, in
password_manager.cc

[kolos1, 2017-05-29 10:24:18]

Patchset #3 (id:300001) has been deleted

[kolos1, 2017-05-29 10:24:57]

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

[commit-bot: I haz the power, 2017-05-29 10:25:02]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-05-29 10:28:54]

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

[commit-bot: I haz the power, 2017-05-29 10:29:02]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-05-29 10:30:36]

Patchset #3 (id:320001) has been deleted

[commit-bot: I haz the power, 2017-05-29 11:19:46]

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

[commit-bot: I haz the power, 2017-05-29 11:19:47]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-05-29 13:15:49]

Can you clarify "If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. ".
Why is there no matching PasswordFormManager?

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_password_form_manager.h
(right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager.h:56:
~CredentialManagerPasswordFormManager() override;
you should move the method in the cc file

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:6:
#include <base/memory/ref_counted.h>
Improper place and style for the include (no <>)

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:8: #include
<base/memory/ref_counted.h>
Should go down

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:30: using
base::RefCounted;
Seems mostly not used.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:268: friend
class base::RefCounted<PasswordFormManager>;
What is the point in making a base class friend?

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:409:
scoped_refptr<PasswordFormManager> form_manager(new PasswordFormManager(
Prefer base::MakeRefCounted to new

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:242:
make_scoped_refptr<PasswordFormManager>(new PasswordFormManager(
MakeRefCounted

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:356: manager =
*matched_manager_it;
Can be done in one statement

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

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:267:
DoAll(WithArg<0>(SaveArg<0>(&form_manager_to_save)), Return(false)));
No need for WithArg<0>. Maybe it will work without Return(false) as well.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:596:
EXPECT_CALL(driver_, FillPasswordForm(_)).Times(3);
What has changed?

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:942:
.Times(2)
Why twice?

[kolos1, 2017-05-29 13:51:03]

Description was changed from

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: If the login has failed, there is no matched
PasswordFormManager in |pending_login_managers_| and we need to re-create it. 

BUG=725883
TEST=PasswordManagerTest.InPageNavigation
==========

to

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: the matched PasswordFormManager is removed
from |pending_login_managers_| on form submission
(https://cs.chromium.org/chromium/src/components/password_manager/core/browser...).
If the login has failed, we might still need that PasswordFormManager and have
to re-create it.

BUG=725883
TEST=PasswordManagerTest.InPageNavigation
==========

[kolos1, 2017-05-29 14:17:02]

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

[commit-bot: I haz the power, 2017-05-29 14:17:12]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-05-29 14:47:10]

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

[commit-bot: I haz the power, 2017-05-29 14:47:24]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-05-29 15:35:33]

Patchset #4 (id:360001) has been deleted

[kolos1, 2017-05-29 15:37:10]

Description was changed from

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: the matched PasswordFormManager is removed
from |pending_login_managers_| on form submission
(https://cs.chromium.org/chromium/src/components/password_manager/core/browser...).
If the login has failed, we might still need that PasswordFormManager and have
to re-create it.

BUG=725883
TEST=PasswordManagerTest.InPageNavigation
==========

to

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: the matched PasswordFormManager is removed
from |pending_login_managers_| on form submission
(https://cs.chromium.org/chromium/src/components/password_manager/core/browser...).
If the login has failed, we might still need that PasswordFormManager in
|pending_login_managers_|  and have to re-create it.

BUG=725883
TEST=PasswordManagerTest.InPageNavigation
==========

[kolos1, 2017-05-29 15:47:06]

Hi Vasilii,

Made changes addressed to your comments. CL description is changed. Please
review.

Regards,
Maxim

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_password_form_manager.h
(right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager.h:56:
~CredentialManagerPasswordFormManager() override;
On 2017/05/29 13:15:48, vasilii wrote:
> you should move the method in the cc file

Done.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc:6:
#include <base/memory/ref_counted.h>
On 2017/05/29 13:15:48, vasilii wrote:
> Improper place and style for the include (no <>)

Done.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:8: #include
<base/memory/ref_counted.h>
On 2017/05/29 13:15:48, vasilii wrote:
> Should go down

Done.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:30: using
base::RefCounted;
On 2017/05/29 13:15:48, vasilii wrote:
> Seems mostly not used.

Removed.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:268: friend
class base::RefCounted<PasswordFormManager>;
On 2017/05/29 13:15:48, vasilii wrote:
> What is the point in making a base class friend?

It is required for using RefCounted
(https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?rcl=a71587c587...).

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:409:
scoped_refptr<PasswordFormManager> form_manager(new PasswordFormManager(
On 2017/05/29 13:15:48, vasilii wrote:
> Prefer base::MakeRefCounted to new

Done.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:242:
make_scoped_refptr<PasswordFormManager>(new PasswordFormManager(
On 2017/05/29 13:15:49, vasilii wrote:
> MakeRefCounted

Done.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:356: manager =
*matched_manager_it;
On 2017/05/29 13:15:49, vasilii wrote:
> Can be done in one statement

Done.

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

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:267:
DoAll(WithArg<0>(SaveArg<0>(&form_manager_to_save)), Return(false)));
On 2017/05/29 13:15:49, vasilii wrote:
> No need for WithArg<0>. Maybe it will work without Return(false) as well.

WithArg is not needed, but w/o "Return(false)" there is an error: 
../../third_party/googletest/src/googlemock/include/gmock/gmock-more-actions.h:173:1:
error: control reaches end of non-void function [-Werror,-Wreturn-type]

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:596:
EXPECT_CALL(driver_, FillPasswordForm(_)).Times(3);
On 2017/05/29 13:15:49, vasilii wrote:
> What has changed?

In |manager()->ProvisionallySavePassword(observed_form, nullptr)|, we don't
remove the matched PasswordFormManager from |pending_login_managers_| anymore.
So, after "submission" the form manager tries to fill the form. 

I believe EXPECT_CALL for FillPasswordForm doesn't test/check anything here, it
just suppresses unexpected call warnings.

https://codereview.chromium.org/2900693002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:942:
.Times(2)
On 2017/05/29 13:15:49, vasilii wrote:
> Why twice?

We don't remove the matched PasswordFormManager from |pending_login_managers_|
anymore. This PasswordFormManager calls GetLogins after in-page navigation. 

form_manager_to_save->Save() =>  password_manager_->UpdateFormManagers() =>
form_manager->form_fetcher()->Fetch() => 
password_store->GetLogins(form_digest_, this);

[commit-bot: I haz the power, 2017-05-29 15:52:15]

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

[commit-bot: I haz the power, 2017-05-29 15:52:16]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-05-30 08:14:36]

lgtm

[dvadym, 2017-05-30 09:12:50]

LGTM with nit

https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (left):

https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:269:
std::move(form_to_save));
Nit: We can remove a call of std::move

[kolos1, 2017-05-30 09:21:24]

https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (left):

https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:269:
std::move(form_to_save));
On 2017/05/30 09:12:50, dvadym wrote:
> Nit: We can remove a call of std::move

Thanks for review Vadym.

I would prefer to keep it as it is. In the old code, it was supposed that
|form_to_save| has no value after this line. Let's keep it.

[kolos1, 2017-05-30 11:05:06]

On 2017/05/30 09:21:24, kolos1 wrote:
>
https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
> File chrome/browser/password_manager/chrome_password_manager_client.cc (left):
> 
>
https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
> chrome/browser/password_manager/chrome_password_manager_client.cc:269:
> std::move(form_to_save));
> On 2017/05/30 09:12:50, dvadym wrote:
> > Nit: We can remove a call of std::move
> 
> Thanks for review Vadym.
> 
> I would prefer to keep it as it is. In the old code, it was supposed that
> |form_to_save| has no value after this line. Let's keep it.

melandory@: friendly ping. Please review ios files.

[melandory, 2017-05-30 16:09:22]

On 2017/05/30 11:05:06, kolos1 wrote:
> On 2017/05/30 09:21:24, kolos1 wrote:
> >
>
https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
> > File chrome/browser/password_manager/chrome_password_manager_client.cc
(left):
> > 
> >
>
https://codereview.chromium.org/2900693002/diff/380001/chrome/browser/passwor...
> > chrome/browser/password_manager/chrome_password_manager_client.cc:269:
> > std::move(form_to_save));
> > On 2017/05/30 09:12:50, dvadym wrote:
> > > Nit: We can remove a call of std::move
> > 
> > Thanks for review Vadym.
> > 
> > I would prefer to keep it as it is. In the old code, it was supposed that
> > |form_to_save| has no value after this line. Let's keep it.
> 
> melandory@: friendly ping. Please review ios files.

lgtm

[kolos1, 2017-05-30 16:29:17]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2017-05-30 16:29:38]

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, 2017-05-30 17:32:15]

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

[commit-bot: I haz the power, 2017-05-30 17:32:18]

Failed to apply patch for
components/password_manager/core/browser/password_form_manager_unittest.cc:
While running git apply --index -3 -p1;
  error: patch failed:
components/password_manager/core/browser/password_form_manager_unittest.cc:3177
  Falling back to three-way merge...
  Applied patch to
'components/password_manager/core/browser/password_form_manager_unittest.cc'
with conflicts.
  U components/password_manager/core/browser/password_form_manager_unittest.cc

Patch:      
components/password_manager/core/browser/password_form_manager_unittest.cc
Index:
components/password_manager/core/browser/password_form_manager_unittest.cc
diff --git
a/components/password_manager/core/browser/password_form_manager_unittest.cc
b/components/password_manager/core/browser/password_form_manager_unittest.cc
index
5a56528b47c67a2f7de94364b6d4cb850b128b54..f6130ae76e549039b3234e6fd5b28eb0d5f71192
100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -387,9 +387,9 @@ class PasswordFormManagerTest : public testing::Test {
     saved_match_.form_data.fields.push_back(field);
 
     password_manager_.reset(new PasswordManager(&client_));
-    form_manager_.reset(new PasswordFormManager(
+    form_manager_ = base::MakeRefCounted<PasswordFormManager>(
         password_manager_.get(), &client_, client_.driver(), observed_form_,
-        base::MakeUnique<NiceMock<MockFormSaver>>(), &fake_form_fetcher_));
+        base::MakeUnique<NiceMock<MockFormSaver>>(), &fake_form_fetcher_);
   }
 
   // Save saved_match() for observed_form() where |observed_form_data|,
@@ -406,9 +406,10 @@ class PasswordFormManagerTest : public testing::Test {
 
     FakeFormFetcher fetcher;
     fetcher.Fetch();
-    PasswordFormManager form_manager(
-        password_manager(), client(), client()->driver(), form,
-        base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher);
+    scoped_refptr<PasswordFormManager> form_manager(
+        base::MakeRefCounted<PasswordFormManager>(
+            password_manager(), client(), client()->driver(), form,
+            base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher));
     PasswordForm match = CreateSavedMatch(false);
     match.generation_upload_status = status;
     match.times_used = times_used;
@@ -437,8 +438,8 @@ class PasswordFormManagerTest : public testing::Test {
     if (field_type) {
       // Show the password generation popup to check that the generation vote
       // would be ignored.
-      form_manager.set_generation_element(saved_match()->password_element);
-      form_manager.set_generation_popup_was_shown(true);
+      form_manager->set_generation_element(saved_match()->password_element);
+      form_manager->set_generation_popup_was_shown(true);
       expect_generation_vote =
           *field_type != autofill::ACCOUNT_CREATION_PASSWORD;
 
@@ -459,9 +460,9 @@ class PasswordFormManagerTest : public testing::Test {
                   StartUploadRequest(_, _, _, _, _))
           .Times(0);
     }
-    form_manager.ProvisionallySave(
+    form_manager->ProvisionallySave(
         form_to_save, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
-    form_manager.Save();
+    form_manager->Save();
     Mock::VerifyAndClearExpectations(
         client()->mock_driver()->mock_autofill_download_manager());
   }
@@ -495,9 +496,10 @@ class PasswordFormManagerTest : public testing::Test {
 
     FakeFormFetcher fetcher;
     fetcher.Fetch();
-    PasswordFormManager form_manager(
-        password_manager(), client(), client()->driver(), *observed_form(),
-        base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher);
+    scoped_refptr<PasswordFormManager> form_manager(
+        base::MakeRefCounted<PasswordFormManager>(
+            password_manager(), client(), client()->driver(), *observed_form(),
+            base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher));
     fetcher.SetNonFederated({saved_match()}, 0u);
 
     // User submits current and new credentials to the observed form.
@@ -507,21 +509,21 @@ class PasswordFormManagerTest : public testing::Test {
     submitted_form.password_value = saved_match()->password_value;
     submitted_form.new_password_value = ASCIIToUTF16("test2");
     submitted_form.preferred = true;
-    form_manager.ProvisionallySave(
+    form_manager->ProvisionallySave(
         submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
 
     // Successful login. The PasswordManager would instruct PasswordFormManager
     // to update.
-    EXPECT_FALSE(form_manager.IsNewLogin());
+    EXPECT_FALSE(form_manager->IsNewLogin());
     EXPECT_FALSE(
-        form_manager.is_possible_change_password_form_without_username());
+        form_manager->is_possible_change_password_form_without_username());
 
     // By now, the PasswordFormManager should have promoted the new password
     // value already to be the current password, and should no longer maintain
     // any info about the new password value.
     EXPECT_EQ(submitted_form.new_password_value,
-              form_manager.pending_credentials().password_value);
-    EXPECT_TRUE(form_manager.pending_credentials().new_password_value.empty());
+              form_manager->pending_credentials().password_value);
+   
EXPECT_TRUE(form_manager->pending_credentials().new_password_value.empty());
 
     std::map<base::string16, autofill::ServerFieldType> expected_types;
     expected_types[ASCIIToUTF16("full_name")] = autofill::UNKNOWN_TYPE;
@@ -557,13 +559,13 @@ class PasswordFormManagerTest : public testing::Test {
 
     switch (field_type) {
       case autofill::NEW_PASSWORD:
-        form_manager.Update(*saved_match());
+        form_manager->Update(*saved_match());
         break;
       case autofill::PROBABLY_NEW_PASSWORD:
-        form_manager.OnNoInteraction(true /* it is an update */);
+        form_manager->OnNoInteraction(true /* it is an update */);
         break;
       case autofill::NOT_NEW_PASSWORD:
-        form_manager.OnNopeUpdateClicked();
+        form_manager->OnNopeUpdateClicked();
         break;
       default:
         NOTREACHED();
@@ -638,9 +640,10 @@ class PasswordFormManagerTest : public testing::Test {
 
     FakeFormFetcher fetcher;
     fetcher.Fetch();
-    PasswordFormManager form_manager(
-        password_manager(), client(), client()->driver(), form,
-        base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher);
+    scoped_refptr<PasswordFormManager> form_manager(
+        base::MakeRefCounted<PasswordFormManager>(
+            password_manager(), client(), client()->driver(), form,
+            base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher));
 
     fetcher.SetNonFederated(std::vector<const PasswordForm*>(), 0u);
 
@@ -649,13 +652,13 @@ class PasswordFormManagerTest : public testing::Test {
     if (interaction == SAVE)
       expected_available_field_types.insert(autofill::PASSWORD);
 
-    form_manager.set_is_manual_generation(is_manual_generation);
+    form_manager->set_is_manual_generation(is_manual_generation);
     base::string16 generation_element = is_change_password_form
                                             ? form.new_password_element
                                             : form.password_element;
-    form_manager.set_generation_element(generation_element);
-    form_manager.set_generation_popup_was_shown(true);
-    form_manager.set_has_generated_password(has_generated_password);
+    form_manager->set_generation_element(generation_element);
+    form_manager->set_generation_popup_was_shown(true);
+    form_manager->set_has_generated_password(has_generated_password);
 
     // Figure out expected generation event type.
     autofill::AutofillUploadContents::Field::PasswordGenerationType
@@ -676,17 +679,17 @@ class PasswordFormManagerTest : public testing::Test {
                 form_structure.FormSignatureAsStr(),
expected_generation_types),
             false, expected_available_field_types, std::string(), true));
 
-    form_manager.ProvisionallySave(
+    form_manager->ProvisionallySave(
         submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
     switch (interaction) {
       case SAVE:
-        form_manager.Save();
+        form_manager->Save();
         break;
       case NEVER:
-        form_manager.OnNeverClicked();
+        form_manager->OnNeverClicked();
         break;
       case NO_INTERACTION:
-        form_manager.OnNoInteraction(false /* not an update prompt*/);
+        form_manager->OnNoInteraction(false /* not an update prompt*/);
         break;
     }
     Mock::VerifyAndClearExpectations(
@@ -725,7 +728,7 @@ class PasswordFormManagerTest : public testing::Test {
   // Define |fake_form_fetcher_| before |form_manager_|, because the former
   // needs to outlive the latter.
   FakeFormFetcher fake_form_fetcher_;
-  std::unique_ptr<PasswordFormManager> form_manager_;
+  scoped_refptr<PasswordFormManager> form_manager_;
 };
 
 class PasswordFormManagerFillOnAccountSelectTest
@@ -844,9 +847,10 @@ TEST_F(PasswordFormManagerTest, TestBlacklistMatching) {
   observed_form()->signon_realm = "http://accounts.google.com";
   FakeFormFetcher fetcher;
   fetcher.Fetch();
-  PasswordFormManager form_manager(password_manager(), client(),
-                                   client()->driver(), *observed_form(),
-                                   base::MakeUnique<MockFormSaver>(),
&fetcher);
+  scoped_refptr<PasswordFormManager> form_manager(
+      base::MakeRefCounted<PasswordFormManager>(
+          password_manager(), client(), client()->driver(), *observed_form(),
+          base::MakeUnique<MockFormSaver>(), &fetcher));
 
   // Doesn't apply because it is just a PSL match of the observed form.
   PasswordForm blacklisted_psl = *observed_form();
@@ -889,12 +893,12 @@ TEST_F(PasswordFormManagerTest, TestBlacklistMatching) {
                       …
(message too large)

[kolos1, 2017-05-31 11:24:09]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2017-05-31 11:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, dvadym@chromium.org, melandory@chromium.org
Link to the patchset: https://codereview.chromium.org/2900693002/#ps400001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-31 11:24:30]

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, 2017-05-31 12:29:00]

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

[commit-bot: I haz the power, 2017-05-31 12:29:02]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kolos1, 2017-05-31 12:30:26]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2017-05-31 12:30:41]

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, 2017-05-31 12:42:58]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1496233826898620,
"parent_rev": "476d6743b4fc42dd2a2bcf3664642c4884a70cc0", "commit_rev":
"b31c9aa77f4ab68acbfc4b2a01216ec0f42e07c9"}

[commit-bot: I haz the power, 2017-05-31 12:43:10]

Description was changed from

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: the matched PasswordFormManager is removed
from |pending_login_managers_| on form submission
(https://cs.chromium.org/chromium/src/components/password_manager/core/browser...).
If the login has failed, we might still need that PasswordFormManager in
|pending_login_managers_|  and have to re-create it.

BUG=725883
TEST=PasswordManagerTest.InPageNavigation
==========

to

==========
[Password Manager] Convert |pending_login_managers_| to an array of
scoped_refptr 

Before this CL, |pending_login_managers_| was an array of unique_ptr. So, we
have to pass ownership to | PasswordsClientUIDelegate|. When manual fallback for
password saving is implemented, both PasswordManager and
PasswordsClientUIDelegate should have access to the PasswordFormManager.

Another issue that this CL fixes: the matched PasswordFormManager is removed
from |pending_login_managers_| on form submission
(https://cs.chromium.org/chromium/src/components/password_manager/core/browser...).
If the login has failed, we might still need that PasswordFormManager in
|pending_login_managers_|  and have to re-create it.

BUG=725883
TEST=PasswordManagerTest.InPageNavigation

Review-Url: https://codereview.chromium.org/2900693002
Cr-Commit-Position: refs/heads/master@{#475889}
Committed:
https://chromium.googlesource.com/chromium/src/+/b31c9aa77f4ab68acbfc4b2a0121...
==========

[commit-bot: I haz the power, 2017-05-31 12:43:12]

Committed patchset #5 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/b31c9aa77f4ab68acbfc4b2a0121...

[vabr (Chromium), 2017-06-05 11:41:32]

I unfortunately missed landing this CL by a couple of days due to my absence.

I'm afraid this approach does not LGTM, at least not until I'm sure we
considered design alternatives.

Shared ownership comes at the cost of unclear time and place of deletion. We are
evading the issue of two places needing to access PFM by giving up the knowledge
who and where destroys the PFM.

The concern which this CL tries to resolve is that both
PasswordsClientUIDelegate and PasswordManager need the information contained in
the PFM at overlapping time intervals. One potential way to resolve this issue
other than share the ownership of the PFM would be to split the PFM into two
smaller classes dedicated to the different functions it fulfills in
PasswordsClientUIDelegate and in PasswordManager. Then, instead of sharing a
single PFM, both PasswordsClientUIDelegate and PasswordManager could use the
appropriate class, and the class for the former would be generated from the
class for the latter instead of passing the PFM.

Let's discuss this during this week in person.

Thanks,
Vaclav

[vabr (Chromium), 2017-06-07 08:09:34]

A revert of this CL (patchset #5 id:400001) has been created in
https://codereview.chromium.org/2926833002/ by vabr@chromium.org.

The reason for reverting is: I am very sorry for reverting this CL. My reasons
are described in detail in https://crbug.com/725883#c3, in summary they are:
(1) the design can be made to satisfy the requirements without refcounting
(2) there is some urgency in removing the refcounting from the codebase before
people start building on it and it will become difficult to remove.

Cheers,
Vaclav.