PasswordStore: Clean up expectations about rewriting vectors of forms

chrome/browser/password_manager/native_backend_gnome_x.cc

Index: chrome/browser/password_manager/native_backend_gnome_x.cc
diff --git a/chrome/browser/password_manager/native_backend_gnome_x.cc b/chrome/browser/password_manager/native_backend_gnome_x.cc
index bf98cd1e78130f2d7276b18f1b5fffecacf0eafd..4ea6bcc07c3d133348e036f3c7659f81370215d2 100644
--- a/chrome/browser/password_manager/native_backend_gnome_x.cc
+++ b/chrome/browser/password_manager/native_backend_gnome_x.cc
@@ -159,17 +159,16 @@ scoped_ptr<PasswordForm> FormFromAttributes(GnomeKeyringAttributeList* attrs) {
   return form.Pass();
 }
 
-// Parse all the results from the given GList into a
-// ScopedVector<autofill::PasswordForm>, and free the GList. PasswordForms are
-// allocated on the heap, and should be deleted by the consumer. If not NULL,
-// |lookup_form| is used to filter out results -- only credentials with signon
-// realms passing the PSL matching against |lookup_form->signon_realm| will be
-// kept. PSL matched results get their signon_realm, origin, and action
-// rewritten to those of |lookup_form_|, with the original signon_realm saved
-// into the result's original_signon_realm data member.
-void ConvertFormList(GList* found,
-                     const PasswordForm* lookup_form,
-                     ScopedVector<autofill::PasswordForm>* forms) {
+// Returns all the results from |found|. If not NULL, |lookup_form| is used to
+// filter out results -- only credentials with signon realms passing the PSL
+// matching against |lookup_form->signon_realm| will be kept. PSL matched
+// results get their signon_realm, origin, and action rewritten to those of
+// |lookup_form_|, with the original signon_realm saved into the result's
+// original_signon_realm data member.
+ScopedVector<autofill::PasswordForm> ConvertFormList(
+    GList* found,
+    const PasswordForm* lookup_form) {
+  ScopedVector<autofill::PasswordForm> forms;
   password_manager::PSLDomainMatchMetric psl_domain_match_metric =
       password_manager::PSL_DOMAIN_MATCH_NONE;
   for (GList* element = g_list_first(found); element != NULL;
@@ -198,7 +197,7 @@ void ConvertFormList(GList* found,
       } else {
         LOG(WARNING) << "Unable to access password from list element!";
       }
-      forms->push_back(form.release());
+      forms.push_back(form.release());
     } else {
       LOG(WARNING) << "Could not initialize PasswordForm from attributes!";
     }
@@ -214,6 +213,7 @@ void ConvertFormList(GList* found,
             : password_manager::PSL_DOMAIN_MATCH_NOT_USED,
         password_manager::PSL_DOMAIN_MATCH_COUNT);
   }
+  return forms.Pass();
 }
 
 // Schema is analagous to the fields in PasswordForm.
@@ -283,7 +283,7 @@ class GKRMethod : public GnomeKeyringLoader {
   GnomeKeyringResult WaitResult();
 
   // Use after AddLoginSearch, UpdateLoginSearch, GetLogins, GetLoginsList,
-  // GetAllLogins.
+  // GetAllLogins. Replaces the content of |forms| with found logins.
   GnomeKeyringResult WaitResult(ScopedVector<autofill::PasswordForm>* forms);
 
  private:
@@ -311,6 +311,9 @@ class GKRMethod : public GnomeKeyringLoader {
   // All these callbacks are called on UI thread.
   static void OnOperationDone(GnomeKeyringResult result, gpointer data);
 
+  // This is marked as static, but acts on the GKRMethod instance pointed to by
+  // |data|. Saves the result to |result_|. If the result is OK, overwrites
+  // |forms_| with the found credentials. Clears |forms_| otherwise.
   static void OnOperationGetList(GnomeKeyringResult result, GList* list,
                                  gpointer data);
 
@@ -480,14 +483,7 @@ GnomeKeyringResult GKRMethod::WaitResult(
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   event_.Wait();
-  if (forms->empty()) {

[vabr (Chromium), 2015/02/11 17:19:14]

https://codereview.chromium.org/7712024 introduced the requirement that
WaitResult appends (and some of the now failing tests).

I don't think there are any valid reasons to do this any more, but will
double-check, and am highlighting this in any case.

-    // Normal case. Avoid extra allocation by swapping.
-    forms->swap(forms_);
-  } else {
-    // Rare case. Append forms_ to *forms.
-    forms->insert(forms->end(), forms_.begin(), forms_.end());
-    forms_.weak_clear();
-  }
+  forms->swap(forms_);
   return result_;
 }
 
@@ -526,7 +522,8 @@ void GKRMethod::OnOperationGetList(GnomeKeyringResult result, GList* list,
   method->result_ = result;
   method->forms_.clear();
   // |list| will be freed after this callback returns, so convert it now.
-  ConvertFormList(list, method->lookup_form_.get(), &method->forms_);
+  if (result == GNOME_KEYRING_RESULT_OK)
+    method->forms_ = ConvertFormList(list, method->lookup_form_.get());
   method->lookup_form_.reset(NULL);
   method->event_.Signal();
 }
@@ -712,6 +709,7 @@ bool NativeBackendGnome::GetLoginsList(
     bool autofillable,
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  forms->clear();
 
   uint32_t blacklisted_by_user = !autofillable;