[Payments] Dedupe subsets in contact detail suggestions.

[Payments] Dedupe subsets in contact detail suggestions.

Previously suggestions that are a subset of another suggestion are not
deduped.

With this patch, only the most complete version will be suggested to
the user.

A side-effect of this change is that ContactDetails are now sorted by
relevance in regards or the information required from the merchant
instead of completeness.

For example, considering a profile A with only email and a profile B
with email and phone, and a merchant requires email, phone and name.
Previously the order between A and B would have been decided by
frecency since both were incomplete. With this patch, B would be
suggested first since it has more information.

BUG=674647
Review-Url: https://codereview.chromium.org/2583593002
Cr-Commit-Position: refs/heads/master@{#441764}
Committed: https://chromium.googlesource.com/chromium/src/+/90bf8242b2d3f9ffb2bef194f4e065fb81a13ce9

[sebsg, 2016-12-15 19:11:50]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 19:12:07]

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-12-15 19:50:09]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-12-15 19:50:10]

Dry run: This issue passed the CQ dry run.

[sebsg, 2016-12-16 00:27:44]

Description was changed from

==========
[Payments] Dedupe subsets in contact detail suggestions.

BUG=674647
==========

to

==========
[Payments] Dedupe subsets in contact detail suggestions.

Previously suggestions that are a subset of another suggestion are not deduped.

With this patch, only the most complete version will be suggested to the user.

A side-effect of this change is that ContactDetails are now sorted by relevance
in regards or the information required from the merchant instead of
completeness.

For example, considering a profile A with only email and a profile B with email
and phone, and a merchant requires email, phone and name. Previously the order
between A and B would have been decided by frecency since both were incomplete.
With this patch, B would be suggested first since it has more information.

BUG=674647
==========

[sebsg, 2016-12-16 00:27:53]

Patchset #2 (id:20001) has been deleted

[sebsg, 2016-12-16 00:28:09]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-12-16 00:32:47]

The CQ bit was checked by [email protected] to run a CQ dry run

[sebsg, 2016-12-16 00:33:00]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-12-16 00:34:28]

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-12-16 02:47:48]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-12-16 02:47:48]

Dry run: This issue passed the CQ dry run.

[sebsg, 2016-12-16 16:27:20]

[email protected] changed reviewers:
+ [email protected]

[sebsg, 2016-12-16 16:27:21]

Hi Rouslan, PTAL :)

[please use gerrit instead, 2016-12-16 19:57:35]

Description was changed from

==========
[Payments] Dedupe subsets in contact detail suggestions.

Previously suggestions that are a subset of another suggestion are not deduped.

With this patch, only the most complete version will be suggested to the user.

A side-effect of this change is that ContactDetails are now sorted by relevance
in regards or the information required from the merchant instead of
completeness.

For example, considering a profile A with only email and a profile B with email
and phone, and a merchant requires email, phone and name. Previously the order
between A and B would have been decided by frecency since both were incomplete.
With this patch, B would be suggested first since it has more information.

BUG=674647
==========

to

==========
[Payments] Dedupe subsets in contact detail suggestions.

Previously suggestions that are a subset of another suggestion are not
deduped.

With this patch, only the most complete version will be suggested to
the user.

A side-effect of this change is that ContactDetails are now sorted by
relevance in regards or the information required from the merchant
instead of completeness.

For example, considering a profile A with only email and a profile B
with email and phone, and a merchant requires email, phone and name.
Previously the order between A and B would have been decided by
frecency since both were incomplete. With this patch, B would be
suggested first since it has more information.

BUG=674647
==========

[please use gerrit instead, 2016-12-16 20:34:51]

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:360:
createShippingSection(profiles);
Collections.unmodifiableList(profiles)

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:364:
createContactSection(profiles, requestPayerName, requestPayerPhone,
requestPayerEmail);
Collections.unmodifiableList(profiles)

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:444:
Collections.sort(profiles, new Comparator<AutofillProfile>() {
Don't sort your input. The caller of createContactSection() still holds a
reference to these profiles, so their list will get sorted too. That's not
something that you intended.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:462:

Do you need to add frecency comparison here to break the tie if scoreB==scoreA?

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:463:
return (scoreB - scoreA);
nit: no need for ( and ).

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:482:
if (!name.equals("") || !phone.equals("") || !email.equals("")) {
.equals("") --> .isEmpty()

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:490:
&& uniqueContactInfos.get(j).contains(email)) {
Using string concantenation is longer good enough. Suppose I have one profile
that has only my email and I use it a lot:

[email protected]

Then I also have a profile that has only my name and I don't use it very often:

Jon Doe

Your algorithm will think that the second profile is a subset of the first, but
they are distinct. Use a struct for uniqueContactInfos instead:

private static class Contact {
 private final String mName;
 private final String mPhone;
 private final String mEmail;
 boolean contains(String name, String phone, String email) {
   return mName.contains(name) && mPhone.contains(phone) &&
mEmail.contains(email);
 }
}

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:494:
}
Add a comment that this algorithm is quadratic in the number of profiles, but
that's typically small, i.e., fewer than 10, so a faster more complicated
algorithm is an overkill.

(Unless, of course, you can come up with a faster algorithm that's still
simple.)

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java
(right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:50:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "",
"[email protected]",
"" /* phoneNumber */

Otherwise reader needs to think hard to figure out how you accomplished "without
phone number" property.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:55:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "555-555-5555", "",
"" /* emailAddress */

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:59:
helper.setProfile(new AutofillProfile("", "https://example.com", true, "",
"Google",
"" /* name */

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java
(right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:49:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "",
"[email protected]",
Ditto

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:54:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "555-555-5555", "",
Ditto

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:58:
helper.setProfile(new AutofillProfile("", "https://example.com", true, "",
"Google",
Ditto

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:133:
assertEquals((i == (PaymentRequestMetrics.REQUESTED_INFORMATION_EMAIL
Nit: Could not hurt to break this 6 line statement with bitmasks into several
statements for clarity.

https://codereview.chromium.org/2583593002/diff/60001/chrome/test/data/androi...
File chrome/test/data/android/payments/email_and_phone.js (right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/test/data/androi...
chrome/test/data/android/payments/email_and_phone.js:22: print(
By the way, you can do JSON.stringify(resp, undefined, 2) now instead of
manually reading things out of resp one by one. Pretty nifty!

[sebsg, 2016-12-19 18:19:21]

Patchset #2 (id:80001) has been deleted

[sebsg, 2016-12-19 20:56:31]

Thanks, another look?

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:360:
createShippingSection(profiles);
On 2016/12/16 20:34:51, rouslan wrote:
> Collections.unmodifiableList(profiles)

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:364:
createContactSection(profiles, requestPayerName, requestPayerPhone,
requestPayerEmail);
On 2016/12/16 20:34:51, rouslan wrote:
> Collections.unmodifiableList(profiles)

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:444:
Collections.sort(profiles, new Comparator<AutofillProfile>() {
On 2016/12/16 20:34:51, rouslan wrote:
> Don't sort your input. The caller of createContactSection() still holds a
> reference to these profiles, so their list will get sorted too. That's not
> something that you intended.

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:462:

On 2016/12/16 20:34:51, rouslan wrote:
> Do you need to add frecency comparison here to break the tie if
scoreB==scoreA?

Collections.sort is stable so the frecency ordering will be preserved in case of
equality.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:463:
return (scoreB - scoreA);
On 2016/12/16 20:34:50, rouslan wrote:
> nit: no need for ( and ).

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:482:
if (!name.equals("") || !phone.equals("") || !email.equals("")) {
On 2016/12/16 20:34:51, rouslan wrote:
> .equals("") --> .isEmpty()

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:490:
&& uniqueContactInfos.get(j).contains(email)) {
On 2016/12/16 20:34:50, rouslan wrote:
> Using string concantenation is longer good enough. Suppose I have one profile
> that has only my email and I use it a lot:
> 
> mailto:[email protected]
> 
> Then I also have a profile that has only my name and I don't use it very
often:
> 
> Jon Doe
> 
> Your algorithm will think that the second profile is a subset of the first,
but
> they are distinct. Use a struct for uniqueContactInfos instead:
> 
> private static class Contact {
>  private final String mName;
>  private final String mPhone;
>  private final String mEmail;
>  boolean contains(String name, String phone, String email) {
>    return mName.contains(name) && mPhone.contains(phone) &&
> mEmail.contains(email);
>  }
> }

You are right. I changed the comparison to be that the individual fields have to
be equal, or the second one (senond contact) can be empty

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:494:
}
On 2016/12/16 20:34:51, rouslan wrote:
> Add a comment that this algorithm is quadratic in the number of profiles, but
> that's typically small, i.e., fewer than 10, so a faster more complicated
> algorithm is an overkill.
> 
> (Unless, of course, you can come up with a faster algorithm that's still
> simple.)

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java
(right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:50:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "",
"[email protected]",
On 2016/12/16 20:34:51, rouslan wrote:
> "" /* phoneNumber */
> 
> Otherwise reader needs to think hard to figure out how you accomplished
"without
> phone number" property.

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:55:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "555-555-5555", "",
On 2016/12/16 20:34:51, rouslan wrote:
> "" /* emailAddress */

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:59:
helper.setProfile(new AutofillProfile("", "https://example.com", true, "",
"Google",
On 2016/12/16 20:34:51, rouslan wrote:
> "" /* name */

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java
(right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:49:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "",
"[email protected]",
On 2016/12/16 20:34:51, rouslan wrote:
> Ditto

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:54:
"340 Main St", "CA", "Los Angeles", "", "90291", "", "US", "555-555-5555", "",
On 2016/12/16 20:34:51, rouslan wrote:
> Ditto

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:58:
helper.setProfile(new AutofillProfile("", "https://example.com", true, "",
"Google",
On 2016/12/16 20:34:51, rouslan wrote:
> Ditto

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:133:
assertEquals((i == (PaymentRequestMetrics.REQUESTED_INFORMATION_EMAIL
On 2016/12/16 20:34:51, rouslan wrote:
> Nit: Could not hurt to break this 6 line statement with bitmasks into several
> statements for clarity.

Done.

https://codereview.chromium.org/2583593002/diff/60001/chrome/test/data/androi...
File chrome/test/data/android/payments/email_and_phone.js (right):

https://codereview.chromium.org/2583593002/diff/60001/chrome/test/data/androi...
chrome/test/data/android/payments/email_and_phone.js:22: print(
On 2016/12/16 20:34:51, rouslan wrote:
> By the way, you can do JSON.stringify(resp, undefined, 2) now instead of
> manually reading things out of resp one by one. Pretty nifty!

Done.

[please use gerrit instead, 2016-12-20 19:14:56]

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:175:
boolean isSubsetOf(Contact contact) {
This method is kind of tricky, can you add Javadoc with an example of when this
method would return true or false? Thank you!

Better yet, put isSubsetOf() on AutofillContact class and add more test cases to
https://cs.chromium.org/chromium/src/chrome/android/junit/src/org/chromium/ch...

Also, do you think the same subset issues should be solved with credit cards and
shipping addresses?

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:464:
private void createContactSection(List<AutofillProfile> unmodifiableProfiles,
FYI, Mathieu is moving this function to ContactDetailsSection.java.

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:483:
if (!TextUtils.isEmpty(a.getPhoneNumber())) ++scoreA;
Can we increment the score only for valid phone numbers?

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:487:
if (!TextUtils.isEmpty(a.getEmailAddress())) ++scoreA;
Can we increment the score only for valid emails?

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:519:
if (uniqueContacts.get(j).isSubsetOf(contact)) {
This logic is reversed. If the contact in question is a subset of one of the
unique contacts that were already observed, then don't add it.

if (contact.isSubsetOf(uniqueContaincts.get(j)) {
    isNewSuggestion = false;
    break;
}

[sebsg, 2016-12-22 18:42:13]

Patchset #4 (id:140001) has been deleted

[sebsg, 2016-12-22 18:46:05]

Hi Rouslan PTAL :)

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:175:
boolean isSubsetOf(Contact contact) {
On 2016/12/20 19:14:56, rouslan-intermittent-holidays wrote:
> This method is kind of tricky, can you add Javadoc with an example of when
this
> method would return true or false? Thank you!
> 
> Better yet, put isSubsetOf() on AutofillContact class and add more test cases
to
>
https://cs.chromium.org/chromium/src/chrome/android/junit/src/org/chromium/ch...
> 
> Also, do you think the same subset issues should be solved with credit cards
and
> shipping addresses?

Brilliant! And yeah let me think about the other ones.

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:464:
private void createContactSection(List<AutofillProfile> unmodifiableProfiles,
On 2016/12/20 19:14:56, rouslan-intermittent-holidays wrote:
> FYI, Mathieu is moving this function to ContactDetailsSection.java.

Thanks! I'll try to beat him to the finish line!

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:483:
if (!TextUtils.isEmpty(a.getPhoneNumber())) ++scoreA;
On 2016/12/20 19:14:56, rouslan-intermittent-holidays wrote:
> Can we increment the score only for valid phone numbers?

Done.

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:487:
if (!TextUtils.isEmpty(a.getEmailAddress())) ++scoreA;
On 2016/12/20 19:14:56, rouslan-intermittent-holidays wrote:
> Can we increment the score only for valid emails?

Done.

https://codereview.chromium.org/2583593002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:519:
if (uniqueContacts.get(j).isSubsetOf(contact)) {
On 2016/12/20 19:14:56, rouslan-intermittent-holidays wrote:
> This logic is reversed. If the contact in question is a subset of one of the
> unique contacts that were already observed, then don't add it.
> 
> if (contact.isSubsetOf(uniqueContaincts.get(j)) {
>     isNewSuggestion = false;
>     break;
> }

Right, I actually made IsEqualorSupersetOf. Thanks

[please use gerrit instead, 2016-12-27 14:33:58]

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java
(left):

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:32:
@Retention(RetentionPolicy.SOURCE)
I think you can use @IntDef and @Retension even for a bitmask.

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:445:
final boolean requestName, final boolean requestPhone, final boolean
requestEmail) {
There seems to be no need to mark the booleans as finals.

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:475:
mContactEditor.addEmailAddressIfValid(contact.getPayerEmail());
You should be adding all names, phones, and emails to the editor autocomplete
list, not just the first SUGGESTIONS_LIMIT of them. Therefore, lines 473-475
should be moved to line 457.

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:479:
|| !TextUtils.isEmpty(contact.getPayerEmail())) {
This if statement should be filtering out irrelevant contacts, i.e., those that
don't have any of the requested fields. Therefore, you should be checking
requestName, requestPhone, and requestEmail booleans in this if statement as
well.

Also, it would be best to perform this check when creating AutofillContact
objects on line 452 instead of down here.

[sebsg, 2017-01-05 18:35:48]

Hi Rouslan, another look?

Thanks!

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java
(left):

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:32:
@Retention(RetentionPolicy.SOURCE)
On 2016/12/27 14:33:58, rouslan wrote:
> I think you can use @IntDef and @Retension even for a bitmask.

IntDef gets confused with the boolean OR operator ex: INVALID_NAME |
INVALID_EMAIL.

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:445:
final boolean requestName, final boolean requestPhone, final boolean
requestEmail) {
On 2016/12/27 14:33:58, rouslan wrote:
> There seems to be no need to mark the booleans as finals.

Done.

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:475:
mContactEditor.addEmailAddressIfValid(contact.getPayerEmail());
On 2016/12/27 14:33:58, rouslan wrote:
> You should be adding all names, phones, and emails to the editor autocomplete
> list, not just the first SUGGESTIONS_LIMIT of them. Therefore, lines 473-475
> should be moved to line 457.

Done.

https://codereview.chromium.org/2583593002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:479:
|| !TextUtils.isEmpty(contact.getPayerEmail())) {
On 2016/12/27 14:33:58, rouslan wrote:
> This if statement should be filtering out irrelevant contacts, i.e., those
that
> don't have any of the requested fields. Therefore, you should be checking
> requestName, requestPhone, and requestEmail booleans in this if statement as
> well.
> 
> Also, it would be best to perform this check when creating AutofillContact
> objects on line 452 instead of down here.

Done.

[please use gerrit instead, 2017-01-05 18:59:23]

LGTM

Some of the best code I've seen this year!

https://codereview.chromium.org/2583593002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java
(right):

https://codereview.chromium.org/2583593002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:109:
public boolean isEqualOrSupersetOf(AutofillContact contact) {
nit: assert contact != null;

[sebsg, 2017-01-05 19:29:12]

Thanks! sending to CQ

https://codereview.chromium.org/2583593002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java
(right):

https://codereview.chromium.org/2583593002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:109:
public boolean isEqualOrSupersetOf(AutofillContact contact) {
On 2017/01/05 18:59:23, rouslan wrote:
> nit: assert contact != null;

Done.

[sebsg, 2017-01-05 19:29:22]

The CQ bit was checked by [email protected]

[sebsg, 2017-01-05 19:29:22]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/2583593002/#ps220001
(title: "Nit")

[commit-bot: I haz the power, 2017-01-05 19:29:42]

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-01-05 21:43:02]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1483644562247810,
"parent_rev": "e4fe80178e0f20f1f70be8190f11b97133dfb09b", "commit_rev":
"90bf8242b2d3f9ffb2bef194f4e065fb81a13ce9"}

[commit-bot: I haz the power, 2017-01-05 21:43:33]

Description was changed from

==========
[Payments] Dedupe subsets in contact detail suggestions.

Previously suggestions that are a subset of another suggestion are not
deduped.

With this patch, only the most complete version will be suggested to
the user.

A side-effect of this change is that ContactDetails are now sorted by
relevance in regards or the information required from the merchant
instead of completeness.

For example, considering a profile A with only email and a profile B
with email and phone, and a merchant requires email, phone and name.
Previously the order between A and B would have been decided by
frecency since both were incomplete. With this patch, B would be
suggested first since it has more information.

BUG=674647
==========

to

==========
[Payments] Dedupe subsets in contact detail suggestions.

Previously suggestions that are a subset of another suggestion are not
deduped.

With this patch, only the most complete version will be suggested to
the user.

A side-effect of this change is that ContactDetails are now sorted by
relevance in regards or the information required from the merchant
instead of completeness.

For example, considering a profile A with only email and a profile B
with email and phone, and a merchant requires email, phone and name.
Previously the order between A and B would have been decided by
frecency since both were incomplete. With this patch, B would be
suggested first since it has more information.

BUG=674647

Review-Url: https://codereview.chromium.org/2583593002
Cr-Commit-Position: refs/heads/master@{#441764}
Committed:
https://chromium.googlesource.com/chromium/src/+/90bf8242b2d3f9ffb2bef194f4e0...
==========

[commit-bot: I haz the power, 2017-01-05 21:43:35]

Committed patchset #7 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/90bf8242b2d3f9ffb2bef194f4e0...