This CL ensures we don't store empty values in the autofill form DB.

Also it applies a clean-up to remove any empty values previously stored in the DB.

BUG=6111
TEST=Submit a form and leave some fields empty. Come back to that form, click on a field that was empty. No autofill popup should show up (or if one show up, it should not contains empty values).
Review URL: http://codereview.chromium.org/21217

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

8 files changed, 109 insertions, 22 deletions

diff --git a/chrome/browser/autofill_manager.cc b/chrome/browser/autofill_manager.cc
index d1beb16..8284cb4 100644
--- a/chrome/browser/autofill_manager.cc
+++ b/chrome/browser/autofill_manager.cc
@@ -8,6 +8,12 @@
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/web_contents.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/pref_service.h"
+
+// static
+void AutofillManager::RegisterUserPrefs(PrefService* prefs) {
+  prefs->RegisterBooleanPref(prefs::kFormAutofillEnabled, true);
+}
 
 AutofillManager::AutofillManager(WebContents* web_contents) :
     web_contents_(web_contents),
@@ -108,3 +114,4 @@ void AutofillManager::StoreFormEntriesInWebDatabase(
   profile()->GetWebDataService(Profile::EXPLICIT_ACCESS)->
       AddAutofillFormElements(form.elements);
 }
+
diff --git a/chrome/browser/autofill_manager.h b/chrome/browser/autofill_manager.h
index 72a04d4..3ece658 100644
--- a/chrome/browser/autofill_manager.h
+++ b/chrome/browser/autofill_manager.h
@@ -42,6 +42,8 @@ class AutofillManager : public WebDataServiceConsumer {
   virtual void OnWebDataServiceRequestDone(WebDataService::Handle h,
                                            const WDTypedResult* result);
 
+  static void RegisterUserPrefs(PrefService* prefs);
+
  private:
   void StoreFormEntriesInWebDatabase(const AutofillForm& form);
 
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 2f42e51..af06203 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1018,9 +1018,6 @@ void Browser::RegisterUserPrefs(PrefService* prefs) {
                              net::CookiePolicy::ALLOW_ALL_COOKIES);
   prefs->RegisterBooleanPref(prefs::kShowHomeButton, false);
   prefs->RegisterStringPref(prefs::kRecentlySelectedEncoding, L"");
-  // TODO(peterson): bug #3870 move this to the AutofillManager once it is
-  //                 checked-in.
-  prefs->RegisterBooleanPref(prefs::kFormAutofillEnabled, true);
   prefs->RegisterBooleanPref(prefs::kDeleteBrowsingHistory, true);
   prefs->RegisterBooleanPref(prefs::kDeleteDownloadHistory, true);
   prefs->RegisterBooleanPref(prefs::kDeleteCache, true);
diff --git a/chrome/browser/browser_prefs.cc b/chrome/browser/browser_prefs.cc
index 445130c..0095dfe 100644
--- a/chrome/browser/browser_prefs.cc
+++ b/chrome/browser/browser_prefs.cc
@@ -13,6 +13,7 @@
 #include "chrome/browser/tab_contents/web_contents.h"
 
 #if defined(OS_WIN)  // TODO(port): whittle this down as we port
+#include "chrome/browser/autofill_manager.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/net/dns_global.h"
 #include "chrome/browser/download/download_manager.h"
@@ -62,6 +63,7 @@ void RegisterAllPrefs(PrefService* user_prefs, PrefService* local_state) {
   DownloadManager::RegisterUserPrefs(user_prefs);
   PasswordManager::RegisterUserPrefs(user_prefs);
   SSLManager::RegisterUserPrefs(user_prefs);
+  AutofillManager::RegisterUserPrefs(user_prefs);
 #endif
   TabContents::RegisterUserPrefs(user_prefs);
   TemplateURLPrepopulateData::RegisterUserPrefs(user_prefs);
diff --git a/chrome/browser/webdata/web_database.cc b/chrome/browser/webdata/web_database.cc
index 7ee4f79..5c33c1a 100755
--- a/chrome/browser/webdata/web_database.cc
+++ b/chrome/browser/webdata/web_database.cc
@@ -105,7 +105,7 @@
 using base::Time;
 
 // Current version number.
-static const int kCurrentVersionNumber = 21;
+static const int kCurrentVersionNumber = 22;
 static const int kCompatibleVersionNumber = 21;
 
 // Keys used in the meta table.
@@ -846,6 +846,30 @@ bool WebDatabase::AddAutofillFormElements(
   return result;
 }
 
+bool WebDatabase::ClearAutofillEmptyValueElements() {
+  SQLStatement s;
+
+  if (s.prepare(db_, "SELECT pair_id FROM autofill "
+                     "WHERE TRIM(value)= \"\"") != SQLITE_OK) {
+    NOTREACHED() << "Statement prepare failed";
+    return false;
+  }
+
+  std::set<int64> ids;
+  int result;
+  while ((result = s.step()) == SQLITE_ROW)
+    ids.insert(s.column_int64(0));
+
+  bool success = true;
+  for (std::set<int64>::const_iterator iter = ids.begin(); iter != ids.end();
+       ++iter) {
+    if (!RemoveFormElement(*iter)) 
+      success = false;
+  }
+  
+  return success;
+}
+
 bool WebDatabase::GetIDAndCountOfFormElement(
     const AutofillForm::Element& element, int64* pair_id, int* count) {
   SQLStatement s;
@@ -1054,8 +1078,10 @@ bool WebDatabase::RemoveFormElementsAddedBetween(const Time delete_begin,
        itr != pair_ids.end();
        itr++) {
     int how_many = 0;
-    if (!RemovePairIDAndDate(*itr, delete_begin, delete_end, &how_many))
+    if (!RemoveFormElementForTimeRange(*itr, delete_begin, delete_end,
+                                       &how_many)) {
       return false;
+    }
     if (!AddToCountOfFormElement(*itr, -how_many))
       return false;
   }
@@ -1063,8 +1089,10 @@ bool WebDatabase::RemoveFormElementsAddedBetween(const Time delete_begin,
   return true;
 }
 
-bool WebDatabase::RemovePairIDAndDate(int64 pair_id, const Time delete_begin,
-    const Time delete_end, int* how_many) {
+bool WebDatabase::RemoveFormElementForTimeRange(int64 pair_id,
+                                                const Time delete_begin,
+                                                const Time delete_end,
+                                                int* how_many) {
   SQLStatement s;
   if (s.prepare(db_,
                 "DELETE FROM autofill_dates WHERE pair_id = ? AND "
@@ -1073,14 +1101,13 @@ bool WebDatabase::RemovePairIDAndDate(int64 pair_id, const Time delete_begin,
     return false;
   }
   s.bind_int64(0, pair_id);
-  s.bind_int64(1, delete_begin.ToTimeT());
-  s.bind_int64(2, 
-               delete_end.is_null() ?
-                   std::numeric_limits<int64>::max() :
-                   delete_end.ToTimeT());
+  s.bind_int64(1, delete_begin.is_null() ? 0 : delete_begin.ToTimeT());
+  s.bind_int64(2, delete_end.is_null() ? std::numeric_limits<int64>::max() :
+                                         delete_end.ToTimeT());
 
   bool result = (s.step() == SQLITE_DONE);
-  *how_many = sqlite3_changes(db_);
+  if (how_many)
+    *how_many = sqlite3_changes(db_);
 
   return result;
 }
@@ -1105,12 +1132,14 @@ bool WebDatabase::RemoveFormElement(int64 pair_id) {
   SQLStatement s;
   if (s.prepare(db_,
                 "DELETE FROM autofill WHERE pair_id = ?") != SQLITE_OK) {
-    NOTREACHED() << "Statement 1 prepare failed";
+    NOTREACHED() << "Statement prepare failed";
     return false;
   }
   s.bind_int64(0, pair_id);
+  if (s.step() != SQLITE_DONE)
+    return false;
 
-  return (s.step() == SQLITE_DONE);
+  return RemoveFormElementForTimeRange(pair_id, Time(), Time(), NULL);
 }
 
 void WebDatabase::MigrateOldVersionsAsNeeded() {
@@ -1141,6 +1170,15 @@ void WebDatabase::MigrateOldVersionsAsNeeded() {
           std::min(21, kCompatibleVersionNumber));
       // FALL THROUGH
 
+    case 21:
+      if (!ClearAutofillEmptyValueElements()) {
+        NOTREACHED() << "Failed to clean-up autofill DB.";
+      }
+      meta_table_.SetVersionNumber(22);
+      // No change in the compatibility version number.
+
+      // FALL THROUGH
+
     // Add successive versions here.  Each should set the version number and
     // compatible version number as appropriate, then fall through to the next
     // case.
diff --git a/chrome/browser/webdata/web_database.h b/chrome/browser/webdata/web_database.h
index 54ec840..864440d 100644
--- a/chrome/browser/webdata/web_database.h
+++ b/chrome/browser/webdata/web_database.h
@@ -147,10 +147,10 @@ class WebDatabase {
 
   // Removes from autofill_dates rows with given pair_id where date_created lies
   // between delte_begin and delte_end.
-  bool RemovePairIDAndDate(int64 pair_id,
-                           const base::Time delete_begin,
-                           const base::Time delete_end,
-                           int* how_many);
+  bool RemoveFormElementForTimeRange(int64 pair_id,
+                                     const base::Time delete_begin,
+                                     const 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.
@@ -176,7 +176,7 @@ class WebDatabase {
   // Adds a new row to the autofill_dates table.
   bool InsertPairIDAndDate(int64 pair_id, const base::Time date_created);
 
-  // Removes row from the autofill table given |pair_id|.
+  // Removes row from the autofill tables given |pair_id|.
   bool RemoveFormElement(int64 pair_id);
 
   //////////////////////////////////////////////////////////////////////////////
@@ -194,7 +194,13 @@ class WebDatabase {
   bool RemoveWebApp(const GURL& url);
 
  private:
-  friend class WebDatabaseTest;
+  FRIEND_TEST(WebDatabaseTest, Autofill);
+
+  // Removes empty values for autofill that were incorrectly stored in the DB
+  // (see bug http://crbug.com/6111).
+  // TODO(jcampan): http://crbug.com/7564 remove when we think all users have
+  //                run this code.
+  bool ClearAutofillEmptyValueElements();
 
   bool InitKeywordsTable();
   bool InitLoginsTable();
diff --git a/chrome/browser/webdata/web_database_unittest.cc b/chrome/browser/webdata/web_database_unittest.cc
index 8842e40..d56edbd 100644
--- a/chrome/browser/webdata/web_database_unittest.cc
+++ b/chrome/browser/webdata/web_database_unittest.cc
@@ -464,6 +464,31 @@ TEST_F(WebDatabaseTest, Autofill) {
 
   EXPECT_TRUE(db.GetFormValuesForElementName(L"Name", L"", &v, 6));
   EXPECT_EQ(0U, v.size());
+
+  // Now add some values with empty strings.
+  const std::wstring kValue = L"  toto   ";
+  EXPECT_TRUE(db.AddAutofillFormElement(AutofillForm::Element(L"blank", L"")));
+  EXPECT_TRUE(db.AddAutofillFormElement(AutofillForm::Element(L"blank",
+                                                              L" ")));
+  EXPECT_TRUE(db.AddAutofillFormElement(AutofillForm::Element(L"blank",
+                                                              L"      ")));
+  EXPECT_TRUE(db.AddAutofillFormElement(AutofillForm::Element(L"blank",
+                                                              kValue)));
+
+  // They should be stored normally as the DB layer does not check for empty
+  // values.
+  v.clear();
+  EXPECT_TRUE(db.GetFormValuesForElementName(L"blank", L"", &v, 10));
+  EXPECT_EQ(4U, v.size());
+
+  // Now we'll check that ClearAutofillEmptyValueElements() works as expected.
+  db.ClearAutofillEmptyValueElements();
+
+  v.clear();
+  EXPECT_TRUE(db.GetFormValuesForElementName(L"blank", L"", &v, 10));
+  ASSERT_EQ(1U, v.size());
+
+  EXPECT_EQ(kValue, v[0]);
 }
 
 static bool AddTimestampedLogin(WebDatabase* db, std::string url,
diff --git a/webkit/glue/autofill_form.cc b/webkit/glue/autofill_form.cc
index 9ad9237..ee38b05 100644
--- a/webkit/glue/autofill_form.cc
+++ b/webkit/glue/autofill_form.cc
@@ -15,6 +15,7 @@ MSVC_POP_WARNING();
 
 #include "base/basictypes.h"
 #include "base/logging.h"
+#include "base/string_util.h"
 #include "webkit/glue/autofill_form.h"
 #include "webkit/glue/glue_util.h"
 
@@ -57,9 +58,18 @@ AutofillForm* AutofillForm::CreateAutofillForm(
       continue;
 
     // For each TEXT input field, store the name and value
-    std::wstring name = webkit_glue::StringToStdWString(input_element->name());
     std::wstring value = webkit_glue::StringToStdWString(
         input_element->value());
+    TrimWhitespace(value, TRIM_LEADING, &value);
+    if (value.length() == 0)
+      continue;
+
+    std::wstring name = webkit_glue::StringToStdWString(input_element->name());
+    // Test that the name is not blank.
+    std::wstring trimmed_name;
+    TrimWhitespace(name, TRIM_LEADING, &trimmed_name);
+    if (trimmed_name.length() == 0)
+      continue;
 
     result->elements.push_back(AutofillForm::Element(name, value));
   }