PasswordStore: Clean up expectations about rewriting vectors of forms

components/password_manager/core/browser/login_database.cc

Index: components/password_manager/core/browser/login_database.cc
diff --git a/components/password_manager/core/browser/login_database.cc b/components/password_manager/core/browser/login_database.cc
index 44dff2ad95611233707014e0e903f6b8f5b6e428..c561959be61c93cec27ccf8b5b591ab49a5b2753 100644
--- a/components/password_manager/core/browser/login_database.cc
+++ b/components/password_manager/core/browser/login_database.cc
@@ -657,9 +657,10 @@ bool LoginDatabase::RemoveLoginsSyncedBetween(base::Time delete_begin,
   return s.Run();
 }
 
+// static
 LoginDatabase::EncryptionResult LoginDatabase::InitPasswordFormFromStatement(
     PasswordForm* form,
-    sql::Statement& s) const {
+    sql::Statement& s) {
   std::string encrypted_password;
   s.ColumnBlobAsString(COLUMN_PASSWORD_VALUE, &encrypted_password);
   base::string16 decrypted_password;
@@ -717,6 +718,7 @@ bool LoginDatabase::GetLogins(
     const PasswordForm& form,
     ScopedVector<autofill::PasswordForm>* forms) const {
   DCHECK(forms);
+  forms->clear();
   // You *must* change LoginTableColumns if this query changes.
   const std::string sql_query =
       "SELECT origin_url, action_url, "
@@ -730,7 +732,6 @@ bool LoginDatabase::GetLogins(
   sql::Statement s;
   const GURL signon_realm(form.signon_realm);
   std::string registered_domain = GetRegistryControlledDomain(signon_realm);
-  PSLDomainMatchMetric psl_domain_match_metric = PSL_DOMAIN_MATCH_NONE;
   const bool should_PSL_matching_apply =
       form.scheme == PasswordForm::SCHEME_HTML &&
       ShouldPSLDomainMatchingApply(registered_domain);
@@ -765,49 +766,15 @@ bool LoginDatabase::GetLogins(
     s.BindString(0, form.signon_realm);
     s.BindString(1, regexp);
   } else {
-    psl_domain_match_metric = PSL_DOMAIN_MATCH_NOT_USED;
+    UMA_HISTOGRAM_ENUMERATION("PasswordManager.PslDomainMatchTriggering",
+                              PSL_DOMAIN_MATCH_NOT_USED,
+                              PSL_DOMAIN_MATCH_COUNT);
     s.Assign(db_.GetCachedStatement(SQL_FROM_HERE, sql_query.c_str()));
     s.BindString(0, form.signon_realm);
   }
 
-  while (s.Step()) {
-    scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
-    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
-      return false;
-    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
-      continue;
-    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
-    if (should_PSL_matching_apply) {
-      if (!IsPublicSuffixDomainMatch(new_form->signon_realm,
-                                     form.signon_realm)) {
-        // The database returned results that should not match. Skipping result.
-        continue;
-      }
-      if (form.signon_realm != new_form->signon_realm) {
-        // Ignore non-HTML matches.
-        if (new_form->scheme != PasswordForm::SCHEME_HTML)
-          continue;
-
-        psl_domain_match_metric = PSL_DOMAIN_MATCH_FOUND;
-        // This is not a perfect match, so we need to create a new valid result.
-        // We do this by copying over origin, signon realm and action from the
-        // observed form and setting the original signon realm to what we found
-        // in the database. We use the fact that |original_signon_realm| is
-        // non-empty to communicate that this match was found using public
-        // suffix matching.
-        new_form->original_signon_realm = new_form->signon_realm;
-        new_form->origin = form.origin;
-        new_form->signon_realm = form.signon_realm;
-        new_form->action = form.action;
-      }
-    }
-    forms->push_back(new_form.release());
-  }
-  UMA_HISTOGRAM_ENUMERATION("PasswordManager.PslDomainMatchTriggering",
-                            psl_domain_match_metric,
-                            PSL_DOMAIN_MATCH_COUNT);
-  return s.Succeeded();
+  return StatementToForms(&s, should_PSL_matching_apply ? &form : nullptr,
+                          forms);
 }
 
 bool LoginDatabase::GetLoginsCreatedBetween(
@@ -815,6 +782,7 @@ bool LoginDatabase::GetLoginsCreatedBetween(
     const base::Time end,
     ScopedVector<autofill::PasswordForm>* forms) const {
   DCHECK(forms);
+  forms->clear();
   sql::Statement s(db_.GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT origin_url, action_url, "
@@ -830,17 +798,7 @@ bool LoginDatabase::GetLoginsCreatedBetween(
   s.BindInt64(1, end.is_null() ? std::numeric_limits<int64>::max()
                                : end.ToInternalValue());
 
-  while (s.Step()) {
-    scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
-    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
-      return false;
-    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
-      continue;
-    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
-    forms->push_back(new_form.release());
-  }
-  return s.Succeeded();
+  return StatementToForms(&s, nullptr, forms);
 }
 
 bool LoginDatabase::GetLoginsSyncedBetween(
@@ -848,6 +806,7 @@ bool LoginDatabase::GetLoginsSyncedBetween(
     const base::Time end,
     ScopedVector<autofill::PasswordForm>* forms) const {
   DCHECK(forms);
+  forms->clear();
   sql::Statement s(db_.GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT origin_url, action_url, username_element, username_value, "
@@ -863,17 +822,7 @@ bool LoginDatabase::GetLoginsSyncedBetween(
               end.is_null() ? base::Time::Max().ToInternalValue()
                             : end.ToInternalValue());
 
-  while (s.Step()) {
-    scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
-    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
-      return false;
-    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
-      continue;
-    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
-    forms->push_back(new_form.release());
-  }
-  return s.Succeeded();
+  return StatementToForms(&s, nullptr, forms);
 }
 
 bool LoginDatabase::GetAutofillableLogins(
@@ -890,6 +839,7 @@ bool LoginDatabase::GetAllLoginsWithBlacklistSetting(
     bool blacklisted,
     ScopedVector<autofill::PasswordForm>* forms) const {
   DCHECK(forms);
+  forms->clear();
   // You *must* change LoginTableColumns if this query changes.
   sql::Statement s(db_.GetCachedStatement(
       SQL_FROM_HERE,
@@ -903,17 +853,7 @@ bool LoginDatabase::GetAllLoginsWithBlacklistSetting(
       "WHERE blacklisted_by_user == ? ORDER BY origin_url"));
   s.BindInt(0, blacklisted ? 1 : 0);
 
-  while (s.Step()) {
-    scoped_ptr<PasswordForm> new_form(new PasswordForm());
-    EncryptionResult result = InitPasswordFormFromStatement(new_form.get(), s);
-    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
-      return false;
-    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
-      continue;
-    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
-    forms->push_back(new_form.release());
-  }
-  return s.Succeeded();
+  return StatementToForms(&s, nullptr, forms);
 }
 
 bool LoginDatabase::DeleteAndRecreateDatabaseFile() {
@@ -924,4 +864,59 @@ bool LoginDatabase::DeleteAndRecreateDatabaseFile() {
   return Init();
 }
 
+// static
+bool LoginDatabase::StatementToForms(
+    sql::Statement* statement,
+    const autofill::PasswordForm* psl_match,
+    ScopedVector<autofill::PasswordForm>* forms) {
+  PSLDomainMatchMetric psl_domain_match_metric = PSL_DOMAIN_MATCH_NONE;
+
+  // Swap |collected_forms| into |*forms| first on success, to avoid returning
+  // partial results on error.
+  ScopedVector<autofill::PasswordForm> collected_forms;
+
+  while (statement->Step()) {
+    scoped_ptr<PasswordForm> new_form(new PasswordForm());
+    EncryptionResult result =
+        InitPasswordFormFromStatement(new_form.get(), *statement);
+    if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
+      return false;
+    if (result == ENCRYPTION_RESULT_ITEM_FAILURE)
+      continue;
+    DCHECK(result == ENCRYPTION_RESULT_SUCCESS);
+    if (psl_match && psl_match->signon_realm != new_form->signon_realm) {
+      if (new_form->scheme != PasswordForm::SCHEME_HTML)
+        continue;  // Ignore non-HTML matches.
+
+      if (!IsPublicSuffixDomainMatch(new_form->signon_realm,
+                                     psl_match->signon_realm)) {
+        continue;
+      }
+
+      psl_domain_match_metric = PSL_DOMAIN_MATCH_FOUND;
+      // This is not a perfect match, so we need to create a new valid result.
+      // We do this by copying over origin, signon realm and action from the
+      // observed form and setting the original signon realm to what we found
+      // in the database. We use the fact that |original_signon_realm| is
+      // non-empty to communicate that this match was found using public
+      // suffix matching.
+      new_form->original_signon_realm = new_form->signon_realm;
+      new_form->origin = psl_match->origin;
+      new_form->signon_realm = psl_match->signon_realm;
+      new_form->action = psl_match->action;
+    }
+    collected_forms.push_back(new_form.Pass());
+  }
+
+  if (psl_match) {
+    UMA_HISTOGRAM_ENUMERATION("PasswordManager.PslDomainMatchTriggering",
+                              psl_domain_match_metric, PSL_DOMAIN_MATCH_COUNT);
+  }
+
+  if (!statement->Succeeded())
+    return false;
+  forms->swap(collected_forms);
+  return true;
+}
+
 }  // namespace password_manager