Revert of [WebPayments] Adding FilterProfilesForShipping to profile comparator...

Revert of [WebPayments] Adding FilterProfilesForShipping to profile comparator (patchset #5 id:80001 of https://codereview.chromium.org/2884393002/ )

Reason for revert:
Appears to be causing test failures in https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin10_Tests_x64%2F11929%2F%2B%2Frecipes%2Fsteps%2Fbrowser_side_navigation_browser_tests_on_Windows-10-10586%2F0%2Flogs%2FPaymentRequestProfileListTest.PrioritizeCompleteness%2F0

 RUN      ] PaymentRequestProfileListTest.PrioritizeCompleteness
[640:3644:0523/102806.488:WARNING:embedded_test_server.cc(219)] Request not handled. Returning 404: /favicon.ico
c:\b\c\b\win\src\chrome\browser\ui\views\payments\profile_list_view_controller_browsertest.cc(46): error:       Expected: partial
      Which is: b5e544d1-6a66-49bf-bd4c-f2eb38671fd3 https://www.example.com/ Jane A. Smith jsmith@example.com ACME      48838  US  13105557889
To be equal to: *profiles[0]
      Which is: 63b73e74-aacb-4a0e-bde5-a3260918586c http://www.example.com/ John H. Doe johndoe@hades.com Underworld 666 Erebus St. Apt 8  Elysium CA 91111  US  16502111111
c:\b\c\b\win\src\chrome\browser\ui\views\payments\profile_list_view_controller_browsertest.cc(47): error:       Expected: complete
      Which is: 63b73e74-aacb-4a0e-bde5-a3260918586c http://www.example.com/ John H. Doe johndoe@hades.com Underworld 666 Erebus St. Apt 8  Elysium CA 91111  US  16502111111
To be equal to: *profiles[1]
      Which is: b5e544d1-6a66-49bf-bd4c-f2eb38671fd3 https://www.example.com/ Jane A. Smith jsmith@example.com ACME      48838  US  13105557889
[  FAILED  ] PaymentRequestProfileListTest.PrioritizeCompleteness, where TypeParam =  and GetParam() =  (1292 ms)

Original issue's description:
> [WebPayments] Adding FilterProfilesForShipping to profile comparator
>
> BUG=722949
>
> Review-Url: https://codereview.chromium.org/2884393002
> Cr-Commit-Position: refs/heads/master@{#473929}
> Committed: https://chromium.googlesource.com/chromium/src/+/eb8d574b98adfcbd3b5ce2eb6e521b83efbb428a

TBR=mahmadi@chromium.org,mathp@chromium.org,tmartino@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=722949

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

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

@@ -237,9 +237,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestContactInfoEditorTest,
   OpenContactInfoScreen();
 
   PaymentRequest* request = GetPaymentRequests(GetActiveWebContents()).front();
-
-  // No contact profiles are selected because both are incomplete.
-  EXPECT_EQ(nullptr, request->state()->selected_contact_profile());
+  EXPECT_EQ(request->state()->contact_profiles().front(),
+            request->state()->selected_contact_profile());
 
   views::View* list_view = dialog_view()->GetViewByID(
       static_cast<int>(DialogViewID::CONTACT_INFO_SHEET_LIST_VIEW));
@@ -270,7 +269,6 @@ 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,7 +2058,6 @@ 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,22 +238,26 @@ 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]));
-    raw_profiles_for_filtering.push_back(profile_cache_.back().get());
+
+    shipping_profiles_.push_back(profile_cache_[i].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.
@@ -265,17 +269,22 @@ void PaymentRequestState::PopulateProfileCache() {
 
 void PaymentRequestState::SetDefaultProfileSelections() {
   // Only pre-select an address if the merchant provided at least one selected
-  // 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];
+  // 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;
+      }
+    }
   }
 
   // Contact profiles were ordered by completeness in addition to frecency;
   // the first one is the best default selection.
-  if (!contact_profiles().empty() &&
-      profile_comparator()->IsContactInfoComplete(contact_profiles_[0]))
+  if (!contact_profiles().empty())
     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,6 +118,9 @@ 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();
@@ -134,42 +137,18 @@ 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
@@ -269,16 +248,4 @@ 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,24 +71,20 @@ class PaymentsProfileComparator : public autofill::AutofillProfileComparator {
   // |profile|.
   bool IsContactInfoComplete(const autofill::AutofillProfile* profile) 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;
-
-  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;
+  // 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 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 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 a shipping
   // address.
@@ -107,14 +103,6 @@ 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,103 +247,6 @@ 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,30 +128,30 @@ 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);
-    raw_profiles_for_filtering.push_back(&profile_cache_.back());
+    shipping_profiles_.push_back(&profile_cache_.back());
   }
 
-  // Contact profiles are deduped and ordered in completeness.
-  contact_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])) {
+  // 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;
+      }
+    }
   }
 
+  // Contact profiles are deduped and ordered in completeness.
+  contact_profiles_ =
+      profile_comparator_.FilterProfilesForContact(shipping_profiles_);
+
   // If the highest-ranking contact profile is usable, select it. Otherwise,
   // select none.
   if (!contact_profiles_.empty() &&