Changes to web database and autofill components required

to sync profiles / addresses.

The main part here is adding Refresh to the PersonalDataManager.
It changes the expectation that the PDM is the only thing modifying autofill,
which is necessary as the sync engine connects directly to the WebDatabase
on the DB thread.

The tricky part is ID generation, which I spent a great deal of time harping
over in my sync change to make sure that everything is in an eventually
consistent state.  Note that because of the way the autofill window takes an
isolated copy of the data, there *are* extremely rare cases where an edit will
get dropped - I cover this case in PersonalDataManagerTest.Refresh.

TEST=WebDataServiceTest, PersonalDataManagerTest.

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

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

5 files changed, 119 insertions, 46 deletions

diff --git a/chrome/browser/webdata/autofill_change.h b/chrome/browser/webdata/autofill_change.h
index 2b7be32..b1108ad 100644
--- a/chrome/browser/webdata/autofill_change.h
+++ b/chrome/browser/webdata/autofill_change.h
@@ -46,26 +46,36 @@ class AutofillChange : public GenericAutofillChange<AutofillKey> {
   }
 };
 
-class AutofillProfileChange : public GenericAutofillChange<int> {
+class AutofillProfileChange : public GenericAutofillChange<string16> {
  public:
-  // If t == REMOVE, |p| should be NULL.
-  AutofillProfileChange(Type t, int k, const AutoFillProfile* p)
-      : GenericAutofillChange<int>(t, k), profile_(p) {}
+  // If t == REMOVE, |p| should be NULL. |pre_update_label| only applies to
+  // UPDATE changes.
+  AutofillProfileChange(Type t, string16 k, const AutoFillProfile* p,
+      const string16& pre_update_label)
+      : GenericAutofillChange<string16>(t, k), profile_(p),
+        pre_update_label_(pre_update_label) {}
 
   const AutoFillProfile* profile() const { return profile_; }
+  const string16& pre_update_label() const { return pre_update_label_; }
   bool operator==(const AutofillProfileChange& change) const {
-    return type() == change.type() && key() == change.key() &&
-        (type() != REMOVE) ? *profile() == *change.profile() : true;
+    if (type() != change.type() || key() != change.key())
+      return false;
+    if (type() == REMOVE)
+      return true;
+    if (*profile() != *change.profile())
+      return false;
+    return type() == ADD || pre_update_label_ == change.pre_update_label();
   }
  private:
   const AutoFillProfile* profile_;  // Unowned pointer, can be NULL.
+  const string16 pre_update_label_;
 };
 
-class AutofillCreditCardChange : public GenericAutofillChange<int> {
+class AutofillCreditCardChange : public GenericAutofillChange<string16> {
  public:
   // If t == REMOVE, |card| should be NULL.
-  AutofillCreditCardChange(Type t, int k, const CreditCard* card)
-      : GenericAutofillChange<int>(t, k), credit_card_(card) {}
+  AutofillCreditCardChange(Type t, string16 k, const CreditCard* card)
+      : GenericAutofillChange<string16>(t, k), credit_card_(card) {}
 
   const CreditCard* credit_card() const { return credit_card_; }
   bool operator==(const AutofillCreditCardChange& change) const {
diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc
index 8811e10..1874564 100644
--- a/chrome/browser/webdata/web_data_service.cc
+++ b/chrome/browser/webdata/web_data_service.cc
@@ -789,7 +789,7 @@ void WebDataService::AddAutoFillProfileImpl(
     ScheduleCommit();
 
     AutofillProfileChange change(AutofillProfileChange::ADD,
-                                 profile.unique_id(), &profile);
+                                 profile.Label(), &profile, string16());
     NotificationService::current()->Notify(
         NotificationType::AUTOFILL_PROFILE_CHANGED,
         NotificationService::AllSources(),
@@ -803,16 +803,25 @@ void WebDataService::UpdateAutoFillProfileImpl(
   InitializeDatabaseIfNecessary();
   if (db_ && !request->IsCancelled()) {
     const AutoFillProfile& profile = request->GetArgument();
-    if (!db_->UpdateAutoFillProfile(profile))
+    // The AUTOFILL_PROFILE_CHANGED contract for an update requires that we
+    // send along the label of the un-updated profile, to detect label
+    // changes separately.  So first, we query for the existing profile.
+    AutoFillProfile* old_profile = NULL;
+    if (!db_->GetAutoFillProfileForID(profile.unique_id(), &old_profile))
       NOTREACHED();
-    ScheduleCommit();
+    if (old_profile) {
+      if (!db_->UpdateAutoFillProfile(profile))
+        NOTREACHED();
+      ScheduleCommit();
 
-    AutofillProfileChange change(AutofillProfileChange::UPDATE,
-                                 profile.unique_id(), &profile);
-    NotificationService::current()->Notify(
-        NotificationType::AUTOFILL_PROFILE_CHANGED,
-        NotificationService::AllSources(),
-        Details<AutofillProfileChange>(&change));
+      AutofillProfileChange change(AutofillProfileChange::UPDATE,
+                                   profile.Label(), &profile,
+                                   old_profile->Label());
+      NotificationService::current()->Notify(
+          NotificationType::AUTOFILL_PROFILE_CHANGED,
+          NotificationService::AllSources(),
+          Details<AutofillProfileChange>(&change));
+    }
   }
   request->RequestComplete();
 }
@@ -822,16 +831,23 @@ void WebDataService::RemoveAutoFillProfileImpl(
   InitializeDatabaseIfNecessary();
   if (db_ && !request->IsCancelled()) {
     int profile_id = request->GetArgument();
-    if (!db_->RemoveAutoFillProfile(profile_id))
+    AutoFillProfile* dead_profile = NULL;
+    if (!db_->GetAutoFillProfileForID(profile_id, &dead_profile))
       NOTREACHED();
-    ScheduleCommit();
 
-    AutofillProfileChange change(AutofillProfileChange::REMOVE,
-                                 profile_id, NULL);
-    NotificationService::current()->Notify(
-        NotificationType::AUTOFILL_PROFILE_CHANGED,
-        NotificationService::AllSources(),
-        Details<AutofillProfileChange>(&change));
+    if (dead_profile) {
+      if (!db_->RemoveAutoFillProfile(profile_id))
+        NOTREACHED();
+      ScheduleCommit();
+
+      AutofillProfileChange change(AutofillProfileChange::REMOVE,
+                                   dead_profile->Label(),
+                                   NULL, string16());
+      NotificationService::current()->Notify(
+          NotificationType::AUTOFILL_PROFILE_CHANGED,
+          NotificationService::AllSources(),
+          Details<AutofillProfileChange>(&change));
+    }
   }
   request->RequestComplete();
 }
@@ -858,7 +874,7 @@ void WebDataService::AddCreditCardImpl(
     ScheduleCommit();
 
     AutofillCreditCardChange change(AutofillCreditCardChange::ADD,
-                                    creditcard.unique_id(), &creditcard);
+        creditcard.Label(), &creditcard);
     NotificationService::current()->Notify(
         NotificationType::AUTOFILL_CREDIT_CARD_CHANGED,
         NotificationService::AllSources(),
@@ -877,7 +893,7 @@ void WebDataService::UpdateCreditCardImpl(
     ScheduleCommit();
 
     AutofillCreditCardChange change(AutofillCreditCardChange::UPDATE,
-                                    creditcard.unique_id(), &creditcard);
+        creditcard.Label(), &creditcard);
     NotificationService::current()->Notify(
         NotificationType::AUTOFILL_CREDIT_CARD_CHANGED,
         NotificationService::AllSources(),
@@ -891,16 +907,21 @@ void WebDataService::RemoveCreditCardImpl(
   InitializeDatabaseIfNecessary();
   if (db_ && !request->IsCancelled()) {
     int creditcard_id = request->GetArgument();
+    CreditCard* dead_card = NULL;
+    if (!db_->GetCreditCardForID(creditcard_id, &dead_card))
+      NOTREACHED();
     if (!db_->RemoveCreditCard(creditcard_id))
       NOTREACHED();
     ScheduleCommit();
 
-    AutofillCreditCardChange change(AutofillCreditCardChange::REMOVE,
-                                    creditcard_id, NULL);
-    NotificationService::current()->Notify(
-        NotificationType::AUTOFILL_CREDIT_CARD_CHANGED,
-        NotificationService::AllSources(),
-        Details<AutofillCreditCardChange>(&change));
+    if (dead_card) {
+      AutofillCreditCardChange change(AutofillCreditCardChange::REMOVE,
+                                      dead_card->Label(), NULL);
+      NotificationService::current()->Notify(
+          NotificationType::AUTOFILL_CREDIT_CARD_CHANGED,
+          NotificationService::AllSources(),
+          Details<AutofillCreditCardChange>(&change));
+    }
   }
   request->RequestComplete();
 }
diff --git a/chrome/browser/webdata/web_data_service_unittest.cc b/chrome/browser/webdata/web_data_service_unittest.cc
index d615902..80688a8 100644
--- a/chrome/browser/webdata/web_data_service_unittest.cc
+++ b/chrome/browser/webdata/web_data_service_unittest.cc
@@ -308,7 +308,7 @@ TEST_F(WebDataServiceAutofillTest,FormFillRemoveMany) {
 TEST_F(WebDataServiceAutofillTest, ProfileAdd) {
   AutoFillProfile profile(name1_, unique_id1_);
   const AutofillProfileChange expected_change(
-      AutofillProfileChange::ADD, unique_id1_, &profile);
+      AutofillProfileChange::ADD, name1_, &profile, string16());
 
   EXPECT_CALL(
       *observer_helper_->observer(),
@@ -331,7 +331,7 @@ TEST_F(WebDataServiceAutofillTest, ProfileRemove) {
   done_event_.TimedWait(test_timeout_);
 
   const AutofillProfileChange expected_change(
-      AutofillProfileChange::REMOVE, unique_id1_, NULL);
+      AutofillProfileChange::REMOVE, name1_, NULL, string16());
   EXPECT_CALL(
       *observer_helper_->observer(),
       Observe(NotificationType(NotificationType::AUTOFILL_PROFILE_CHANGED),
@@ -358,9 +358,10 @@ TEST_F(WebDataServiceAutofillTest, ProfileUpdate) {
   done_event_.TimedWait(test_timeout_);
 
   AutoFillProfile profile1_delta(profile1);
-  profile1_delta.set_label(ASCIIToUTF16("new_label!"));
+  string16 new_label(ASCIIToUTF16("new_label!"));
+  profile1_delta.set_label(new_label);
   const AutofillProfileChange expected_change(
-      AutofillProfileChange::UPDATE, unique_id1_, &profile1_delta);
+      AutofillProfileChange::UPDATE, new_label, &profile1_delta, name1_);
 
   EXPECT_CALL(
       *observer_helper_->observer(),
@@ -377,7 +378,7 @@ TEST_F(WebDataServiceAutofillTest, ProfileUpdate) {
 TEST_F(WebDataServiceAutofillTest, CreditAdd) {
   CreditCard card(name1_, unique_id1_);
   const AutofillCreditCardChange expected_change(
-      AutofillCreditCardChange::ADD, unique_id1_, &card);
+      AutofillCreditCardChange::ADD, name1_, &card);
 
   EXPECT_CALL(
       *observer_helper_->observer(),
@@ -399,7 +400,7 @@ TEST_F(WebDataServiceAutofillTest, CreditRemove) {
   done_event_.TimedWait(test_timeout_);
 
   const AutofillCreditCardChange expected_change(
-      AutofillCreditCardChange::REMOVE, unique_id1_, NULL);
+      AutofillCreditCardChange::REMOVE, name1_, NULL);
 
   EXPECT_CALL(
       *observer_helper_->observer(),
@@ -428,7 +429,7 @@ TEST_F(WebDataServiceAutofillTest, CreditUpdate) {
   CreditCard card1_delta(card1);
   card1_delta.set_label(ASCIIToUTF16("new_label!"));
   const AutofillCreditCardChange expected_change(
-      AutofillCreditCardChange::UPDATE, unique_id1_, &card1_delta);
+      AutofillCreditCardChange::UPDATE, name1_, &card1_delta);
 
   EXPECT_CALL(
       *observer_helper_->observer(),
diff --git a/chrome/browser/webdata/web_database.cc b/chrome/browser/webdata/web_database.cc
index 91bb373..e447881 100644
--- a/chrome/browser/webdata/web_database.cc
+++ b/chrome/browser/webdata/web_database.cc
@@ -1519,6 +1519,23 @@ bool WebDatabase::RemoveAutoFillProfile(int profile_id) {
   return s.Run();
 }
 
+bool WebDatabase::GetAutoFillProfileForID(int profile_id,
+    AutoFillProfile** profile) {
+  sql::Statement s(db_.GetUniqueStatement(
+      "SELECT * FROM autofill_profiles "
+      "WHERE unique_id = ?"));
+  if (!s) {
+    NOTREACHED() << "Statement prepare failed";
+    return false;
+  }
+
+  s.BindInt(0, profile_id);
+  if (s.Step())
+    *profile = AutoFillProfileFromStatement(s);
+
+  return s.Succeeded();
+}
+
 static void BindCreditCardToStatement(const CreditCard& creditcard,
                                       sql::Statement* s) {
   s->BindString(0, UTF16ToUTF8(creditcard.Label()));
@@ -1602,6 +1619,24 @@ bool WebDatabase::GetCreditCardForLabel(const string16& label,
   return s.Succeeded();
 }
 
+bool WebDatabase::GetCreditCardForID(int card_id, CreditCard** card) {
+  sql::Statement s(db_.GetUniqueStatement(
+      "SELECT * FROM credit_cards "
+      "WHERE unique_id = ?"));
+  if (!s) {
+    NOTREACHED() << "Statement prepare failed";
+    return false;
+  }
+
+  s.BindInt(0, card_id);
+  if (!s.Step())
+    return false;
+
+  *card = CreditCardFromStatement(s);
+
+  return s.Succeeded();
+}
+
 bool WebDatabase::GetCreditCards(
     std::vector<CreditCard*>* creditcards) {
   DCHECK(creditcards);
diff --git a/chrome/browser/webdata/web_database.h b/chrome/browser/webdata/web_database.h
index 06727e9..07f30a9 100644
--- a/chrome/browser/webdata/web_database.h
+++ b/chrome/browser/webdata/web_database.h
@@ -217,21 +217,24 @@ class WebDatabase {
   virtual bool UpdateAutofillEntries(const std::vector<AutofillEntry>& entries);
 
   // Records a single AutoFill profile in the autofill_profiles table.
-  bool AddAutoFillProfile(const AutoFillProfile& profile);
+  virtual bool AddAutoFillProfile(const AutoFillProfile& profile);
 
   // Updates the database values for the specified profile.
-  bool UpdateAutoFillProfile(const AutoFillProfile& profile);
+  virtual bool UpdateAutoFillProfile(const AutoFillProfile& profile);
 
   // Removes a row from the autofill_profiles table.  |profile_id| is the
   // unique ID of the profile to remove.
-  bool RemoveAutoFillProfile(int profile_id);
+  virtual bool RemoveAutoFillProfile(int profile_id);
+
+  // Retrieves profile for unique id |profile_id|, owned by caller.
+  bool GetAutoFillProfileForID(int profile_id, AutoFillProfile** profile);
 
   // Retrieves a profile with label |label|.  The caller owns |profile|.
   bool GetAutoFillProfileForLabel(const string16& label,
                                   AutoFillProfile** profile);
 
   // Retrieves all profiles in the database.  Caller owns the returned profiles.
-  bool GetAutoFillProfiles(std::vector<AutoFillProfile*>* profiles);
+  virtual bool GetAutoFillProfiles(std::vector<AutoFillProfile*>* profiles);
 
   // Records a single credit card in the credit_cards table.
   bool AddCreditCard(const CreditCard& creditcard);
@@ -247,8 +250,11 @@ class WebDatabase {
   bool GetCreditCardForLabel(const string16& label,
                                   CreditCard** profile);
 
+  // Retrieves credit card for a card with unique id |card_id|.
+  bool GetCreditCardForID(int card_id, CreditCard** card);
+
   // Retrieves all profiles in the database.  Caller owns the returned profiles.
-  bool GetCreditCards(std::vector<CreditCard*>* profiles);
+  virtual bool GetCreditCards(std::vector<CreditCard*>* profiles);
 
   //////////////////////////////////////////////////////////////////////////////
   //