AutoFill Empty profiles should not be saved from AutoFillDialog Add and Edit sheets on Mac.

Changes the AutoFillDialog on Mac to not store intermediate results of "Add" and "Edit" actions
when those actions result in empty address or credit card information.  Empty addresses and credit
card information is culled at this level now, not just after "Save" from the main dialog.

BUG=47742
TEST=AutoFillDialogControllerTest.AddNewProfile, AutoFillDialogControllerTest.AddNewEmptyProfile, AutoFillDialogControllerTest.AddNewCreditCard, AutoFillDialogControllerTest.AddNewEmptyCreditCard

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

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

7 files changed, 121 insertions, 31 deletions

diff --git a/chrome/browser/autofill/autofill_dialog_controller_mac.mm b/chrome/browser/autofill/autofill_dialog_controller_mac.mm
index 912d970..594808b 100644
--- a/chrome/browser/autofill/autofill_dialog_controller_mac.mm
+++ b/chrome/browser/autofill/autofill_dialog_controller_mac.mm
@@ -318,16 +318,18 @@ void PersonalDataManagerObserver::OnPersonalDataLoaded() {
     // Create a new address and save it to the |profiles_| list.
     AutoFillProfile newAddress(string16(), 0);
     [addressSheetController copyModelToProfile:&newAddress];
-    profiles_.push_back(newAddress);
+    if (!newAddress.IsEmpty()) {
+      profiles_.push_back(newAddress);
 
-    // Refresh the view based on new data.
-    UpdateProfileLabels(&profiles_);
-    [tableView_ reloadData];
+      // Refresh the view based on new data.
+      UpdateProfileLabels(&profiles_);
+      [tableView_ reloadData];
 
-    // Update the selection to the newly added item.
-    NSInteger row = [self rowFromProfileIndex:profiles_.size() - 1];
-    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:row]
-            byExtendingSelection:NO];
+      // Update the selection to the newly added item.
+      NSInteger row = [self rowFromProfileIndex:profiles_.size() - 1];
+      [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:row]
+              byExtendingSelection:NO];
+    }
   }
   [sheet orderOut:self];
   addressSheetController.reset(nil);
@@ -343,15 +345,17 @@ void PersonalDataManagerObserver::OnPersonalDataLoaded() {
     // Create a new credit card and save it to the |creditCards_| list.
     CreditCard newCreditCard(string16(), 0);
     [creditCardSheetController copyModelToCreditCard:&newCreditCard];
-    creditCards_.push_back(newCreditCard);
+    if (!newCreditCard.IsEmpty()) {
+      creditCards_.push_back(newCreditCard);
 
-    // Refresh the view based on new data.
-    [tableView_ reloadData];
+      // Refresh the view based on new data.
+      [tableView_ reloadData];
 
-    // Update the selection to the newly added item.
-    NSInteger row = [self rowFromCreditCardIndex:creditCards_.size() - 1];
-    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:row]
-            byExtendingSelection:NO];
+      // Update the selection to the newly added item.
+      NSInteger row = [self rowFromCreditCardIndex:creditCards_.size() - 1];
+      [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:row]
+              byExtendingSelection:NO];
+    }
   }
   [sheet orderOut:self];
   creditCardSheetController.reset(nil);
@@ -454,6 +458,14 @@ void PersonalDataManagerObserver::OnPersonalDataLoaded() {
   if (returnCode) {
     AutoFillProfile* profile = static_cast<AutoFillProfile*>(contextInfo);
     [addressSheetController copyModelToProfile:profile];
+
+    if (profile->IsEmpty())
+      [tableView_ deselectAll:self];
+    profiles_.erase(
+        std::remove_if(profiles_.begin(), profiles_.end(),
+                       std::mem_fun_ref(&AutoFillProfile::IsEmpty)),
+        profiles_.end());
+
     UpdateProfileLabels(&profiles_);
     [tableView_ reloadData];
   }
@@ -470,6 +482,14 @@ void PersonalDataManagerObserver::OnPersonalDataLoaded() {
   if (returnCode) {
     CreditCard* creditCard = static_cast<CreditCard*>(contextInfo);
     [creditCardSheetController copyModelToCreditCard:creditCard];
+
+    if (creditCard->IsEmpty())
+      [tableView_ deselectAll:self];
+    creditCards_.erase(
+        std::remove_if(
+            creditCards_.begin(), creditCards_.end(),
+            std::mem_fun_ref(&CreditCard::IsEmpty)),
+        creditCards_.end());
     [tableView_ reloadData];
   }
   [sheet orderOut:self];
diff --git a/chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm b/chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm
index 80678077..96276e4 100644
--- a/chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm
+++ b/chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm
@@ -383,6 +383,9 @@ TEST_F(AutoFillDialogControllerTest, AddNewProfile) {
   [controller_ addNewAddress:nil];
   AutoFillAddressSheetController* sheet = [controller_ addressSheetController];
   ASSERT_TRUE(sheet != nil);
+  AutoFillAddressModel* model = [sheet addressModel];
+  ASSERT_TRUE(model != nil);
+  [model setFullName:@"Don"];
   [sheet save:nil];
   [controller_ save:nil];
 
@@ -393,8 +396,10 @@ TEST_F(AutoFillDialogControllerTest, AddNewProfile) {
   ASSERT_NE(observer_.profiles_.size(), profiles().size());
   ASSERT_EQ(observer_.profiles_.size(), 2UL);
 
-  // New address should match.
+  // New address should match.  Don't compare labels.
   AutoFillProfile new_profile;
+  new_profile.SetInfo(AutoFillType(NAME_FULL), ASCIIToUTF16("Don"));
+  observer_.profiles_[1].set_label(string16());
   ASSERT_EQ(observer_.profiles_[1], new_profile);
 }
 
@@ -407,6 +412,9 @@ TEST_F(AutoFillDialogControllerTest, AddNewCreditCard) {
   AutoFillCreditCardSheetController* sheet =
       [controller_ creditCardSheetController];
   ASSERT_TRUE(sheet != nil);
+  AutoFillCreditCardModel* model = [sheet creditCardModel];
+  ASSERT_TRUE(model != nil);
+  [model setNameOnCard:@"Don"];
   [sheet save:nil];
   [controller_ save:nil];
 
@@ -417,11 +425,60 @@ TEST_F(AutoFillDialogControllerTest, AddNewCreditCard) {
   ASSERT_NE(observer_.credit_cards_.size(), credit_cards().size());
   ASSERT_EQ(observer_.credit_cards_.size(), 2UL);
 
-  // New address should match.
+  // New credit card should match.  Don't compare labels.
   CreditCard new_credit_card;
+  new_credit_card.SetInfo(AutoFillType(CREDIT_CARD_NAME), ASCIIToUTF16("Don"));
+  observer_.credit_cards_[1].set_label(string16());
   ASSERT_EQ(observer_.credit_cards_[1], new_credit_card);
 }
 
+TEST_F(AutoFillDialogControllerTest, AddNewEmptyProfile) {
+  AutoFillProfile profile(string16(), 1);
+  profile.SetInfo(AutoFillType(NAME_FIRST), ASCIIToUTF16("Joe"));
+  profiles().push_back(&profile);
+  LoadDialog();
+  [controller_ addNewAddress:nil];
+  AutoFillAddressSheetController* sheet = [controller_ addressSheetController];
+  ASSERT_TRUE(sheet != nil);
+  [sheet save:nil];
+  [controller_ save:nil];
+
+  // Should hit our observer.
+  ASSERT_TRUE(observer_.hit_);
+
+  // Sizes should be same.  Empty profile should not be saved.
+  ASSERT_EQ(observer_.profiles_.size(), profiles().size());
+  ASSERT_EQ(observer_.profiles_.size(), 1UL);
+
+  // Profile should match original.
+  observer_.profiles_[0].set_label(string16());
+  ASSERT_EQ(observer_.profiles_[0], profile);
+}
+
+TEST_F(AutoFillDialogControllerTest, AddNewEmptyCreditCard) {
+  CreditCard credit_card(string16(), 1);
+  credit_card.SetInfo(AutoFillType(CREDIT_CARD_NAME), ASCIIToUTF16("Joe"));
+  credit_cards().push_back(&credit_card);
+  LoadDialog();
+  [controller_ addNewCreditCard:nil];
+  AutoFillCreditCardSheetController* sheet =
+      [controller_ creditCardSheetController];
+  ASSERT_TRUE(sheet != nil);
+  [sheet save:nil];
+  [controller_ save:nil];
+
+  // Should hit our observer.
+  ASSERT_TRUE(observer_.hit_);
+
+  // Sizes should be same.  Empty credit card should not be saved.
+  ASSERT_EQ(observer_.credit_cards_.size(), credit_cards().size());
+  ASSERT_EQ(observer_.credit_cards_.size(), 1UL);
+
+  // Credit card should match original.
+  observer_.credit_cards_[0].set_label(string16());
+  ASSERT_EQ(observer_.credit_cards_[0], credit_card);
+}
+
 TEST_F(AutoFillDialogControllerTest, DeleteProfile) {
   AutoFillProfile profile(ASCIIToUTF16("One"), 1);
   profile.SetInfo(AutoFillType(NAME_FIRST), ASCIIToUTF16("Joe"));
diff --git a/chrome/browser/autofill/autofill_profile.cc b/chrome/browser/autofill/autofill_profile.cc
index 3aa8fbe..f12b0f8 100644
--- a/chrome/browser/autofill/autofill_profile.cc
+++ b/chrome/browser/autofill/autofill_profile.cc
@@ -205,6 +205,7 @@ string16 AutoFillProfile::PreviewSummary() const {
   return summary_format;
 }
 
+// static
 bool AutoFillProfile::AdjustInferredLabels(
     std::vector<AutoFillProfile*>* profiles) {
   std::vector<string16> created_labels;
@@ -222,6 +223,7 @@ bool AutoFillProfile::AdjustInferredLabels(
   return updated_labels;
 }
 
+// static
 void AutoFillProfile::CreateInferredLabels(
     const std::vector<AutoFillProfile*>* profiles,
     std::vector<string16>* created_labels,
@@ -375,6 +377,12 @@ void AutoFillProfile::CreateInferredLabels(
   }
 }
 
+bool AutoFillProfile::IsEmpty() const {
+  FieldTypeSet types;
+  GetAvailableFieldTypes(&types);
+  return types.empty();
+}
+
 void AutoFillProfile::operator=(const AutoFillProfile& source) {
   label_ = source.label_;
   unique_id_ = source.unique_id_;
diff --git a/chrome/browser/autofill/autofill_profile.h b/chrome/browser/autofill/autofill_profile.h
index ed12b77..17e8528 100644
--- a/chrome/browser/autofill/autofill_profile.h
+++ b/chrome/browser/autofill/autofill_profile.h
@@ -80,6 +80,10 @@ class AutoFillProfile : public FormGroup {
       std::vector<string16>* created_labels,
       size_t minimal_fields_shown,
       AutoFillFieldType exclude_field);
+
+  // Returns true if there are no values (field types) set.
+  bool IsEmpty() const;
+
   // For use in STL containers.
   void operator=(const AutoFillProfile&);
 
diff --git a/chrome/browser/autofill/credit_card.cc b/chrome/browser/autofill/credit_card.cc
index 4a9483e..de2a740 100644
--- a/chrome/browser/autofill/credit_card.cc
+++ b/chrome/browser/autofill/credit_card.cc
@@ -306,6 +306,7 @@ bool CreditCard::operator!=(const CreditCard& creditcard) const {
 }
 
 // Use the Luhn formula to validate the number.
+// static
 bool CreditCard::IsCreditCardNumber(const string16& text) {
   string16 number;
   RemoveChars(text, kCreditCardSeparators.c_str(), &number);
@@ -330,6 +331,13 @@ bool CreditCard::IsCreditCardNumber(const string16& text) {
   return (sum % 10) == 0;
 }
 
+bool CreditCard::IsEmpty() const {
+  FieldTypeSet types;
+  GetAvailableFieldTypes(&types);
+  return types.empty() && billing_address().empty();
+}
+
+
 string16 CreditCard::ExpirationMonthAsString() const {
   if (expiration_month_ == 0)
     return string16();
diff --git a/chrome/browser/autofill/credit_card.h b/chrome/browser/autofill/credit_card.h
index faf4326..85d104f 100644
--- a/chrome/browser/autofill/credit_card.h
+++ b/chrome/browser/autofill/credit_card.h
@@ -59,6 +59,9 @@ class CreditCard : public FormGroup {
   // validate the number.
   static bool IsCreditCardNumber(const string16& text);
 
+  // Returns true if there are no values (field types) set.
+  bool IsEmpty() const;
+
  private:
   // The month and year are zero if not present.
   int Expiration4DigitYear() const { return expiration_year_; }
diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index b4d43f1..e566909 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -27,18 +27,6 @@ const int kMinImportSize = 3;
 
 const char kUnlabeled[] = "Unlabeled";
 
-bool IsEmptyProfile(const AutoFillProfile& profile) {
-  FieldTypeSet types;
-  profile.GetAvailableFieldTypes(&types);
-  return types.empty();
-}
-
-bool IsEmptyCreditCard(const CreditCard& credit_card) {
-  FieldTypeSet types;
-  credit_card.GetAvailableFieldTypes(&types);
-  return types.empty() && credit_card.billing_address().empty();
-}
-
 }  // namespace
 
 PersonalDataManager::~PersonalDataManager() {
@@ -231,7 +219,8 @@ void PersonalDataManager::SetProfiles(std::vector<AutoFillProfile>* profiles) {
 
   // Remove empty profiles from input.
   profiles->erase(
-      std::remove_if(profiles->begin(), profiles->end(), IsEmptyProfile),
+      std::remove_if(profiles->begin(), profiles->end(),
+                     std::mem_fun_ref(&AutoFillProfile::IsEmpty)),
       profiles->end());
 
   SetUniqueProfileLabels(profiles);
@@ -310,7 +299,8 @@ void PersonalDataManager::SetCreditCards(
   // Remove empty credit cards from input.
   credit_cards->erase(
       std::remove_if(
-          credit_cards->begin(), credit_cards->end(), IsEmptyCreditCard),
+          credit_cards->begin(), credit_cards->end(),
+          std::mem_fun_ref(&CreditCard::IsEmpty)),
       credit_cards->end());
 
   SetUniqueCreditCardLabels(credit_cards);