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 1d30df71f3a5f3e014d5605c02ee832fb3e5c781..6c30b5824890d023aa02706ebfd91733793bf882 100644
--- a/chrome/browser/password_manager/native_backend_gnome_x.cc
+++ b/chrome/browser/password_manager/native_backend_gnome_x.cc
@@ -162,17 +162,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

[engedy, 2015/02/25 15:17:48]

Phrasing suggestion: Converts native credentials in |found| to PasswordForms...

Nit: We should either mention that this destroys/consumes |found|, or make that
a scoped_ptr.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.
Are you sure this destroys |found|?
To destroy it, gnome_keyring_found_list_free would need to be called, but it
currently is not.
Since we do not observe a leak either, I assume gnome_keyring_find_items keeps
owning |found| when it passes the pointer to it to OnOperationGetList (which, in
turn, calls ConvertFromList).
Alas, the documentation
https://developer.gnome.org/gnome-keyring/stable/gnome-keyring-Search-Functio...
does not say whether that's the case.

[engedy, 2015/03/09 13:33:17]

I am not sure, but the original comment mentioned that the function destroys the
lists. All I wanted is pointing out that we should preserve that comment.

+// 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;
@@ -201,7 +200,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!";
     }
@@ -217,6 +216,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.
@@ -287,7 +287,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:
@@ -315,6 +315,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

[engedy, 2015/02/25 15:17:47]

Phrasing suggestion: s/GKRMethod instance pointed to by |data|/GKRMethod
instance that |data| points to/.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

+  // |data|. Saves the result to |result_|. If the result is OK, overwrites

[engedy, 2015/02/25 15:17:47]

Nit: |result|.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

+  // |forms_| with the found credentials. Clears |forms_| otherwise.
   static void OnOperationGetList(GnomeKeyringResult result, GList* list,
                                  gpointer data);
 
@@ -485,14 +488,7 @@ GnomeKeyringResult GKRMethod::WaitResult(
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   event_.Wait();
-  if (forms->empty()) {

[vabr (Chromium), 2015/02/24 18:06:21]

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, and am
highlighting this in any case.

I also removed tests designed just to check the appending, as a result of
removing the appending.

-    // 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_;
 }
 
@@ -531,7 +527,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();
   method->event_.Signal();
 }
@@ -717,6 +714,7 @@ bool NativeBackendGnome::GetLoginsList(
     bool autofillable,
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  forms->clear();

[engedy, 2015/02/25 15:17:48]

I think this is not needed, WaitResult will always return what
OnOperationGetList leaves in |forms_|, and that will be cleared on error.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

 
   uint32_t blacklisted_by_user = !autofillable;