Autofill saves duplicate profiles and credit card info in preferences

Adds logic to filter out duplicate profiles and credit cards when the Autofill preferences dialog commits changes.

BUG=67395
TEST=Manual according to bug.

Review URL: http://codereview.chromium.org/6082001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69795 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 98 insertions, 36 deletions

diff --git a/chrome/browser/autofill/autofill-inl.h b/chrome/browser/autofill/autofill-inl.h
new file mode 100644
index 0000000..66e0df2
--- /dev/null
+++ b/chrome/browser/autofill/autofill-inl.h
@@ -0,0 +1,36 @@
+// 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.
+
+#ifndef CHROME_BROWSER_AUTOFILL_AUTOFILL_INL_H_
+#define CHROME_BROWSER_AUTOFILL_AUTOFILL_INL_H_
+#pragma once
+
+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;
+  }
+
+  bool operator()(const T& form_group) {
+    return form_group.Compare(form_group_) == 0;
+  }
+
+ private:
+  const T& form_group_;
+};
+
+template<typename C, typename T>
+bool FindByContents(const C& container, const T& form_group) {
+  return std::find_if(
+      container.begin(),
+      container.end(),
+      FormGroupMatchesByCompareFunctor<T>(form_group)) != container.end();
+}
+
+#endif  // CHROME_BROWSER_AUTOFILL_AUTOFILL_INL_H_
diff --git a/chrome/browser/autofill/autofill_dialog_controller_mac.mm b/chrome/browser/autofill/autofill_dialog_controller_mac.mm
index ac17b07..713c172 100644
--- a/chrome/browser/autofill/autofill_dialog_controller_mac.mm
+++ b/chrome/browser/autofill/autofill_dialog_controller_mac.mm
@@ -12,6 +12,7 @@
 #import "chrome/browser/autofill/autofill_address_sheet_controller_mac.h"
 #import "chrome/browser/autofill/autofill_credit_card_model_mac.h"
 #import "chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h"
+#import "chrome/browser/autofill/autofill-inl.h"
 #import "chrome/browser/autofill/personal_data_manager.h"
 #include "chrome/browser/browser_process.h"
 #import "chrome/browser/ui/cocoa/window_size_autosaver.h"
@@ -94,6 +95,14 @@ static base::LazyInstance<ProfileControllerMap> g_profile_controller_map(
 // UI.
 - (void)preferenceDidChange:(const std::string&)preferenceName;
 
+// Adjust the selected index when underlying data changes.
+// Selects the previous row if possible, else current row, else deselect all.
+- (void) adjustSelectionOnDelete:(NSInteger)selectedRow;
+
+// Adjust the selected index when underlying data changes.
+// Selects the current row if possible, else previous row, else deselect all.
+- (void) adjustSelectionOnReload:(NSInteger)selectedRow;
+
 // Returns true if |row| is an index to a valid profile in |tableView_|, and
 // false otherwise.
 - (BOOL)isProfileRow:(NSInteger)row;
@@ -332,7 +341,7 @@ class PreferenceObserver : public NotificationObserver {
     // Create a new address and save it to the |profiles_| list.
     AutoFillProfile newAddress;
     [addressSheetController copyModelToProfile:&newAddress];
-    if (!newAddress.IsEmpty()) {
+    if (!newAddress.IsEmpty() && !FindByContents(profiles_, newAddress)) {
       profiles_.push_back(newAddress);
 
       // Saving will save to the PDM and the table will refresh when PDM sends
@@ -359,7 +368,8 @@ class PreferenceObserver : public NotificationObserver {
     // Create a new credit card and save it to the |creditCards_| list.
     CreditCard newCreditCard;
     [creditCardSheetController copyModelToCreditCard:&newCreditCard];
-    if (!newCreditCard.IsEmpty()) {
+    if (!newCreditCard.IsEmpty() &&
+        !FindByContents(creditCards_, newCreditCard)) {
       creditCards_.push_back(newCreditCard);
 
       // Saving will save to the PDM and the table will refresh when PDM sends
@@ -400,15 +410,7 @@ class PreferenceObserver : public NotificationObserver {
   }
 
   // Select the previous row if possible, else current row, else deselect all.
-  if ([self tableView:tableView_ shouldSelectRow:selectedRow-1]) {
-    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:selectedRow-1]
-            byExtendingSelection:NO];
-  } else if ([self tableView:tableView_ shouldSelectRow:selectedRow]) {
-    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:selectedRow]
-            byExtendingSelection:NO];
-  } else {
-    [tableView_ deselectAll:self];
-  }
+  [self adjustSelectionOnDelete:selectedRow];
 
   // Saving will save to the PDM and the table will refresh when PDM sends
   // notification that the underlying model has changed.
@@ -799,6 +801,7 @@ class PreferenceObserver : public NotificationObserver {
        iter != creditCards.end(); ++iter)
     creditCards_.push_back(**iter);
 
+  [self adjustSelectionOnReload:[tableView_ selectedRow]];
   [tableView_ reloadData];
 }
 
@@ -815,6 +818,30 @@ class PreferenceObserver : public NotificationObserver {
   }
 }
 
+- (void) adjustSelectionOnDelete:(NSInteger)selectedRow {
+  if ([self tableView:tableView_ shouldSelectRow:selectedRow-1]) {
+    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:selectedRow-1]
+            byExtendingSelection:NO];
+  } else if ([self tableView:tableView_ shouldSelectRow:selectedRow]) {
+    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:selectedRow]
+            byExtendingSelection:NO];
+  } else {
+    [tableView_ deselectAll:self];
+  }
+}
+
+- (void) adjustSelectionOnReload:(NSInteger)selectedRow {
+  if ([self tableView:tableView_ shouldSelectRow:selectedRow]) {
+    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:selectedRow]
+            byExtendingSelection:NO];
+  } else if ([self tableView:tableView_ shouldSelectRow:selectedRow-1]) {
+    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:selectedRow-1]
+            byExtendingSelection:NO];
+  } else {
+    [tableView_ deselectAll:self];
+  }
+}
+
 - (BOOL)isProfileRow:(NSInteger)row {
   if (row > 0 && static_cast<size_t>(row) <= profiles_.size())
     return YES;
diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index 8554064..75f89ce 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -11,6 +11,7 @@
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill_field.h"
+#include "chrome/browser/autofill/autofill-inl.h"
 #include "chrome/browser/autofill/form_structure.h"
 #include "chrome/browser/autofill/phone_number.h"
 #include "chrome/browser/browser_thread.h"
@@ -28,23 +29,16 @@ const int kMinProfileImportSize = 3;
 const int kMinCreditCardImportSize = 2;
 
 template<typename T>
-class FormGroupGUIDMatchesFunctor {
+class FormGroupMatchesByGUIDFunctor {
  public:
-  explicit FormGroupGUIDMatchesFunctor(const std::string& guid) : guid_(guid) {}
+  explicit FormGroupMatchesByGUIDFunctor(const std::string& guid)
+      : guid_(guid) {
+  }
 
   bool operator()(const T& form_group) {
     return form_group.guid() == guid_;
   }
 
- private:
-  std::string guid_;
-};
-
-template<typename T>
-class FormGroupGUIDMatchesFunctor<T *> {
- public:
-  explicit FormGroupGUIDMatchesFunctor(const std::string& guid) : guid_(guid) {}
-
   bool operator()(const T* form_group) {
     return form_group->guid() == guid_;
   }
@@ -53,6 +47,14 @@ class FormGroupGUIDMatchesFunctor<T *> {
   std::string guid_;
 };
 
+template<typename T, typename C>
+bool FindByGUID(const C& container, const std::string& guid) {
+  return std::find_if(
+      container.begin(),
+      container.end(),
+      FormGroupMatchesByGUIDFunctor<T>(guid)) != container.end();
+}
+
 template<typename T>
 class DereferenceFunctor {
  public:
@@ -67,12 +69,6 @@ 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();
-}
-
 }  // namespace
 
 PersonalDataManager::~PersonalDataManager() {
@@ -309,14 +305,15 @@ void PersonalDataManager::SetProfiles(std::vector<AutoFillProfile>* profiles) {
   // 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()))
+    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(web_profiles_, *iter))
       wds->AddAutoFillProfileGUID(*iter);
   }
 
@@ -362,14 +359,15 @@ void PersonalDataManager::SetCreditCards(
   // 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()))
+    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(credit_cards_, *iter))
       wds->AddCreditCardGUID(*iter);
   }
 
@@ -479,7 +477,7 @@ void PersonalDataManager::RemoveProfile(const std::string& guid) {
   // Remove the profile that matches |guid|.
   profiles.erase(
       std::remove_if(profiles.begin(), profiles.end(),
-                     FormGroupGUIDMatchesFunctor<AutoFillProfile>(guid)),
+                     FormGroupMatchesByGUIDFunctor<AutoFillProfile>(guid)),
       profiles.end());
 
   SetProfiles(&profiles);
@@ -535,7 +533,7 @@ void PersonalDataManager::RemoveCreditCard(const std::string& guid) {
   // Remove the credit card that matches |guid|.
   credit_cards.erase(
       std::remove_if(credit_cards.begin(), credit_cards.end(),
-                     FormGroupGUIDMatchesFunctor<CreditCard>(guid)),
+                     FormGroupMatchesByGUIDFunctor<CreditCard>(guid)),
       credit_cards.end());
 
   SetCreditCards(&credit_cards);
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 45abad8..b5fabe1 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -183,6 +183,7 @@
         'browser/autofill/autofill_type.h',
         'browser/autofill/autofill_xml_parser.cc',
         'browser/autofill/autofill_xml_parser.h',
+        'browser/autofill/autofill-inl.h',
         'browser/autofill/billing_address.h',
         'browser/autofill/contact_info.cc',
         'browser/autofill/contact_info.h',