GetLoginsRequest: Use ScopedVector to express ownership of forms

components/password_manager/content/browser/credential_manager_dispatcher.cc

 // Copyright 2014 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 "components/password_manager/content/browser/credential_manager_dispatcher.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_vector.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 10 matching lines @@
 #include "content/public/browser/web_contents.h"
 #include "ipc/ipc_message_macros.h"
 
 namespace password_manager {
 
 // CredentialManagerDispatcher::PendingRequestTask -----------------------------
 
 class CredentialManagerDispatcher::PendingRequestTask
     : public PasswordStoreConsumer {
  public:
+  // TODO(mkwst): De-inline the methods in this class. http://goo.gl/RmFwKd
   PendingRequestTask(CredentialManagerDispatcher* const dispatcher,
                      int request_id,
                      bool request_zero_click_only,
                      const GURL& request_origin,
                      const std::vector<GURL>& request_federations)
       : dispatcher_(dispatcher),
         id_(request_id),
         zero_click_only_(request_zero_click_only),
         origin_(request_origin) {
     for (const GURL& origin : request_federations)
       federations_.insert(origin.spec());
   }
 
   int id() const { return id_; }
 
   // PasswordStoreConsumer implementation.
   void OnGetPasswordStoreResults(
-      const std::vector<autofill::PasswordForm*>& results) override {
-    // We own the PasswordForm instances, so we're responsible for cleaning
-    // up the instances we don't add to |local_results| or |federated_results|.
-    //
-    // TODO(mkwst): Switch this and PromptUserToChooseCredentials() to use
-    // ScopedVector.
-    std::vector<autofill::PasswordForm*> local_results;
-    std::vector<autofill::PasswordForm*> federated_results;
-    autofill::PasswordForm* zero_click_form_to_return = nullptr;
-    bool found_zero_clickable_credential = false;
-    for (autofill::PasswordForm* form : results) {
-      if (form->origin == origin_) {
-        local_results.push_back(form);
-
-        // If this is a zero-clickable PasswordForm, and we haven't found any
-        // other zero-clickable PasswordForms, then store this one for later.
-        // If we have found other zero-clickable PasswordForms, then clear
-        // the stored form (we return zero-click forms iff there is a single,
-        // unambigious choice).
-        if (!form->skip_zero_click) {
-          zero_click_form_to_return =
-              found_zero_clickable_credential ? nullptr : form;
-          found_zero_clickable_credential = true;
-        }
-      } else if (federations_.count(form->origin.spec()))
-        federated_results.push_back(form);
-      else
-        delete form;
-    }
-
-    if ((local_results.empty() && federated_results.empty()) ||
-        dispatcher_->web_contents()->GetLastCommittedURL().GetOrigin() !=
-            origin_) {
-      dispatcher_->SendCredential(id_, CredentialInfo());
-      return;
-    }
-    if (zero_click_form_to_return && dispatcher_->IsZeroClickAllowed()) {
-      CredentialInfo info(*zero_click_form_to_return,
-                          zero_click_form_to_return->federation_url.is_empty()
-                              ? CredentialType::CREDENTIAL_TYPE_LOCAL
-                              : CredentialType::CREDENTIAL_TYPE_FEDERATED);
-      STLDeleteElements(&local_results);
-      STLDeleteElements(&federated_results);
-      dispatcher_->SendCredential(id_, info);
-      return;
-    }
-
-    if (zero_click_only_ ||
-        !dispatcher_->client()->PromptUserToChooseCredentials(
-            local_results, federated_results,
-            base::Bind(&CredentialManagerDispatcher::SendCredential,
-                       base::Unretained(dispatcher_), id_))) {
-      STLDeleteElements(&local_results);
-      STLDeleteElements(&federated_results);
-      dispatcher_->SendCredential(id_, CredentialInfo());
-    }
-  }
+      ScopedVector<autofill::PasswordForm> results) override;
 
  private:
   // Backlink to the CredentialManagerDispatcher that owns this object.
   CredentialManagerDispatcher* const dispatcher_;
 
   const int id_;
   const bool zero_click_only_;
   const GURL origin_;
   std::set<std::string> federations_;
 
   DISALLOW_COPY_AND_ASSIGN(PendingRequestTask);
 };
 
+void CredentialManagerDispatcher::PendingRequestTask::OnGetPasswordStoreResults(
+    ScopedVector<autofill::PasswordForm> results) {
+  ScopedVector<autofill::PasswordForm> local_results;
+  ScopedVector<autofill::PasswordForm> federated_results;
+  autofill::PasswordForm* zero_click_form_to_return = nullptr;
+  bool found_zero_clickable_credential = false;
+  for (auto& form : results) {
+    if (form->origin == origin_) {
+      local_results.push_back(form);
+
+      // If this is a zero-clickable PasswordForm, and we haven't found any
+      // other zero-clickable PasswordForms, then store this one for later.
+      // If we have found other zero-clickable PasswordForms, then clear
+      // the stored form (we return zero-click forms iff there is a single,
+      // unambigious choice).
+      if (!form->skip_zero_click) {
+        zero_click_form_to_return =
+            found_zero_clickable_credential ? nullptr : form;
+        found_zero_clickable_credential = true;
+      }
+      form = nullptr;
+    } else if (federations_.count(form->origin.spec())) {
+      federated_results.push_back(form);
+      form = nullptr;
+    }
+  }
+
+  if ((local_results.empty() && federated_results.empty()) ||
+      dispatcher_->web_contents()->GetLastCommittedURL().GetOrigin() !=
+          origin_) {
+    dispatcher_->SendCredential(id_, CredentialInfo());
+    return;
+  }
+  if (zero_click_form_to_return && dispatcher_->IsZeroClickAllowed()) {
+    CredentialInfo info(*zero_click_form_to_return,
+                        zero_click_form_to_return->federation_url.is_empty()
+                            ? CredentialType::CREDENTIAL_TYPE_LOCAL
+                            : CredentialType::CREDENTIAL_TYPE_FEDERATED);
+    dispatcher_->SendCredential(id_, info);
+    return;
+  }
+
+  if (zero_click_only_ ||
+      !dispatcher_->client()->PromptUserToChooseCredentials(
+          local_results.Pass(), federated_results.Pass(),
+          base::Bind(&CredentialManagerDispatcher::SendCredential,
+                     base::Unretained(dispatcher_), id_))) {
+    dispatcher_->SendCredential(id_, CredentialInfo());
+  }
+}
+
 // CredentialManagerDispatcher::PendingSignedOutTask ---------------------------
 
 class CredentialManagerDispatcher::PendingSignedOutTask
     : public PasswordStoreConsumer {
  public:
   PendingSignedOutTask(CredentialManagerDispatcher* const dispatcher,
                        const GURL& origin);
 
   void AddOrigin(const GURL& origin);
 
   // PasswordStoreConsumer implementation.
   void OnGetPasswordStoreResults(
-      const std::vector<autofill::PasswordForm*>& results) override;
+      ScopedVector<autofill::PasswordForm> results) override;
 
  private:
   // Backlink to the CredentialManagerDispatcher that owns this object.
   CredentialManagerDispatcher* const dispatcher_;
   std::set<std::string> origins_;
 
   DISALLOW_COPY_AND_ASSIGN(PendingSignedOutTask);
 };
 
 CredentialManagerDispatcher::PendingSignedOutTask::PendingSignedOutTask(
     CredentialManagerDispatcher* const dispatcher,
     const GURL& origin)
     : dispatcher_(dispatcher) {
   origins_.insert(origin.spec());
 }
 
 void CredentialManagerDispatcher::PendingSignedOutTask::AddOrigin(
     const GURL& origin) {
   origins_.insert(origin.spec());
 }
 
 void CredentialManagerDispatcher::PendingSignedOutTask::
-    OnGetPasswordStoreResults(
-        const std::vector<autofill::PasswordForm*>& results) {
+    OnGetPasswordStoreResults(ScopedVector<autofill::PasswordForm> results) {
   PasswordStore* store = dispatcher_->GetPasswordStore();
   for (autofill::PasswordForm* form : results) {
     if (origins_.count(form->origin.spec())) {
       form->skip_zero_click = true;
+      // Note that UpdateLogin ends up copying the form while posting a task to
+      // update the PasswordStore, so it's fine to let |results| delete the
+      // original at the end of this method.
       store->UpdateLogin(*form);
     }
-    // We own the PasswordForms, so we need to dispose of them. This is safe to
-    // do, as ::UpdateLogin ends up copying the form while posting a task to
-    // update the PasswordStore.
-    delete form;
   }
 
   dispatcher_->DoneSigningOut();
 }
 
 // CredentialManagerDispatcher -------------------------------------------------
 
 CredentialManagerDispatcher::CredentialManagerDispatcher(
     content::WebContents* web_contents,
     PasswordManagerClient* client)
@@ skipping 151 matching lines @@
           pending_request_->id(), info));
   pending_request_.reset();
 }
 
 void CredentialManagerDispatcher::DoneSigningOut() {
   DCHECK(pending_sign_out_);
   pending_sign_out_.reset();
 }
 
 }  // namespace password_manager