Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The PSL matches are filtered out.
The decision to filter the Android federated credentials in PasswordStore was taken in https://codereview.chromium.org/1385563007. It was wrong for the long term.

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

[vasilii, 2016-11-22 18:52:57]

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

[commit-bot: I haz the power, 2016-11-22 18:53:22]

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-22 18:54:06]

Description was changed from

==========
Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The decision to filter the Android federated credentials in PasswordStore was
taken in https://codereview.chromium.org/1385563007. It was wrong in the long
term.

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

to

==========
Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The PSL matches are filtered out.
The decision to filter the Android federated credentials in PasswordStore was
taken in https://codereview.chromium.org/1385563007. It was wrong for the long
term.

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

[vasilii, 2016-11-22 18:54:06]

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

[vasilii, 2016-11-22 18:56:53]

Hi guys,

please review the CL.

[commit-bot: I haz the power, 2016-11-22 19:14:28]

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

[commit-bot: I haz the power, 2016-11-22 19:14:28]

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

[vabr (Chromium), 2016-11-23 08:57:56]

Thanks, Vasilii. My main question is:
How do we make sure that the issue reported in http://crbug.com/539833 does not
happen again? Is there a test guarding that?

Cheers,
Vaclav

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(left):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:573:
form_.signon_realm = "this is a realm";
I'm curious why this needed to be removed. If I read the code correctly, the
signon_realm, unless overridden here, will be some web URL. If this is important
for the test, should we document it somehow (explicitly setting it, ASSERTing
its value, or just commenting on it)?

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:113:
std::vector<std::unique_ptr<autofill::PasswordForm>> psl_results;
The CL description says that the PSL results are filtered out, and indeed, the
code only pushes_back to |psl_results| but never uses what is stored there.

Why cannot we just keep those in |results| and execute continue; on line 132?
Alternatively, what about calling |psl_results| something like
|filtered_out_results| to make it clearer that it is a sink vector?

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.h
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.h:76:
std::vector<std::string> federations_;
I'm not sure I understand why you made this a vector instead of a set, when it
is used for lookups (forcing you to use sort and binary_search explicitly).

We should also comment here that the vector is kept sorted if we want to keep it
that way.

[vasilii, 2016-11-23 09:40:21]

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

[commit-bot: I haz the power, 2016-11-23 09:40:32]

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-23 09:40:54]

What you asked is tested by FormFetcherImplTest.Federated and
FormFetcherImplTest.Mixed

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(left):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:573:
form_.signon_realm = "this is a realm";
On 2016/11/23 08:57:56, vabr (Chromium) wrote:
> I'm curious why this needed to be removed. If I read the code correctly, the
> signon_realm, unless overridden here, will be some web URL. If this is
important
> for the test, should we document it somehow (explicitly setting it, ASSERTing
> its value, or just commenting on it)?

|form_| has already correct origin and signon_realm after SetUp(). The test
overwrites |origin| so that it's different from the current URL.
"this is a realm" is obviously incorrect and can't be used anywhere.

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:113:
std::vector<std::unique_ptr<autofill::PasswordForm>> psl_results;
On 2016/11/23 08:57:56, vabr (Chromium) wrote:
> The CL description says that the PSL results are filtered out, and indeed, the
> code only pushes_back to |psl_results| but never uses what is stored there.
> 
> Why cannot we just keep those in |results| and execute continue; on line 132?
> Alternatively, what about calling |psl_results| something like
> |filtered_out_results| to make it clearer that it is a sink vector?

We can but it's not a refactoring on its own. It's a preparation for fixing the
bug. In the next CL today or tomorrow |psl_results| is to be used below.

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.h
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.h:76:
std::vector<std::string> federations_;
On 2016/11/23 08:57:56, vabr (Chromium) wrote:
> I'm not sure I understand why you made this a vector instead of a set, when it
> is used for lookups (forcing you to use sort and binary_search explicitly).
> 
> We should also comment here that the vector is kept sorted if we want to keep
it
> that way.

sorted vector is faster than the set. An alternative would be unordered_set. For
0-2 values it also will be slower.

[vabr (Chromium), 2016-11-23 09:51:03]

Thanks, Vasilii.

Good point about the FormFetcherImplTests checking the handling of federated
forms, that sounds good.

The CL LGTM, although I still have one point to discuss about the set vs. vector
below.

Cheers,
Vaclav

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:113:
std::vector<std::unique_ptr<autofill::PasswordForm>> psl_results;
On 2016/11/23 09:40:54, vasilii wrote:
> On 2016/11/23 08:57:56, vabr (Chromium) wrote:
> > The CL description says that the PSL results are filtered out, and indeed,
the
> > code only pushes_back to |psl_results| but never uses what is stored there.
> > 
> > Why cannot we just keep those in |results| and execute continue; on line
132?
> > Alternatively, what about calling |psl_results| something like
> > |filtered_out_results| to make it clearer that it is a sink vector?
> 
> We can but it's not a refactoring on its own. It's a preparation for fixing
the
> bug. In the next CL today or tomorrow |psl_results| is to be used below.

Acknowledged.

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.h
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.h:76:
std::vector<std::string> federations_;
On 2016/11/23 09:40:54, vasilii wrote:
> On 2016/11/23 08:57:56, vabr (Chromium) wrote:
> > I'm not sure I understand why you made this a vector instead of a set, when
it
> > is used for lookups (forcing you to use sort and binary_search explicitly).
> > 
> > We should also comment here that the vector is kept sorted if we want to
keep
> it
> > that way.
> 
> sorted vector is faster than the set. An alternative would be unordered_set.
For
> 0-2 values it also will be slower.

Thanks for explaining!

So this is the tradeoff for using vector:
Benefit -- better performance
Drawback -- increased fragility and verbosity of the code; if future changes
miss the requirement of keeping the vector sorted, the code will have bugs

Do we know how much performance gain we get from using vector? To me it seems
neither like a hotspot, nor like we expect large enough sizes to make a
difference here.

I'm happy to stand corrected on that observation, but until I do, I'd suggest to
have an unordered or just plain set to the vector.

[vasilii, 2016-11-23 10:15:13]

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.h
(right):

https://codereview.chromium.org/2517993004/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.h:76:
std::vector<std::string> federations_;
On 2016/11/23 09:51:03, vabr (Chromium) wrote:
> On 2016/11/23 09:40:54, vasilii wrote:
> > On 2016/11/23 08:57:56, vabr (Chromium) wrote:
> > > I'm not sure I understand why you made this a vector instead of a set,
when
> it
> > > is used for lookups (forcing you to use sort and binary_search
explicitly).
> > > 
> > > We should also comment here that the vector is kept sorted if we want to
> keep
> > it
> > > that way.
> > 
> > sorted vector is faster than the set. An alternative would be unordered_set.
> For
> > 0-2 values it also will be slower.
> 
> Thanks for explaining!
> 
> So this is the tradeoff for using vector:
> Benefit -- better performance
> Drawback -- increased fragility and verbosity of the code; if future changes
> miss the requirement of keeping the vector sorted, the code will have bugs
> 
> Do we know how much performance gain we get from using vector? To me it seems
> neither like a hotspot, nor like we expect large enough sizes to make a
> difference here.
> 
> I'm happy to stand corrected on that observation, but until I do, I'd suggest
to
> have an unordered or just plain set to the vector. 

Reverted.

[vasilii, 2016-11-23 10:15:18]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-11-23 10:15:18]

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/2517993004/#ps80001
(title: "use set")

[commit-bot: I haz the power, 2016-11-23 10:15:29]

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-23 10:53:02]

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

[commit-bot: I haz the power, 2016-11-23 10:53:03]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vasilii, 2016-11-23 11:00:31]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2016-11-23 11:00:48]

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-23 11:46:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479898831721760,
"parent_rev": "b197590c4a66e4437e5ad7a45e1f46bf8d436a1b", "commit_rev":
"8c64e6e30ae654e044e331f1393f108b6d94e522"}

[commit-bot: I haz the power, 2016-11-23 11:47:04]

Description was changed from

==========
Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The PSL matches are filtered out.
The decision to filter the Android federated credentials in PasswordStore was
taken in https://codereview.chromium.org/1385563007. It was wrong for the long
term.

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

to

==========
Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The PSL matches are filtered out.
The decision to filter the Android federated credentials in PasswordStore was
taken in https://codereview.chromium.org/1385563007. It was wrong for the long
term.

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

[commit-bot: I haz the power, 2016-11-23 11:47:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-23 11:48:53]

Description was changed from

==========
Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The PSL matches are filtered out.
The decision to filter the Android federated credentials in PasswordStore was
taken in https://codereview.chromium.org/1385563007. It was wrong for the long
term.

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

to

==========
Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

The PSL matches are filtered out.
The decision to filter the Android federated credentials in PasswordStore was
taken in https://codereview.chromium.org/1385563007. It was wrong for the long
term.

BUG=666340

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

[commit-bot: I haz the power, 2016-11-23 11:48:54]

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