Autofill saves duplicate profiles and credit card info in preferences

chrome/browser/autofill/personal_data_manager.cc

Index: chrome/browser/autofill/personal_data_manager.cc
diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index 8554064766ceebd56d20ab168f91a943eaf4adda..c7e15922fd0012e2e674b483352137c96ed4623f 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -54,6 +54,66 @@ class FormGroupGUIDMatchesFunctor<T *> {
 };
 
 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>
@@ -73,6 +133,22 @@ bool FindByGUID(const C& container, const std::string& guid) {
                       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() {
@@ -313,10 +389,11 @@ void PersonalDataManager::SetProfiles(std::vector<AutoFillProfile>* profiles) {
       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);
   }
 
@@ -366,10 +443,11 @@ void PersonalDataManager::SetCreditCards(
       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);
   }