Return a list of changed from WebDatabase::RemoveFormElementsAddedBetween()

This changes add a list of AutofillChange out parameter to the RemoveFormElementsAddedBetween() method in order to communicate exactly what happened to the caller.  This method may only update some autofill entries (by removing use timestamps that fall in the specified time range) or completely remove the autofill entry (if all the use timestamps fall between the specified time range).  For sync, we need to know the difference.

This change also required some new testing method on WebDatabase to make it possible to add new autofill entries with specific timestamps.  This is needed to create a reliable test that requires entries to be added at different times.  If there is a better way to do this, please let me know.

The next change will add the notifications to the WebDataService.

BUG=30169

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

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

9 files changed, 190 insertions, 39 deletions

diff --git a/chrome/browser/webdata/autofill_change.cc b/chrome/browser/webdata/autofill_change.cc
new file mode 100644
index 0000000..ceb8205
--- /dev/null
+++ b/chrome/browser/webdata/autofill_change.cc
@@ -0,0 +1,9 @@
+// Copyright (c) 2009 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/webdata/autofill_change.h"
+
+bool AutofillChange::operator==(const AutofillChange& change) const {
+  return type_ == change.type() && key_ == change.key();
+}
diff --git a/chrome/browser/webdata/autofill_change.h b/chrome/browser/webdata/autofill_change.h
index 2eb8d50..ea16846 100644
--- a/chrome/browser/webdata/autofill_change.h
+++ b/chrome/browser/webdata/autofill_change.h
@@ -23,9 +23,7 @@ class AutofillChange {
   Type type() const { return type_; }
   const AutofillKey& key() const { return key_; }
 
-  bool operator==(const AutofillChange& change) const {
-    return type_ == change.type() && key_ == change.key();
-  }
+  bool operator==(const AutofillChange& change) const;
 
  private:
   Type type_;
diff --git a/chrome/browser/webdata/autofill_entry.cc b/chrome/browser/webdata/autofill_entry.cc
new file mode 100644
index 0000000..7e0b106
--- /dev/null
+++ b/chrome/browser/webdata/autofill_entry.cc
@@ -0,0 +1,9 @@
+// Copyright (c) 2009 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/webdata/autofill_entry.h"
+
+bool AutofillKey::operator==(const AutofillKey& key) const {
+  return name_ == key.name() && value_ == key.value();
+}
diff --git a/chrome/browser/webdata/autofill_entry.h b/chrome/browser/webdata/autofill_entry.h
index 9aadc3a..664cbaeb 100644
--- a/chrome/browser/webdata/autofill_entry.h
+++ b/chrome/browser/webdata/autofill_entry.h
@@ -20,9 +20,7 @@ class AutofillKey {
   const string16& name() const { return name_; }
   const string16& value() const { return value_; }
 
-  bool operator==(const AutofillKey& key) const {
-    return name_ == key.name() && value_ == key.value();
-  }
+  bool operator==(const AutofillKey& key) const;
 
  private:
   string16 name_;
diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc
index ee1d4c9..1b81ac2 100644
--- a/chrome/browser/webdata/web_data_service.cc
+++ b/chrome/browser/webdata/web_data_service.cc
@@ -652,8 +652,10 @@ void WebDataService::RemoveFormElementsAddedBetweenImpl(
     GenericRequest2<Time, Time>* request) {
   InitializeDatabaseIfNecessary();
   if (db_ && !request->IsCancelled()) {
+    AutofillChangeList changes;
     if (db_->RemoveFormElementsAddedBetween(request->GetArgument1(),
-                                            request->GetArgument2()))
+                                            request->GetArgument2(),
+                                            &changes))
       ScheduleCommit();
   }
   request->RequestComplete();
diff --git a/chrome/browser/webdata/web_database.cc b/chrome/browser/webdata/web_database.cc
index b20ca2c..41d5e8b 100644
--- a/chrome/browser/webdata/web_database.cc
+++ b/chrome/browser/webdata/web_database.cc
@@ -10,9 +10,11 @@
 #include "app/l10n_util.h"
 #include "app/sql/statement.h"
 #include "app/sql/transaction.h"
+#include "base/tuple.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 #include "chrome/browser/history/history_database.h"
+#include "chrome/browser/webdata/autofill_change.h"
 #include "webkit/glue/password_form.h"
 
 // Encryptor is the *wrong* way of doing things; we need to turn it into a
@@ -116,6 +118,10 @@ std::string JoinStrings(const std::string& separator,
   return result;
 }
 
+namespace {
+typedef std::vector<Tuple3<int64, string16, string16> > AutofillElementList;
+}
+
 WebDatabase::WebDatabase() {
 }
 
@@ -798,14 +804,18 @@ bool WebDatabase::GetAllLogins(std::vector<PasswordForm*>* forms,
   return s.Succeeded();
 }
 
-bool WebDatabase::AddFormFieldValues(
-    const std::vector<FormField>& elements) {
+bool WebDatabase::AddFormFieldValues(const std::vector<FormField>& elements) {
+  return AddFormFieldValuesTime(elements, Time::Now());
+}
+
+bool WebDatabase::AddFormFieldValuesTime(const std::vector<FormField>& elements,
+                                         base::Time time) {
   bool result = true;
   for (std::vector<FormField>::const_iterator
        itr = elements.begin();
        itr != elements.end();
        itr++) {
-    result = result && AddFormFieldValue(*itr);
+    result = result && AddFormFieldValueTime(*itr, time);
   }
   return result;
 }
@@ -936,6 +946,11 @@ bool WebDatabase::SetCountOfFormElement(int64 pair_id, int count) {
 }
 
 bool WebDatabase::AddFormFieldValue(const FormField& element) {
+  return AddFormFieldValueTime(element, base::Time::Now());
+}
+
+bool WebDatabase::AddFormFieldValueTime(const FormField& element,
+                                        base::Time time) {
   int count = 0;
   int64 pair_id;
 
@@ -946,7 +961,7 @@ bool WebDatabase::AddFormFieldValue(const FormField& element) {
     return false;
 
   return SetCountOfFormElement(pair_id, count + 1) &&
-      InsertPairIDAndDate(pair_id, Time::Now());
+      InsertPairIDAndDate(pair_id, time);
 }
 
 bool WebDatabase::GetFormValuesForElementName(const string16& name,
@@ -998,11 +1013,17 @@ bool WebDatabase::GetFormValuesForElementName(const string16& name,
   return s.Succeeded();
 }
 
-bool WebDatabase::RemoveFormElementsAddedBetween(base::Time delete_begin,
-                                                 base::Time delete_end) {
+bool WebDatabase::RemoveFormElementsAddedBetween(
+    base::Time delete_begin,
+    base::Time delete_end,
+    std::vector<AutofillChange>* changes) {
+  DCHECK(changes);
+  // Query for the pair_id, name, and value of all form elements that
+  // were used between the given times.
   sql::Statement s(db_.GetUniqueStatement(
-      "SELECT DISTINCT pair_id FROM autofill_dates "
-      "WHERE date_created >= ? AND date_created < ?"));
+      "SELECT DISTINCT a.pair_id, a.name, a.value "
+      "FROM autofill_dates ad JOIN autofill a ON ad.pair_id = a.pair_id "
+      "WHERE ad.date_created >= ? AND ad.date_created < ?"));
   if (!s) {
     NOTREACHED() << "Statement 1 prepare failed";
     return false;
@@ -1013,25 +1034,32 @@ bool WebDatabase::RemoveFormElementsAddedBetween(base::Time delete_begin,
                   std::numeric_limits<int64>::max() :
                   delete_end.ToTimeT());
 
-  std::vector<int64> pair_ids;
+  AutofillElementList elements;
   while (s.Step())
-    pair_ids.push_back(s.ColumnInt64(0));
+    elements.push_back(MakeTuple(s.ColumnInt64(0),
+                                 UTF8ToUTF16(s.ColumnString(1)),
+                                 UTF8ToUTF16(s.ColumnString(2))));
 
   if (!s.Succeeded()) {
     NOTREACHED();
     return false;
   }
 
-  for (std::vector<int64>::iterator itr = pair_ids.begin();
-       itr != pair_ids.end();
+  for (AutofillElementList::iterator itr = elements.begin();
+       itr != elements.end();
        itr++) {
     int how_many = 0;
-    if (!RemoveFormElementForTimeRange(*itr, delete_begin, delete_end,
+    if (!RemoveFormElementForTimeRange(itr->a, delete_begin, delete_end,
                                        &how_many)) {
       return false;
     }
-    if (!AddToCountOfFormElement(*itr, -how_many))
+    bool was_removed = false;
+    if (!AddToCountOfFormElement(itr->a, -how_many, &was_removed))
       return false;
+    AutofillChange::Type change_type =
+        was_removed ? AutofillChange::REMOVE : AutofillChange::UPDATE;
+    changes->push_back(AutofillChange(change_type,
+                                      AutofillKey(itr->b, itr->c)));
   }
 
   return true;
@@ -1077,8 +1105,12 @@ bool WebDatabase::RemoveFormElement(const string16& name,
   return false;
 }
 
-bool WebDatabase::AddToCountOfFormElement(int64 pair_id, int delta) {
+bool WebDatabase::AddToCountOfFormElement(int64 pair_id,
+                                          int delta,
+                                          bool* was_removed) {
+  DCHECK(was_removed);
   int count = 0;
+  *was_removed = false;
 
   if (!GetCountOfFormElement(pair_id, &count))
     return false;
@@ -1086,6 +1118,7 @@ bool WebDatabase::AddToCountOfFormElement(int64 pair_id, int delta) {
   if (count + delta == 0) {
     if (!RemoveFormElementForID(pair_id))
       return false;
+    *was_removed = true;
   } else {
     if (!SetCountOfFormElement(pair_id, count + delta))
       return false;
diff --git a/chrome/browser/webdata/web_database.h b/chrome/browser/webdata/web_database.h
index 8d93363..f5451ab 100644
--- a/chrome/browser/webdata/web_database.h
+++ b/chrome/browser/webdata/web_database.h
@@ -14,6 +14,7 @@
 #include "testing/gtest/include/gtest/gtest_prod.h"
 #include "webkit/glue/form_field.h"
 
+class AutofillChange;
 class FilePath;
 
 namespace base {
@@ -144,11 +145,14 @@ class WebDatabase {
                                    int limit);
 
   // Removes rows from autofill_dates if they were created on or after
-  // |delete_begin| and strictly before |delete_end|.  Decrements the count of
-  // the corresponding rows in the autofill table, and removes those rows if the
-  // count goes to 0.
+  // |delete_begin| and strictly before |delete_end|.  Decrements the
+  // count of the corresponding rows in the autofill table, and
+  // removes those rows if the count goes to 0.  A list of all changed
+  // keys and whether each was updater or removed is returned in the
+  // changes out parameter.
   bool RemoveFormElementsAddedBetween(base::Time delete_begin,
-                                      base::Time delete_end);
+                                      base::Time delete_end,
+                                      std::vector<AutofillChange>* changes);
 
   // Removes from autofill_dates rows with given pair_id where date_created lies
   // between delte_begin and delte_end.
@@ -157,9 +161,10 @@ class WebDatabase {
                                      base::Time delete_end,
                                      int* how_many);
 
-  // Increments the count in the row corresponding to |pair_id| by |delta|.
-  // Removes the row from the table if the count becomes 0.
-  bool AddToCountOfFormElement(int64 pair_id, int delta);
+  // Increments the count in the row corresponding to |pair_id| by
+  // |delta|.  Removes the row from the table and sets the
+  // |was_removed| out parameter to true if the count becomes 0.
+  bool AddToCountOfFormElement(int64 pair_id, int delta, bool* was_removed);
 
   // Gets the pair_id and count entries from name and value specified in
   // |element|.  Sets *count to 0 if there is no such row in the table.
@@ -202,6 +207,15 @@ class WebDatabase {
 
  private:
   FRIEND_TEST(WebDatabaseTest, Autofill);
+  FRIEND_TEST(WebDatabaseTest, Autofill_RemoveBetweenChanges);
+
+  // Methods for adding autofill entries at a specified time.  For
+  // testing only.
+  bool AddFormFieldValuesTime(
+      const std::vector<webkit_glue::FormField>& elements,
+      base::Time time);
+  bool AddFormFieldValueTime(const webkit_glue::FormField& element,
+                             base::Time time);
 
   // Removes empty values for autofill that were incorrectly stored in the DB
   // (see bug http://crbug.com/6111).
diff --git a/chrome/browser/webdata/web_database_unittest.cc b/chrome/browser/webdata/web_database_unittest.cc
index beb716d..eb94e4f 100644
--- a/chrome/browser/webdata/web_database_unittest.cc
+++ b/chrome/browser/webdata/web_database_unittest.cc
@@ -9,6 +9,8 @@
 #include "base/time.h"
 #include "base/values.h"
 #include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/webdata/autofill_change.h"
+#include "chrome/browser/webdata/autofill_entry.h"
 #include "chrome/browser/webdata/web_database.h"
 #include "chrome/common/chrome_paths.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -21,6 +23,30 @@ using base::TimeDelta;
 using webkit_glue::FormField;
 using webkit_glue::PasswordForm;
 
+// So we can compare AutofillKeys with EXPECT_EQ().
+std::ostream& operator<<(std::ostream& os, const AutofillKey& key) {
+  return os << UTF16ToASCII(key.name()) << ", " << UTF16ToASCII(key.value());
+}
+
+// So we can compare AutofillChanges with EXPECT_EQ().
+std::ostream& operator<<(std::ostream& os, const AutofillChange& change) {
+  switch (change.type()) {
+    case AutofillChange::ADD: {
+      os << "ADD";
+      break;
+    }
+    case AutofillChange::UPDATE: {
+      os << "UPDATE";
+      break;
+    }
+    case AutofillChange::REMOVE: {
+      os << "REMOVE";
+      break;
+    }
+  }
+  return os << " " << change.key();
+}
+
 class WebDatabaseTest : public testing::Test {
  protected:
 
@@ -87,7 +113,7 @@ class WebDatabaseTest : public testing::Test {
 TEST_F(WebDatabaseTest, Keywords) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   TemplateURL template_url;
   template_url.set_short_name(L"short_name");
@@ -148,7 +174,7 @@ TEST_F(WebDatabaseTest, Keywords) {
 TEST_F(WebDatabaseTest, KeywordMisc) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   ASSERT_EQ(0, db.GetDefaulSearchProviderID());
   ASSERT_EQ(0, db.GetBuitinKeywordVersion());
@@ -163,7 +189,7 @@ TEST_F(WebDatabaseTest, KeywordMisc) {
 TEST_F(WebDatabaseTest, UpdateKeyword) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   TemplateURL template_url;
   template_url.set_short_name(L"short_name");
@@ -225,7 +251,7 @@ TEST_F(WebDatabaseTest, UpdateKeyword) {
 TEST_F(WebDatabaseTest, KeywordWithNoFavicon) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   TemplateURL template_url;
   template_url.set_short_name(L"short_name");
@@ -252,7 +278,7 @@ TEST_F(WebDatabaseTest, KeywordWithNoFavicon) {
 TEST_F(WebDatabaseTest, Logins) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   std::vector<PasswordForm*> result;
 
@@ -385,7 +411,7 @@ TEST_F(WebDatabaseTest, Logins) {
 TEST_F(WebDatabaseTest, Autofill) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   Time t1 = Time::Now();
 
@@ -485,7 +511,27 @@ TEST_F(WebDatabaseTest, Autofill) {
 
   // Removing all elements since the beginning of this function should remove
   // everything from the database.
-  EXPECT_TRUE(db.RemoveFormElementsAddedBetween(t1, Time()));
+  std::vector<AutofillChange> changes;
+  EXPECT_TRUE(db.RemoveFormElementsAddedBetween(t1, Time(), &changes));
+
+  const AutofillChange expected_changes[] = {
+    AutofillChange(AutofillChange::REMOVE,
+                   AutofillKey(ASCIIToUTF16("Name"),
+                               ASCIIToUTF16("Superman"))),
+    AutofillChange(AutofillChange::REMOVE,
+                   AutofillKey(ASCIIToUTF16("Name"),
+                               ASCIIToUTF16("Clark Kent"))),
+    AutofillChange(AutofillChange::REMOVE,
+                   AutofillKey(ASCIIToUTF16("Name"),
+                               ASCIIToUTF16("Clark Sutter"))),
+    AutofillChange(AutofillChange::REMOVE,
+                   AutofillKey(ASCIIToUTF16("Favorite Color"),
+                               ASCIIToUTF16("Green"))),
+  };
+  EXPECT_EQ(arraysize(expected_changes), changes.size());
+  for (size_t i = 0; i < arraysize(expected_changes); i++) {
+    EXPECT_EQ(expected_changes[i], changes[i]);
+  }
 
   EXPECT_TRUE(db.GetIDAndCountOfFormElement(
       FormField(string16(),
@@ -536,6 +582,44 @@ TEST_F(WebDatabaseTest, Autofill) {
   EXPECT_EQ(kValue, v[0]);
 }
 
+TEST_F(WebDatabaseTest, Autofill_RemoveBetweenChanges) {
+  WebDatabase db;
+  ASSERT_TRUE(db.Init(file_));
+
+  TimeDelta one_day(TimeDelta::FromDays(1));
+  Time t1 = Time::Now();
+  Time t2 = t1 + one_day;
+
+  EXPECT_TRUE(db.AddFormFieldValueTime(
+      FormField(string16(),
+                ASCIIToUTF16("Name"),
+                string16(),
+                ASCIIToUTF16("Superman")),
+      t1));
+  EXPECT_TRUE(db.AddFormFieldValueTime(
+      FormField(string16(),
+                ASCIIToUTF16("Name"),
+                string16(),
+                ASCIIToUTF16("Superman")),
+      t2));
+
+  std::vector<AutofillChange> changes;
+  EXPECT_TRUE(db.RemoveFormElementsAddedBetween(t1, t2, &changes));
+  ASSERT_EQ(1U, changes.size());
+  EXPECT_EQ(AutofillChange(AutofillChange::UPDATE,
+                             AutofillKey(ASCIIToUTF16("Name"),
+                                         ASCIIToUTF16("Superman"))),
+            changes[0]);
+  changes.clear();
+
+  EXPECT_TRUE(db.RemoveFormElementsAddedBetween(t2, t2 + one_day, &changes));
+  ASSERT_EQ(1U, changes.size());
+  EXPECT_EQ(AutofillChange(AutofillChange::REMOVE,
+                             AutofillKey(ASCIIToUTF16("Name"),
+                                         ASCIIToUTF16("Superman"))),
+            changes[0]);
+}
+
 static bool AddTimestampedLogin(WebDatabase* db, std::string url,
                                 const std::string& unique_string,
                                 const Time& time) {
@@ -561,7 +645,7 @@ static void ClearResults(std::vector<PasswordForm*>* results) {
 TEST_F(WebDatabaseTest, ClearPrivateData_SavedPasswords) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
 
   std::vector<PasswordForm*> result;
 
@@ -603,7 +687,7 @@ TEST_F(WebDatabaseTest, ClearPrivateData_SavedPasswords) {
 TEST_F(WebDatabaseTest, BlacklistedLogins) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
   std::vector<PasswordForm*> result;
 
   // Verify the database is empty.
@@ -642,7 +726,7 @@ TEST_F(WebDatabaseTest, BlacklistedLogins) {
 TEST_F(WebDatabaseTest, WebAppHasAllImages) {
   WebDatabase db;
 
-  EXPECT_TRUE(db.Init(file_));
+  ASSERT_TRUE(db.Init(file_));
   GURL url("http://google.com/");
 
   // Initial value for unknown web app should be false.
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 13e6383..71ad681 100755
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -1810,6 +1810,10 @@
         'browser/visitedlink_master.h',
         'browser/visitedlink_event_listener.cc',
         'browser/visitedlink_event_listener.h',
+        'browser/webdata/autofill_change.cc',
+        'browser/webdata/autofill_change.h',
+        'browser/webdata/autofill_entry.cc',
+        'browser/webdata/autofill_entry.h',
         'browser/webdata/web_data_service.cc',
         'browser/webdata/web_data_service.h',
         'browser/webdata/web_data_service_win.cc',