[WebPayments] Adding FilterProfilesForShipping to profile comparator

BUG=722949

Review-Url: https://codereview.chromium.org/2884393002
Cr-Commit-Position: refs/heads/master@{#473929}

chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc

@@ -237,8 +237,9 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestContactInfoEditorTest,
   OpenContactInfoScreen();
 
   PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
-  EXPECT_EQ(request->state()->contact_profiles().front(),
-            request->state()->selected_contact_profile());
+
+  // No contact profiles are selected because both are incomplete.
+  EXPECT_EQ(nullptr, request->state()->selected_contact_profile());
 
   views::View* list_view = dialog_view()->GetViewByID(
       static_cast<int>(DialogViewID::CONTACT_INFO_SHEET_LIST_VIEW));
@@ -269,6 +270,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestContactInfoEditorTest,
             profile->GetInfo(autofill::AutofillType(autofill::EMAIL_ADDRESS),
                              GetLocale()));
 
+  // Expect the newly-completed profile to be selected.
   EXPECT_EQ(2U, request->state()->contact_profiles().size());
   EXPECT_EQ(request->state()->contact_profiles().back(), profile);
 }

chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.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 "base/guid.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/ui/views/payments/payment_request_browsertest_base.h"
+#include "chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h"
+#include "components/autofill/core/browser/autofill_profile.h"
+#include "components/autofill/core/browser/autofill_test_utils.h"
+#include "components/autofill/core/browser/field_types.h"
+#include "components/autofill/core/browser/personal_data_manager.h"
+#include "ui/views/controls/label.h"
+
+namespace payments {
+
+autofill::AutofillProfile CreateProfileWithPartialAddress() {
+  autofill::AutofillProfile profile = autofill::test::GetFullProfile2();
+  profile.SetRawInfo(autofill::ADDRESS_HOME_LINE1, base::ASCIIToUTF16(""));
+  profile.SetRawInfo(autofill::ADDRESS_HOME_LINE2, base::ASCIIToUTF16(""));
+  profile.SetRawInfo(autofill::ADDRESS_HOME_CITY, base::ASCIIToUTF16(""));
+  profile.SetRawInfo(autofill::ADDRESS_HOME_STATE, base::ASCIIToUTF16(""));
+  return profile;
+}
+
+class PaymentRequestProfileListTest : public PaymentRequestBrowserTestBase {
+ protected:
+  PaymentRequestProfileListTest()
+      : PaymentRequestBrowserTestBase(
+            "/payment_request_free_shipping_test.html") {}
+};
+
+IN_PROC_BROWSER_TEST_F(PaymentRequestProfileListTest, PrioritizeCompleteness) {
+  autofill::AutofillProfile complete = autofill::test::GetFullProfile();
+  autofill::AutofillProfile partial = CreateProfileWithPartialAddress();
+  partial.set_use_count(1000);
+
+  AddAutofillProfile(complete);
+  AddAutofillProfile(partial);
+
+  // In the Personal Data Manager, the partial address is more frecent.
+  autofill::PersonalDataManager* personal_data_manager = GetDataManager();
+  std::vector<autofill::AutofillProfile*> profiles =
+      personal_data_manager->GetProfiles();
+  ASSERT_EQ(2UL, profiles.size());
+  EXPECT_EQ(partial, *profiles[0]);
+  EXPECT_EQ(complete, *profiles[1]);
+
+  InvokePaymentRequestUI();
+
+  PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
+
+  // The complete profile should be selected.
+  ASSERT_TRUE(request->state()->selected_shipping_profile());
+  EXPECT_EQ(complete, *request->state()->selected_shipping_profile());
+
+  // It should appear first in the shipping profiles.
+  ASSERT_EQ(2UL, request->state()->shipping_profiles().size());
+  EXPECT_EQ(complete, *request->state()->shipping_profiles()[0]);
+  EXPECT_EQ(partial, *request->state()->shipping_profiles()[1]);
+
+  // And both should appear in the UI.
+  OpenShippingAddressSectionScreen();
+  views::View* sheet = dialog_view()->GetViewByID(
+      static_cast<int>(DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW));
+  ASSERT_EQ(2, sheet->child_count());
+  views::View* first_label = sheet->child_at(0)->GetViewByID(
+      static_cast<int>(DialogViewID::PROFILE_LABEL_LINE_1));
+  views::View* second_label = sheet->child_at(1)->GetViewByID(
+      static_cast<int>(DialogViewID::PROFILE_LABEL_LINE_1));
+
+  EXPECT_EQ(base::ASCIIToUTF16("John H. Doe"),
+            static_cast<views::Label*>(first_label)->text());
+  EXPECT_EQ(base::ASCIIToUTF16("Jane A. Smith"),
+            static_cast<views::Label*>(second_label)->text());
+}
+
+}  // namespace payments

chrome/test/BUILD.gn

@@ -2058,6 +2058,7 @@ test("browser_tests") {
         "../browser/ui/views/payments/payment_request_payment_response_browsertest.cc",
         "../browser/ui/views/payments/payment_request_use_stats_browsertest.cc",
         "../browser/ui/views/payments/payment_sheet_view_controller_browsertest.cc",
+        "../browser/ui/views/payments/profile_list_view_controller_browsertest.cc",
         "../browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc",
         "../browser/ui/views/payments/shipping_option_view_controller_browsertest.cc",
         "../browser/ui/views/select_file_dialog_extension_browsertest.cc",

components/payments/content/payment_request_state.cc

@@ -238,26 +238,22 @@ void PaymentRequestState::PopulateProfileCache() {
   std::vector<autofill::AutofillProfile*> profiles =
       personal_data_manager_->GetProfilesToSuggest();
 
+  std::vector<autofill::AutofillProfile*> raw_profiles_for_filtering;
+  raw_profiles_for_filtering.reserve(profiles.size());
+
   // PaymentRequest may outlive the Profiles returned by the Data Manager.
   // Thus, we store copies, and return a vector of pointers to these copies
   // whenever Profiles are requested.
   for (size_t i = 0; i < profiles.size(); i++) {
     profile_cache_.push_back(
         base::MakeUnique<autofill::AutofillProfile>(*profiles[i]));
-
-    shipping_profiles_.push_back(profile_cache_[i].get());
+    raw_profiles_for_filtering.push_back(profile_cache_.back().get());
   }
 
-  std::vector<autofill::AutofillProfile*> raw_profiles_for_filtering(
-      profile_cache_.size());
-  std::transform(profile_cache_.begin(), profile_cache_.end(),
-                 raw_profiles_for_filtering.begin(),
-                 [](const std::unique_ptr<autofill::AutofillProfile>& p) {
-                   return p.get();
-                 });
-
   contact_profiles_ = profile_comparator()->FilterProfilesForContact(
       raw_profiles_for_filtering);
+  shipping_profiles_ = profile_comparator()->FilterProfilesForShipping(
+      raw_profiles_for_filtering);
 
   // Create the list of available instruments. A copy of each card will be made
   // by their respective AutofillPaymentInstrument.
@@ -269,22 +265,17 @@ void PaymentRequestState::PopulateProfileCache() {
 
 void PaymentRequestState::SetDefaultProfileSelections() {
   // Only pre-select an address if the merchant provided at least one selected
-  // shipping option.
-  if (!shipping_profiles().empty() && spec_->selected_shipping_option()) {
-    // Choose any complete shipping profile, or default to the most frecent
-    // address if no complete address could be found.
-    selected_shipping_profile_ = shipping_profiles_[0];
-    for (autofill::AutofillProfile* profile : shipping_profiles_) {
-      if (profile_comparator_.IsShippingComplete(profile)) {
-        selected_shipping_profile_ = profile;
-        break;
-      }
-    }
+  // shipping option, and the top profile is complete. Assumes that profiles
+  // have already been sorted for completeness and frecency.
+  if (!shipping_profiles().empty() && spec_->selected_shipping_option() &&
+      profile_comparator()->IsShippingComplete(shipping_profiles_[0])) {
+    selected_shipping_profile_ = shipping_profiles()[0];
   }
 
   // Contact profiles were ordered by completeness in addition to frecency;
   // the first one is the best default selection.
-  if (!contact_profiles().empty())
+  if (!contact_profiles().empty() &&
+      profile_comparator()->IsContactInfoComplete(contact_profiles_[0]))
     selected_contact_profile_ = contact_profiles()[0];
 
   // TODO(crbug.com/702063): Change this code to prioritize instruments by use

components/payments/core/payments_profile_comparator.cc

@@ -118,9 +118,6 @@ bool PaymentsProfileComparator::IsContactEqualOrSuperset(
 
 int PaymentsProfileComparator::GetContactCompletenessScore(
     const autofill::AutofillProfile* profile) const {
-  if (!profile)
-    return 0;
-
   // Create a bitmask of the fields that are both present and required.
   ProfileFields present =
       ~GetMissingProfileFields(profile) & GetRequiredProfileFieldsForContact();
@@ -137,18 +134,42 @@ bool PaymentsProfileComparator::IsContactInfoComplete(
            GetRequiredProfileFieldsForContact());
 }
 
-bool PaymentsProfileComparator::IsContactMoreComplete(
-    const autofill::AutofillProfile* p1,
-    const autofill::AutofillProfile* p2) const {
-  return GetContactCompletenessScore(p1) > GetContactCompletenessScore(p2);
-}
-
 base::string16 PaymentsProfileComparator::GetStringForMissingContactFields(
     const autofill::AutofillProfile& profile) const {
   return GetStringForMissingFields(GetMissingProfileFields(&profile) &
                                    GetRequiredProfileFieldsForContact());
 }
 
+std::vector<autofill::AutofillProfile*>
+PaymentsProfileComparator::FilterProfilesForShipping(
+    const std::vector<autofill::AutofillProfile*>& profiles) const {
+  // Since we'll be changing the order/contents of the const input vector,
+  // we make a copy.
+  std::vector<autofill::AutofillProfile*> processed = profiles;
+
+  std::stable_sort(
+      processed.begin(), processed.end(),
+      std::bind(&PaymentsProfileComparator::IsShippingMoreComplete, this,
+                std::placeholders::_1, std::placeholders::_2));
+
+  // TODO(crbug.com/722949): Remove profiles with no relevant information, or
+  // which are subsets of more-complete profiles.
+
+  return processed;
+}
+
+int PaymentsProfileComparator::GetShippingCompletenessScore(
+    const autofill::AutofillProfile* profile) const {
+  // Create a bitmask of the fields that are both present and required.
+  ProfileFields present =
+      ~GetMissingProfileFields(profile) & GetRequiredProfileFieldsForShipping();
+
+  // Count how many are set. The completeness of the address is weighted so as
+  // to dominate the other fields.
+  return !!(present & kName) + !!(present & kPhone) +
+         (10 * !!(present & kAddress));
+}
+
 bool PaymentsProfileComparator::IsShippingComplete(
     const autofill::AutofillProfile* profile) const {
   // Mask the fields that are missing with those that are requried. If any bits
@@ -248,4 +269,16 @@ bool PaymentsProfileComparator::AreRequiredAddressFieldsPresent(
   return autofill::addressinput::HasAllRequiredFields(*data);
 }
 
+bool PaymentsProfileComparator::IsContactMoreComplete(
+    const autofill::AutofillProfile* p1,
+    const autofill::AutofillProfile* p2) const {
+  return GetContactCompletenessScore(p1) > GetContactCompletenessScore(p2);
+}
+
+bool PaymentsProfileComparator::IsShippingMoreComplete(
+    const autofill::AutofillProfile* p1,
+    const autofill::AutofillProfile* p2) const {
+  return GetShippingCompletenessScore(p1) > GetShippingCompletenessScore(p2);
+}
+
 }  // namespace payments

components/payments/core/payments_profile_comparator.h

@@ -71,20 +71,24 @@ class PaymentsProfileComparator : public autofill::AutofillProfileComparator {
   // |profile|.
   bool IsContactInfoComplete(const autofill::AutofillProfile* profile) const;
 
-  // Comparison function suitable for sorting profiles by contact completeness
-  // score with std::sort.
-  bool IsContactMoreComplete(const autofill::AutofillProfile* p1,
-                             const autofill::AutofillProfile* p2) const;
+  // Returns profiles for shipping, ordered by completeness. |profiles| should
+  // be passed in order of frecency, and this order will be preserved among
+  // equally-complete profiles.
+  std::vector<autofill::AutofillProfile*> FilterProfilesForShipping(
+      const std::vector<autofill::AutofillProfile*>& profiles) const;
 
-  // Returns a localized string to be displayed in UI indicating what action,
-  // if any, must be taken for the given profile to be used as contact info.
-  base::string16 GetStringForMissingContactFields(
-      const autofill::AutofillProfile& profile) const;
+  int GetShippingCompletenessScore(
+      const autofill::AutofillProfile* profile) const;
 
   // Returns true iff every field needed to use |profile| as a shipping address
   // is populated.
   bool IsShippingComplete(const autofill::AutofillProfile* profile) const;
 
+  // Returns a localized string to be displayed in UI indicating what action,
+  // if any, must be taken for the given profile to be used as contact info.
+  base::string16 GetStringForMissingContactFields(
+      const autofill::AutofillProfile& profile) const;
+
   // Returns a localized string to be displayed in UI indicating what action,
   // if any, must be taken for the given profile to be used as a shipping
   // address.
@@ -103,6 +107,14 @@ class PaymentsProfileComparator : public autofill::AutofillProfileComparator {
   base::string16 GetStringForMissingFields(ProfileFields fields) const;
   bool AreRequiredAddressFieldsPresent(
       const autofill::AutofillProfile& profile) const;
+
+  // Comparison functions suitable for sorting profiles by completeness
+  // score with std::sort.
+  bool IsContactMoreComplete(const autofill::AutofillProfile* p1,
+                             const autofill::AutofillProfile* p2) const;
+  bool IsShippingMoreComplete(const autofill::AutofillProfile* p1,
+                              const autofill::AutofillProfile* p2) const;
+
   mutable std::map<std::string, ProfileFields> cache_;
   const PaymentOptionsProvider& options_;
 };

components/payments/core/payments_profile_comparator_unittest.cc

@@ -247,6 +247,103 @@ TEST(PaymentRequestProfileUtilTest, IsContactInfoComplete) {
   EXPECT_TRUE(empty_comp.IsContactInfoComplete(nullptr));
 }
 
+TEST(PaymentRequestProfileUtilTest, FilterProfilesForShipping) {
+  MockPaymentOptionsProvider provider(kRequestShipping);
+  PaymentsProfileComparator comp("en-US", provider);
+
+  AutofillProfile address_only = CreateProfileWithCompleteAddress("", "");
+
+  AutofillProfile no_name = CreateProfileWithCompleteAddress("", "6515553226");
+  AutofillProfile no_phone = CreateProfileWithCompleteAddress("Homer", "");
+
+  AutofillProfile empty = CreateProfileWithContactInfo("", "", "");
+
+  AutofillProfile complete1 =
+      CreateProfileWithCompleteAddress("Homer", "6515553226");
+
+  AutofillProfile partial_address =
+      CreateProfileWithPartialAddress("Homer", "6515553226");
+  AutofillProfile no_address =
+      CreateProfileWithContactInfo("Homer", "", "6515553226");
+
+  AutofillProfile complete2 =
+      CreateProfileWithCompleteAddress("Bart", "6515553226");
+
+  AutofillProfile partial_no_phone =
+      CreateProfileWithPartialAddress("", "6515553226");
+  AutofillProfile partial_no_name =
+      CreateProfileWithPartialAddress("Homer", "");
+
+  std::vector<AutofillProfile*> profiles = {
+      &address_only,     &no_name,         &no_phone,   &empty,
+      &complete1,        &partial_address, &no_address, &complete2,
+      &partial_no_phone, &partial_no_name};
+
+  std::vector<AutofillProfile*> filtered =
+      comp.FilterProfilesForShipping(profiles);
+
+  // Current logic does not remove profiles, only reorder them.
+  ASSERT_EQ(10u, filtered.size());
+
+  // First, the complete profiles should be hoisted to the top, keeping their
+  // relative order.
+  EXPECT_EQ(&complete1, filtered[0]);
+  EXPECT_EQ(&complete2, filtered[1]);
+
+  // Next are profiles with a complete address but missing one other field.
+  EXPECT_EQ(&no_name, filtered[2]);
+  EXPECT_EQ(&no_phone, filtered[3]);
+
+  // A profile with only a complete address should still appear before profiles
+  // with partial/empty addresses.
+  EXPECT_EQ(&address_only, filtered[4]);
+
+  // Profiles with partial/no address then are sorted by whether or not they
+  // have names and/or phone numbers.
+  EXPECT_EQ(&partial_address, filtered[5]);
+  EXPECT_EQ(&no_address, filtered[6]);
+
+  EXPECT_EQ(&partial_no_phone, filtered[7]);
+  EXPECT_EQ(&partial_no_name, filtered[8]);
+
+  EXPECT_EQ(&empty, filtered[9]);
+}
+
+TEST(PaymentRequestProfileUtilTest, GetShippingCompletenessScore) {
+  MockPaymentOptionsProvider provider(kRequestShipping);
+  PaymentsProfileComparator comp("en-US", provider);
+
+  // 12 points for a complete profile: 10 for address, 1 each for name/phone.
+  AutofillProfile p1 = CreateProfileWithCompleteAddress("Homer", "6515553226");
+  EXPECT_EQ(12, comp.GetShippingCompletenessScore(&p1));
+
+  // 11 points if name or phone is missing.
+  AutofillProfile p2 = CreateProfileWithCompleteAddress("", "6515553226");
+  AutofillProfile p3 = CreateProfileWithCompleteAddress("Homer", "");
+  EXPECT_EQ(11, comp.GetShippingCompletenessScore(&p2));
+  EXPECT_EQ(11, comp.GetShippingCompletenessScore(&p3));
+
+  // 10 points for complete address only.
+  AutofillProfile p4 = CreateProfileWithCompleteAddress("", "");
+  EXPECT_EQ(10, comp.GetShippingCompletenessScore(&p4));
+
+  // 2 points for name and phone without address.
+  AutofillProfile p5 = CreateProfileWithContactInfo("Homer", "", "6515553226");
+  EXPECT_EQ(2, comp.GetShippingCompletenessScore(&p5));
+
+  // 1 point for name or phone alone.
+  AutofillProfile p6 = CreateProfileWithContactInfo("Homer", "", "");
+  AutofillProfile p7 = CreateProfileWithContactInfo("", "", "6515553226");
+  EXPECT_EQ(1, comp.GetShippingCompletenessScore(&p6));
+  EXPECT_EQ(1, comp.GetShippingCompletenessScore(&p7));
+
+  // No points for empty profile, or profile with only a partial address.
+  AutofillProfile p8 = CreateProfileWithContactInfo("", "", "");
+  AutofillProfile p9 = CreateProfileWithPartialAddress("", "");
+  EXPECT_EQ(0, comp.GetShippingCompletenessScore(&p8));
+  EXPECT_EQ(0, comp.GetShippingCompletenessScore(&p9));
+}
+
 TEST(PaymentRequestProfileUtilTest, IsShippingComplete) {
   MockPaymentOptionsProvider provider(kRequestShipping);
   PaymentsProfileComparator comp("en-US", provider);

ios/chrome/browser/payments/payment_request.mm

@@ -128,29 +128,29 @@ void PaymentRequest::PopulateProfileCache() {
     return;
 
   profile_cache_.reserve(profiles_to_suggest.size());
+
+  std::vector<autofill::AutofillProfile*> raw_profiles_for_filtering;
+  raw_profiles_for_filtering.reserve(profiles_to_suggest.size());
+
   for (const auto* profile : profiles_to_suggest) {
     profile_cache_.push_back(*profile);
-    shipping_profiles_.push_back(&profile_cache_.back());
-  }
-
-  // If the merchant provided a shipping option, select a suitable default
-  // shipping profile. We pick the profile that is most complete, going down
-  // the list in Frecency order.
-  // TODO(crbug.com/719652): Have a proper ordering of shipping addresses by
-  // completeness.
-  if (selected_shipping_option_) {
-    selected_shipping_profile_ = shipping_profiles_[0];
-    for (autofill::AutofillProfile* profile : shipping_profiles_) {
-      if (profile_comparator_.IsShippingComplete(profile)) {
-        selected_shipping_profile_ = profile;
-        break;
-      }
-    }
+    raw_profiles_for_filtering.push_back(&profile_cache_.back());
   }
 
   // Contact profiles are deduped and ordered in completeness.
   contact_profiles_ =
-      profile_comparator_.FilterProfilesForContact(shipping_profiles_);
+      profile_comparator_.FilterProfilesForContact(raw_profiles_for_filtering);
+
+  // Shipping profiles are ordered by completeness.
+  shipping_profiles_ =
+      profile_comparator_.FilterProfilesForShipping(raw_profiles_for_filtering);
+
+  // If the merchant provided a shipping option, and the highest-ranking
+  // shipping profile is usable, select it.
+  if (selected_shipping_option_ && !shipping_profiles_.empty() &&
+      profile_comparator_.IsShippingComplete(shipping_profiles_[0])) {
+    selected_shipping_profile_ = shipping_profiles_[0];
+  }
 
   // If the highest-ranking contact profile is usable, select it. Otherwise,
   // select none.