Refactor CredentialManagerImpl::RequireUserMediation method.

Refactor CredentialManagerImpl::RequireUserMediation method.

CredentialManagerImpl shouldn't use PasswordStore::GetAutofillableLogins. It's not effective. Instead
GetLogins() should be used like in the PasswordFormManager.

BUG=666340
Committed: https://crrev.com/8f99dc049b44da99478c2d714834960a859afbff
Cr-Commit-Position: refs/heads/master@{#433826}

[vasilii, 2016-11-18 17:38:16]

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

[commit-bot: I haz the power, 2016-11-18 17:39:07]

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

[vasilii, 2016-11-18 17:42:04]

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

[vasilii, 2016-11-18 17:42:04]

Hi all,

please review.
Jan, I added you because you'll need to know this code.

[commit-bot: I haz the power, 2016-11-18 18:03:24]

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

[commit-bot: I haz the power, 2016-11-18 18:03:25]

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...)

[vabr (Chromium), 2016-11-21 09:39:06]

LGTM, thanks for this great simplification and clean-up.

I have no requests for code change, just one question for my own curiosity, and
a note on why I think a particular code change is correct (for cross-checking).
I'm not sure how we ended up duplicating the affiliation and PSL matching in the
credential manager, or even if it was unnecessary from the beginning.

Cheers,
Vaclav

https://codereview.chromium.org/2513033002/diff/1/components/password_manager...
File
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc
(right):

https://codereview.chromium.org/2513033002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc:19:
void CredentialManagerPendingRequireUserMediationTask::AddOrigin(
Just out of curiosity: In which situations is this called multiple times, with
different origins, within the same WebContents?

https://codereview.chromium.org/2513033002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc:21:
delegate_->GetPasswordStore()->GetLogins(form_digest, this);
I acknowledge that this GetLogins call will result in the same set of
PSL-matched credentials as the filtering in the old code did: they both boil
down to the registry controlled domain of
web_contents()->GetLastCommittedURL().GetOrigin().

I also acknowledge that the GetLogins call involves both the PSL matching and
the affiliation matching, so it's OK to drop it here.

[vasilii, 2016-11-21 18:56:50]

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

[vasilii, 2016-11-21 18:57:09]

Jan?

https://codereview.chromium.org/2513033002/diff/1/components/password_manager...
File
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc
(right):

https://codereview.chromium.org/2513033002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc:19:
void CredentialManagerPendingRequireUserMediationTask::AddOrigin(
On 2016/11/21 09:39:06, vabr (Chromium) wrote:
> Just out of curiosity: In which situations is this called multiple times, with
> different origins, within the same WebContents?

WebContents can change the URL. In theory.

https://codereview.chromium.org/2513033002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.cc:21:
delegate_->GetPasswordStore()->GetLogins(form_digest, this);
On 2016/11/21 09:39:06, vabr (Chromium) wrote:
> I acknowledge that this GetLogins call will result in the same set of
> PSL-matched credentials as the filtering in the old code did: they both boil
> down to the registry controlled domain of
> web_contents()->GetLastCommittedURL().GetOrigin().
> 
> I also acknowledge that the GetLogins call involves both the PSL matching and
> the affiliation matching, so it's OK to drop it here.

I also acknowledge :-)

[commit-bot: I haz the power, 2016-11-21 18:57:39]

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, 2016-11-21 19:42:25]

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

[commit-bot: I haz the power, 2016-11-21 19:42:26]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2016-11-21 22:45:35]

LGTM, although I must admit I'm very inexperienced with this part of the
codebase. Could you elaborate on how exactly I would test this? Thanks a lot!

https://codereview.chromium.org/2513033002/diff/20001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h
(right):

https://codereview.chromium.org/2513033002/diff/20001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h:39:
void OnGetPasswordStoreResults(
Could you explain why this is private now?

[vasilii, 2016-11-22 09:39:22]

Vaclav, originally GetLogins() didn't read the federated credentials. That's why
the implementation was like this.
Jan, the relevant unit test is
CredentialManagerImplTest.CredentialManagerOnRequireUserMediation.

https://codereview.chromium.org/2513033002/diff/20001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h
(right):

https://codereview.chromium.org/2513033002/diff/20001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_require_user_mediation_task.h:39:
void OnGetPasswordStoreResults(
On 2016/11/21 22:45:35, jdoerrie wrote:
> Could you explain why this is private now?

It's only called via PasswordStoreConsumer interface.

[vasilii, 2016-11-22 09:39:52]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-11-22 09:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2513033002/#ps20001
(title: "ios")

[commit-bot: I haz the power, 2016-11-22 09:40:01]

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-11-22 09:43:15]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479807592590260,
"parent_rev": "0c5549ae9c4e66b32c9e9b581a7e4d07f7d3d31d", "commit_rev":
"326db3db51f082ab7e5395eeb42bd1d2bff4a7fe"}

[commit-bot: I haz the power, 2016-11-22 09:43:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-22 09:46:02]

Description was changed from

==========
Refactor CredentialManagerImpl::RequireUserMediation method.

CredentialManagerImpl shouldn't use PasswordStore::GetAutofillableLogins. It's
not effective. Instead
GetLogins() should be used like in the PasswordFormManager.

BUG=666340
==========

to

==========
Refactor CredentialManagerImpl::RequireUserMediation method.

CredentialManagerImpl shouldn't use PasswordStore::GetAutofillableLogins. It's
not effective. Instead
GetLogins() should be used like in the PasswordFormManager.

BUG=666340

Committed: https://crrev.com/8f99dc049b44da99478c2d714834960a859afbff
Cr-Commit-Position: refs/heads/master@{#433826}
==========

[commit-bot: I haz the power, 2016-11-22 09:46:03]

Patchset 2 (id:??) landed as
https://crrev.com/8f99dc049b44da99478c2d714834960a859afbff
Cr-Commit-Position: refs/heads/master@{#433826}

[vabr (Chromium), 2016-11-22 10:53:39]

On 2016/11/22 09:39:22, vasilii wrote:
> Vaclav, originally GetLogins() didn't read the federated credentials. That's
why
> the implementation was like this.

Thanks for the explanation!

Cheers,
Vaclav