[Payments] Dedupe subsets in contact detail suggestions.

chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java

Index: chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
index f1b2243623f7849350ffa9cb086c8f3b0dfa9d06..bfbc25ade1875e6ecf642ef27bedc8e3036f48bd 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
@@ -434,34 +434,67 @@ public class PaymentRequestImpl
                 PaymentRequestUI.TYPE_SHIPPING_ADDRESSES, firstCompleteAddressIndex, addresses);
     }
 
-    private void createContactSection(List<AutofillProfile> profiles, boolean requestName,
-            boolean requestPhone, boolean requestEmail) {
-        Set<String> uniqueContactInfos = new HashSet<>();
+    private void createContactSection(List<AutofillProfile> profiles, final boolean requestName,
+            final boolean requestPhone, final boolean requestEmail) {
+        List<String> uniqueContactInfos = new ArrayList<>();
         mContactEditor = new ContactEditor(requestName, requestPhone, requestEmail);
         List<AutofillContact> contacts = new ArrayList<>();
 
+        // Order the profiles so the ones that have most of the required information are put first.
+        Collections.sort(profiles, new Comparator<AutofillProfile>() {

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

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.

[sebsg, 2016/12/19 20:56:30]

Done.

+            @Override
+            public int compare(AutofillProfile a, AutofillProfile b) {
+                int scoreA = 0;
+                int scoreB = 0;
+
+                if (requestName) {
+                    if (!TextUtils.isEmpty(a.getFullName())) ++scoreA;
+                    if (!TextUtils.isEmpty(b.getFullName())) ++scoreB;
+                }
+                if (requestPhone) {
+                    if (!TextUtils.isEmpty(a.getPhoneNumber())) ++scoreA;
+                    if (!TextUtils.isEmpty(b.getPhoneNumber())) ++scoreB;
+                }
+                if (requestEmail) {
+                    if (!TextUtils.isEmpty(a.getEmailAddress())) ++scoreA;
+                    if (!TextUtils.isEmpty(b.getEmailAddress())) ++scoreB;
+                }
+

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

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

[sebsg, 2016/12/19 20:56:30]

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

+                return (scoreB - scoreA);

[please use gerrit instead, 2016/12/16 20:34:50]

nit: no need for ( and ).

[sebsg, 2016/12/19 20:56:30]

Done.

+            }
+        });
+
         for (int i = 0; i < profiles.size(); i++) {
             AutofillProfile profile = profiles.get(i);
             String name = requestName && !TextUtils.isEmpty(profile.getFullName())
                     ? profile.getFullName()
-                    : null;
+                    : "";
             String phone = requestPhone && !TextUtils.isEmpty(profile.getPhoneNumber())
                     ? profile.getPhoneNumber()
-                    : null;
+                    : "";
             String email = requestEmail && !TextUtils.isEmpty(profile.getEmailAddress())
                     ? profile.getEmailAddress()
-                    : null;
+                    : "";
             mContactEditor.addPayerNameIfValid(name);
             mContactEditor.addPhoneNumberIfValid(phone);
             mContactEditor.addEmailAddressIfValid(email);
 
-            if (name != null || phone != null || email != null) {
-                // Different profiles can have identical contact info. Do not add the same
-                // contact info to the list twice.
-                String uniqueContactInfo = name + phone + email;
-                if (!uniqueContactInfos.contains(uniqueContactInfo)) {
-                    uniqueContactInfos.add(uniqueContactInfo);
+            if (!name.equals("") || !phone.equals("") || !email.equals("")) {

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

.equals("") --> .isEmpty()

[sebsg, 2016/12/19 20:56:30]

Done.

+                // Different profiles can have identical contact info. Do not add the same contact
+                // info or a subset of it twice. It's important that the profiles be sorted by
+                // the quantity of required info they have.
+                boolean isNewSuggestion = true;
+                for (int j = 0; j < uniqueContactInfos.size(); ++j) {
+                    if (uniqueContactInfos.get(j).contains(name)
+                            && uniqueContactInfos.get(j).contains(phone)
+                            && uniqueContactInfos.get(j).contains(email)) {

[please use gerrit instead, 2016/12/16 20:34:50]

Using string concantenation is longer good enough. Suppose I have one profile
that has only my email and I use it a lot:

Jon.Doe@company.com

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);
 }
}

[sebsg, 2016/12/19 20:56:30]

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

+                        isNewSuggestion = false;
+                        break;
+                    }
+                }

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

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

[sebsg, 2016/12/19 20:56:30]

Done.

 
+                if (isNewSuggestion) {
+                    uniqueContactInfos.add(name + phone + email);
                     @ContactEditor.CompletionStatus
                     int completionStatus =
                             mContactEditor.checkContactCompletionStatus(name, phone, email);
@@ -469,13 +502,10 @@ public class PaymentRequestImpl
                             mContext, profile, name, phone, email, completionStatus));
                 }
             }
-        }
 
-        // Suggest complete contact infos first.
-        Collections.sort(contacts, COMPLETENESS_COMPARATOR);
-
-        // Limit the number of suggestions.
-        contacts = contacts.subList(0, Math.min(contacts.size(), SUGGESTIONS_LIMIT));
+            // Limit the number of suggestions.
+            if (contacts.size() == SUGGESTIONS_LIMIT) break;
+        }
 
         // Log the number of suggested contact infos.
         mJourneyLogger.setNumberOfSuggestionsShown(