Show source of affiliated matches in the drop-down password suggestions list.

Show source of affiliated matches in the drop-down password suggestions list.

Also this CL addresses comments to https://codereview.chromium.org/1314903003.

BUG=359315, 540611
Committed: https://crrev.com/fcc76c56fdc1021790488c16e0df7b5877442ac4
Cr-Commit-Position: refs/heads/master@{#355055}

[dvadym, 2015-10-12 09:31:28]

dvadym@chromium.org changed reviewers:
+ engedy@chromium.org

[dvadym, 2015-10-12 09:32:51]

Hi Balazs,

 Could you please review this CL?

Best regards,
Vadym

[engedy, 2015-10-12 10:38:19]

LGTM. I'd like to see a follow-up CL with some tests, however, to verify that
the |preferred_realm| in PasswordFormFillData is correctly filled when it ought
to be. I'd put one unit test in password_form_field_data_unittest.cc, and we
should really have an integration-level test too, but I'm not really sure what
would be a good location, though. This latter test should verify the entire
chain of events starting from an Android credential in the PasswordStore, to
this getting autofilled with the |preferred_realm| set.

Also, please change the description of the CL to something more descriptive, and
mention that it also fixes an issue that the source of affiliated matches was no
longer shown in the drop-down suggestions list.

[engedy, 2015-10-13 10:43:43]

On 2015/10/12 10:38:19, engedy wrote:
> LGTM. I'd like to see a follow-up CL with some tests, however, to verify that
> the |preferred_realm| in PasswordFormFillData is correctly filled when it
ought
> to be. I'd put one unit test in password_form_field_data_unittest.cc, and we
> should really have an integration-level test too, but I'm not really sure what
> would be a good location, though. This latter test should verify the entire
> chain of events starting from an Android credential in the PasswordStore, to
> this getting autofilled with the |preferred_realm| set.
> 
> Also, please change the description of the CL to something more descriptive,
and
> mention that it also fixes an issue that the source of affiliated matches was
no
> longer shown in the drop-down suggestions list.

Vadym, one more question: I could not find where we set the
|is_affiliation_based_match| to true for affiliated matches? Do we?

[dvadym, 2015-10-13 10:51:09]

On 2015/10/13 10:43:43, engedy wrote:
> On 2015/10/12 10:38:19, engedy wrote:
> > LGTM. I'd like to see a follow-up CL with some tests, however, to verify
that
> > the |preferred_realm| in PasswordFormFillData is correctly filled when it
> ought
> > to be. I'd put one unit test in password_form_field_data_unittest.cc, and we
> > should really have an integration-level test too, but I'm not really sure
what
> > would be a good location, though. This latter test should verify the entire
> > chain of events starting from an Android credential in the PasswordStore, to
> > this getting autofilled with the |preferred_realm| set.
> > 
> > Also, please change the description of the CL to something more descriptive,
> and
> > mention that it also fixes an issue that the source of affiliated matches
was
> no
> > longer shown in the drop-down suggestions list.
> 
> Vadym, one more question: I could not find where we set the
> |is_affiliation_based_match| to true for affiliated matches? Do we?

We do it here 
https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...

[engedy, 2015-10-13 10:52:59]

On 2015/10/13 10:51:09, dvadym wrote:
> On 2015/10/13 10:43:43, engedy wrote:
> > On 2015/10/12 10:38:19, engedy wrote:
> > > LGTM. I'd like to see a follow-up CL with some tests, however, to verify
> that
> > > the |preferred_realm| in PasswordFormFillData is correctly filled when it
> > ought
> > > to be. I'd put one unit test in password_form_field_data_unittest.cc, and
we
> > > should really have an integration-level test too, but I'm not really sure
> what
> > > would be a good location, though. This latter test should verify the
entire
> > > chain of events starting from an Android credential in the PasswordStore,
to
> > > this getting autofilled with the |preferred_realm| set.
> > > 
> > > Also, please change the description of the CL to something more
descriptive,
> > and
> > > mention that it also fixes an issue that the source of affiliated matches
> was
> > no
> > > longer shown in the drop-down suggestions list.
> > 
> > Vadym, one more question: I could not find where we set the
> > |is_affiliation_based_match| to true for affiliated matches? Do we?
> 
> We do it here 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...

But wait a second, that's a different code path, only used by the Credential
Manager API. That line is not executed for filling logins into web-based login
forms.

[dvadym, 2015-10-13 14:07:55]

Thanks Balazs for review! I've addressed your comments:
1.Tests added
2.Added settings is_affiliation=true in store

PTAL.

[engedy, 2015-10-14 10:47:50]

One more thing from another CL that we should address here:

https://codereview.chromium.org/1375883002/diff/20001/chrome/browser/password...

[engedy, 2015-10-14 10:58:26]

LGTM % comments, and the one fix from the other CL mentioned above. Thanks!

https://codereview.chromium.org/1401723002/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1401723002/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.h:268: // found using
affiliation-based match.
nit: matching.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:193: //
Create a match from the database that matches using public suffix.
nit: s/public suffix/.../

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:196:
preferred_match.action = GURL("https://foo.com/login");
Fields |action|, |username_element|, |password_element|, |submit_element| should
be empty for Android credentials.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:202:
preferred_match.signon_realm = "android://foo.com/";
This should be android://hash@com.foo.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:223: //
Create a match that was matched using public suffix, so
nit: Also s/public suffix/.../

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:228:
affiliated_match.action = GURL("https://foo1.com/login");
Please also remove surplus fields here.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:240: // Add
one exact match and one public suffix match.
nit: Here too

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

https://codereview.chromium.org/1401723002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1283:
form.origin = GURL("android://google.com");
Add the hash@ part here too. I'd also create the Android form from scratch, as
opposed to using MakeSimpleForm(), as most of the fields should not get filled.

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

https://codereview.chromium.org/1401723002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.cc:387: for
(PasswordForm* form : more_results.get())
nit: Do we really need get() here?

[dvadym, 2015-10-14 13:53:02]

Thanks Balazs! I've addressed your comments

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:193: //
Create a match from the database that matches using public suffix.
On 2015/10/14 10:58:26, engedy wrote:
> nit: s/public suffix/.../

Done.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:196:
preferred_match.action = GURL("https://foo.com/login");
On 2015/10/14 10:58:26, engedy wrote:
> Fields |action|, |username_element|, |password_element|, |submit_element|
should
> be empty for Android credentials.

Done.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:202:
preferred_match.signon_realm = "android://foo.com/";
On 2015/10/14 10:58:26, engedy wrote:
> This should be android://hash@com.foo.

Done.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:223: //
Create a match that was matched using public suffix, so
On 2015/10/14 10:58:26, engedy wrote:
> nit: Also s/public suffix/.../

Done.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:228:
affiliated_match.action = GURL("https://foo1.com/login");
On 2015/10/14 10:58:26, engedy wrote:
> Please also remove surplus fields here.

Done.

https://codereview.chromium.org/1401723002/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form_fill_data_unittest.cc:240: // Add
one exact match and one public suffix match.
On 2015/10/14 10:58:26, engedy wrote:
> nit: Here too

Done.

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

https://codereview.chromium.org/1401723002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:1283:
form.origin = GURL("android://google.com");
On 2015/10/14 10:58:26, engedy wrote:
> Add the hash@ part here too. I'd also create the Android form from scratch, as
> opposed to using MakeSimpleForm(), as most of the fields should not get
filled.

Done.

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

https://codereview.chromium.org/1401723002/diff/20001/components/password_man...
components/password_manager/core/browser/password_store.cc:387: for
(PasswordForm* form : more_results.get())
On 2015/10/14 10:58:26, engedy wrote:
> nit: Do we really need get() here?

Done.

[engedy, 2015-10-20 11:58:00]

Vadym, just to clarify, I have no more comments, feel free to land this CL.

[dvadym, 2015-10-20 14:24:20]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-10-20 14:24:20]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/1401723002/#ps80001
(title: "Test fix")

[commit-bot: I haz the power, 2015-10-20 14:24:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1401723002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1401723002/80001

[commit-bot: I haz the power, 2015-10-20 14:41:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-20 14:42:34]

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