Autofill saves duplicate profiles and credit card info in preferences

chrome/browser/autofill/personal_data_manager.cc

 // Copyright (c) 2010 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 "chrome/browser/autofill/personal_data_manager.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/logging.h"
@@ skipping 36 matching lines @@
 
   bool operator()(const T* form_group) {
     return form_group->guid() == guid_;
   }
 
  private:
   std::string guid_;
 };
 
 template<typename T>
+class FormGroupProfileMatchesFunctor {
+ public:
+  explicit FormGroupProfileMatchesFunctor(const AutoFillProfile& profile)
+      : profile_(profile) {
+  }
+
+  bool operator()(const T& profile) {
+    return profile.Compare(profile_) == 0;
+  }
+
+ private:
+  const AutoFillProfile& profile_;
+};
+
+template<typename T>
+class FormGroupProfileMatchesFunctor<T *> {
+ public:
+  explicit FormGroupProfileMatchesFunctor(const AutoFillProfile& profile)
+      : profile_(profile) {
+  }
+
+  bool operator()(const T* profile) {
+    return profile->Compare(profile_) == 0;
+  }
+
+ private:
+  const AutoFillProfile& profile_;
+};
+
+template<typename T>
+class FormGroupCreditCardMatchesFunctor {
+ public:
+  explicit FormGroupCreditCardMatchesFunctor(const CreditCard& credit_card)
+      : credit_card_(credit_card) {
+  }
+
+  bool operator()(const T& credit_card) {
+    return credit_card.Compare(credit_card_) == 0;
+  }
+
+ private:
+  const CreditCard& credit_card_;
+};
+
+template<typename T>
+class FormGroupCreditCardMatchesFunctor<T *> {
+ public:
+  explicit FormGroupCreditCardMatchesFunctor(const CreditCard& credit_card)
+      : credit_card_(credit_card) {
+  }
+
+  bool operator()(const T* credit_card) {
+    return credit_card->Compare(credit_card_) == 0;
+  }
+
+ private:
+  const CreditCard& credit_card_;
+};

[Ilya Sherman, 2010/12/20 20:57:04]

Wow, that's a lot of new functors.  I think we just need one -- something like
this:

template<typename T>
class FormGroupMatchesByCompareFunctor {
 public:
  explicit FormGroupMatchesByCompareFunctor(const T& form_group)
      : form_group_(form_group) {
  }

  bool operator()(const T* form_group) {
    return form_group->Compare(form_group_) == 0;
  }

 private:
  const T& form_group_;
};

[dhollowa, 2010/12/20 23:16:44]

Yes, better.  But to do this I needed to add a second "operator()" for the const
ref case.

On 2010/12/20 20:57:04, Ilya Sherman wrote:

+
+template<typename T>
 class DereferenceFunctor {
  public:
   template<typename T_Iterator>
   const T& operator()(const T_Iterator& iterator) {
     return *iterator;
   }
 };
 
 template<typename T>
 T* address_of(T& v) {
   return &v;
 }
 
 template<typename T, typename C>
 bool FindByGUID(const C& container, const std::string& guid) {
   return std::find_if(container.begin(), container.end(),
                       FormGroupGUIDMatchesFunctor<T>(guid)) != container.end();
 }
 
+template<typename T, typename C>
+bool FindByContents(const C& container, const AutoFillProfile& profile) {
+  return std::find_if(
+      container.begin(),
+      container.end(),
+      FormGroupProfileMatchesFunctor<T>(profile)) != container.end();
+}
+
+template<typename T, typename C>
+bool FindByContents(const C& container, const CreditCard& credit_card) {
+  return std::find_if(
+      container.begin(),
+      container.end(),
+      FormGroupCreditCardMatchesFunctor<T>(credit_card)) != container.end();
+}

[Ilya Sherman, 2010/12/20 20:57:04]

Likewise, here we just need one function -- something like this:

template<typename T, typename C>
bool FindByContents(const C& container, const T& form_group) {
  return std::find_if(
      container.begin(),
      container.end(),
      FormGroupMatchesByCompareFunctor<T>(form_group)) != container.end();
}

[dhollowa, 2010/12/20 23:16:44]

Done.

+
 }  // namespace
 
 PersonalDataManager::~PersonalDataManager() {
   CancelPendingQuery(&pending_profiles_query_);
   CancelPendingQuery(&pending_creditcards_query_);
 }
 
 void PersonalDataManager::OnWebDataServiceRequestDone(
     WebDataService::Handle h,
     const WDTypedResult* result) {
@@ skipping 220 matching lines @@
       wds->RemoveAutoFillProfileGUID((*iter)->guid());
   }
 
   // Update the web database with the existing profiles.
   for (std::vector<AutoFillProfile>::iterator iter = profiles->begin();
        iter != profiles->end(); ++iter) {
     if (FindByGUID<AutoFillProfile*>(web_profiles_, iter->guid()))
       wds->UpdateAutoFillProfileGUID(*iter);
   }
 
-  // Add the new profiles to the web database.
+  // Add the new profiles to the web database.  Don't add a duplicate.
   for (std::vector<AutoFillProfile>::iterator iter = profiles->begin();
        iter != profiles->end(); ++iter) {
-    if (!FindByGUID<AutoFillProfile*>(web_profiles_, iter->guid()))
+    if (!FindByGUID<AutoFillProfile*>(web_profiles_, iter->guid()) &&
+        !FindByContents<AutoFillProfile*>(web_profiles_, *iter))

[Ilya Sherman, 2010/12/20 20:57:04]

nit: With the above changes, you should be able to call
FindByContents(web_profiles_, *iter) without specifying the template type (and
below too)

[dhollowa, 2010/12/20 23:16:44]

Done.

       wds->AddAutoFillProfileGUID(*iter);
   }
 
   // Copy in the new profiles.
   web_profiles_.reset();
   for (std::vector<AutoFillProfile>::iterator iter = profiles->begin();
        iter != profiles->end(); ++iter) {
     web_profiles_.push_back(new AutoFillProfile(*iter));
   }
 
@@ skipping 29 matching lines @@
       wds->RemoveCreditCardGUID((*iter)->guid());
   }
 
   // Update the web database with the existing credit cards.
   for (std::vector<CreditCard>::iterator iter = credit_cards->begin();
        iter != credit_cards->end(); ++iter) {
     if (FindByGUID<CreditCard*>(credit_cards_, iter->guid()))
       wds->UpdateCreditCardGUID(*iter);
   }
 
-  // Add the new credit cards to the web database.
+  // Add the new credit cards to the web database.  Don't add a duplicate.
   for (std::vector<CreditCard>::iterator iter = credit_cards->begin();
        iter != credit_cards->end(); ++iter) {
-    if (!FindByGUID<CreditCard*>(credit_cards_, iter->guid()))
+    if (!FindByGUID<CreditCard*>(credit_cards_, iter->guid()) &&
+        !FindByContents<CreditCard*>(credit_cards_, *iter))
       wds->AddCreditCardGUID(*iter);
   }
 
   // Copy in the new credit cards.
   credit_cards_.reset();
   for (std::vector<CreditCard>::iterator iter = credit_cards->begin();
        iter != credit_cards->end(); ++iter) {
     credit_cards_.push_back(new CreditCard(*iter));
   }
 
@@ skipping 411 matching lines @@
     }
 
     creditcards.push_back(**iter);
   }
 
   if (!merged)
     creditcards.push_back(*imported_credit_card_);
 
   SetCreditCards(&creditcards);
 }