[WebPayments] Implementing Profile filter and dedupe

components/payments/content/profile_util.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/payments/content/profile_util.h"
+
+#include "components/autofill/core/browser/autofill_profile.h"
+#include "components/autofill/core/browser/field_types.h"
+
+namespace payments {
+namespace profile_util {
+
+std::vector<autofill::AutofillProfile*> FilterProfilesForContact(
+    const std::vector<autofill::AutofillProfile*>& profiles,
+    const std::string& app_locale,
+    PaymentRequestSpec* spec) {
+  // We will be removing profiles, so we operate on a copy.
+  std::vector<autofill::AutofillProfile*> processed = profiles;

[Roger McFarlane (Chromium), 2017/03/24 18:23:16]

Note, but don't actually apply, that this could have been accomplished by taking
profiles by value... which would of course confuse the next person who looked at
the code.

:)

+
+  PaymentsProfileComparator comparator(app_locale, spec);
+
+  // Stable sort, since profiles are expected to be passed in frecency order.
+  std::stable_sort(
+      processed.begin(), processed.end(),
+      std::bind(&PaymentsProfileComparator::CompareContactCompleteness,
+                &comparator, std::placeholders::_1, std::placeholders::_2));
+
+  auto it = processed.begin();
+  while (it != processed.end()) {
+    if (comparator.GetContactCompletenessScore(*it) == 0) {
+      // Since profiles are sorted by completeness, this and any further
+      // profiles can be discarded.
+      processed.erase(it, processed.end());
+      break;
+    }
+
+    // Attempt to find a matching element in the vector before the current.
+    // This is quadratic, but the number of elements is generally small
+    // (< 10), so a more complicated algorithm would be overkill.
+    if (std::find_if(processed.begin(), it,
+                     [&](autofill::AutofillProfile* prior) {
+                       return comparator.IsContactEqualOrSuperset(prior, *it);
+                     }) != it) {
+      // Remove the subset profile. Do not update |it|, as it will point to
+      // the next element after erasure.
+      processed.erase(it);

[Roger McFarlane (Chromium), 2017/03/24 18:23:16]

this isn't safe (i.e., you're venturing off into undefined behavior).

Maybe structure this as a std::partition then erase the tail?

Alternatively, use integer indices not iterators.

[sebsg, 2017/03/27 17:13:28]

+1. What I did in Java is have add them to a new vector... again... Not sure
efficient but the dataset is very small.

You could also keep the index of the ones you want to remove, and remove
starting from the back to reduce the number of profiles that will be moved. But
this is not very efficient either :P

[tmartino, 2017/04/03 16:42:54]

As discussed offline, erase returns an iterator to the next element. Therefore
this is safe if we add an assignment:

it = processed.erase(it);

We also discussed optimizations which would reduce the number of things being
shifted around during these removes, but IMO the extra complexity (either an
alg. which involves shifting these around in-place, or making additional vectors
in-memory) is not justified w/r/t the expected small number of elements.

+    } else {
+      it++;
+    }
+  }
+
+  return processed;
+}
+
+PaymentsProfileComparator::PaymentsProfileComparator(
+    const std::string& app_locale,
+    PaymentRequestSpec* spec)
+    : autofill::AutofillProfileComparator(app_locale), spec_(spec) {}
+
+PaymentsProfileComparator::~PaymentsProfileComparator() {}
+
+bool PaymentsProfileComparator::IsContactEqualOrSuperset(

[Roger McFarlane (Chromium), 2017/03/24 18:23:16]

Alas, the merge-ability tests don't really yield an ordering. They're not giving
you the superset condition.

For example: John Public & J Q Public are mergeable but not orderable.

This is also true for phone numbers.

Email addresses are kind of a degenerate case as it's just a case-insensitive
comparison.

[sebsg, 2017/03/27 17:13:28]

Yeah that's a good point Roger. In the Java implementation I just check if they
are the same (minus case, maybe we could add punctuation too).

It's not perfect, but I think it's fine to show both profiles in Roger's
example. What we really want to avoid is showing all for cases like

1- Jon Snow, jon@snow.com, 123-123-1234
2- Jon Snow, jon@snow.com
3- Jon Snow, 123-123-1234
4- Jon Snow

etc.

[tmartino, 2017/04/03 16:42:54]

As resolved offline we've decided to accept this.

+    autofill::AutofillProfile* super,
+    autofill::AutofillProfile* sub) {
+  if (spec_->request_payer_name()) {
+    if (sub->HasInfo(autofill::NAME_FULL) &&
+        !super->HasInfo(autofill::NAME_FULL))

[Roger McFarlane (Chromium), 2017/03/24 18:23:16]

nit: multi-line if requires curly braces

[tmartino, 2017/04/03 16:42:54]

Believe it or not, I've been told elsewhere that this is not required! (And it's
clearly not enforced by git cl format, which I ran here.)

However, I don't understand Chrome's bizarre war on "unnecessary brackets" so I
will happily take your comment as an indication that the code is difficult for a
reviewer to read, and therefore as permission to put them in.

+      return false;
+    if (!HaveMergeableNames(*super, *sub))
+      return false;
+  }
+  if (spec_->request_payer_phone()) {
+    if (sub->HasInfo(autofill::PHONE_HOME_WHOLE_NUMBER) &&
+        !super->HasInfo(autofill::PHONE_HOME_WHOLE_NUMBER))
+      return false;
+    if (!HaveMergeablePhoneNumbers(*super, *sub))
+      return false;
+  }
+  if (spec_->request_payer_email()) {
+    if (sub->HasInfo(autofill::EMAIL_ADDRESS) &&
+        !super->HasInfo(autofill::EMAIL_ADDRESS))
+      return false;
+    if (!HaveMergeableEmailAddresses(*super, *sub))
+      return false;
+  }
+  return true;
+}
+
+int PaymentsProfileComparator::GetContactCompletenessScore(
+    autofill::AutofillProfile* profile) {
+  int score = 0;
+  if (spec_->request_payer_name() && profile->HasInfo(autofill::NAME_FULL))
+    score++;
+  if (spec_->request_payer_phone() &&
+      profile->HasInfo(autofill::PHONE_HOME_WHOLE_NUMBER))
+    score++;
+  if (spec_->request_payer_email() && profile->HasInfo(autofill::EMAIL_ADDRESS))
+    score++;
+  return score;

[Roger McFarlane (Chromium), 2017/03/24 18:23:16]

could be made a bit more succinct?

return
    (spec_->request_foo() && profile->HasInfo(foo)) +
    (spec_->request_bar() && profile->HasInfo(bar)) +
    (spec_->request_baz() && profile->HasInfo(baz));

Design/Spec question:

  Should there be any difference in weight between the items.

[sebsg, 2017/03/27 17:13:28]

For the spec question: Not really, they all need to be there. before the user
can do the transaction.

[tmartino, 2017/04/03 16:42:54]

Done re: succinctness.

+}
+
+bool PaymentsProfileComparator::IsContactInfoComplete(
+    autofill::AutofillProfile* profile) {
+  int score = spec_->request_payer_name() + spec_->request_payer_phone() +

[Roger McFarlane (Chromium), 2017/03/24 18:23:16]

s/score/desired_score|full_score/

?

[tmartino, 2017/04/03 16:42:54]

Done

+              spec_->request_payer_email();
+  return GetContactCompletenessScore(profile) == score;
+}
+
+bool PaymentsProfileComparator::CompareContactCompleteness(
+    autofill::AutofillProfile* p1,
+    autofill::AutofillProfile* p2) {
+  return GetContactCompletenessScore(p1) > GetContactCompletenessScore(p2);
+}
+
+}  // namespace profile_util
+}  // namespace payments