Credential Manager: Introduce PendingRequestTask.

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"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
 #include "components/password_manager/content/browser/credential_manager_password_form_manager.h"
 #include "components/password_manager/content/common/credential_manager_messages.h"
 #include "components/password_manager/content/common/credential_manager_types.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "ipc/ipc_message_macros.h"
 
 namespace password_manager {
 
-struct CredentialManagerDispatcher::PendingRequestParameters {
-  PendingRequestParameters(int request_id,
-                           bool request_zero_click_only,
-                           GURL request_origin,
-                           const std::vector<GURL>& request_federations)
-      : id(request_id),
-        zero_click_only(request_zero_click_only),
-        origin(request_origin),
-        federations(request_federations) {}
+class CredentialManagerDispatcher::PendingRequestTask
+    : public PasswordStoreConsumer {
+ public:
+  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)

[vabr (Chromium), 2015/01/30 12:54:19]

If this is possible in the IPC framework we have, I suggest the following
changes:
1. Changing the type of federations in
CredentialManagerDispatcher::OnRequestCredential and request_federations here to
something non-const.
2. Making federations_ a vector of GURL (given that we do not expect duplicated
URLs, and creating the set is actually O(N logN), the linear search in the
vector as opposed to the log search in the map should not make the performance
worse).
3. Swapping the request_federations with federations_ instead of copying the
contents.

If the above is not possible, let's live with the copy you have here.

+      federations_.insert(origin.spec());
+  }
 
-  int id;
-  bool zero_click_only;
-  GURL origin;
-  std::vector<GURL> federations;
+  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|.
+    std::vector<autofill::PasswordForm*> local_results;

[vabr (Chromium), 2015/01/30 12:54:19]

Consider using ScopedVector for both these vectors. Then you can drop the
STLDeleteElements, and use .get() if you still need the vector with the raw
pointers.

+    std::vector<autofill::PasswordForm*> federated_results;
+    for (autofill::PasswordForm* form : results) {
+      if (form->origin == origin_)
+        local_results.push_back(form);
+      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 (local_results.size() == 1 && dispatcher_->IsZeroClickAllowed()) {
+      // TODO(mkwst): Use the `one_time_disable_zero_click` flag on the result
+      // to prevent auto-sign-in, once that flag is implemented.
+      CredentialInfo info(*local_results[0],
+                          local_results[0]->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());
+    }
+  }
+
+ 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);
 };
 
 CredentialManagerDispatcher::CredentialManagerDispatcher(
     content::WebContents* web_contents,
     PasswordManagerClient* client)
     : WebContentsObserver(web_contents), client_(client) {
   DCHECK(web_contents);
 }
 
 CredentialManagerDispatcher::~CredentialManagerDispatcher() {
@@ skipping 77 matching lines @@
   }
 
   if (zero_click_only && !IsZeroClickAllowed()) {
     web_contents()->GetRenderViewHost()->Send(
         new CredentialManagerMsg_SendCredential(
             web_contents()->GetRenderViewHost()->GetRoutingID(), request_id,
             CredentialInfo()));
     return;
   }
 
-  pending_request_.reset(new PendingRequestParameters(
-      request_id, zero_click_only,
+  pending_request_.reset(new PendingRequestTask(
+      this, request_id, zero_click_only,
       web_contents()->GetLastCommittedURL().GetOrigin(), federations));
 
-  // This will result in a callback to ::OnGetPasswordStoreResults().
-  store->GetAutofillableLogins(this);
-}
-
-void CredentialManagerDispatcher::OnGetPasswordStoreResults(
-    const std::vector<autofill::PasswordForm*>& results) {
-  DCHECK(pending_request_);
-
-  std::set<std::string> federations;
-  for (const GURL& origin : pending_request_->federations)
-    federations.insert(origin.spec());
-
-  // We own the PasswordForm instances, so we're responsible for cleaning
-  // up the instances we don't add to |local_results| or |federated_results|.
-  std::vector<autofill::PasswordForm*> local_results;
-  std::vector<autofill::PasswordForm*> federated_results;
-  for (autofill::PasswordForm* form : results) {
-    if (form->origin == pending_request_->origin)
-      local_results.push_back(form);
-    else if (federations.count(form->origin.spec()) != 0)
-      federated_results.push_back(form);
-    else
-      delete form;
-  }
-
-  if ((local_results.empty() && federated_results.empty()) ||
-      web_contents()->GetLastCommittedURL().GetOrigin() !=
-          pending_request_->origin) {
-    SendCredential(pending_request_->id, CredentialInfo());
-    return;
-  }
-
-  if (local_results.size() == 1 && IsZeroClickAllowed()) {
-    // TODO(mkwst): Use the `one_time_disable_zero_click` flag on the result
-    // to prevent auto-sign-in, once that flag is implemented.
-    CredentialInfo info(*local_results[0],
-                        local_results[0]->federation_url.is_empty()
-                            ? CredentialType::CREDENTIAL_TYPE_LOCAL
-                            : CredentialType::CREDENTIAL_TYPE_FEDERATED);
-    STLDeleteElements(&local_results);
-    STLDeleteElements(&federated_results);
-    SendCredential(pending_request_->id, info);
-    return;
-  }
-
-  if (pending_request_->zero_click_only ||
-      !client_->PromptUserToChooseCredentials(
-          local_results, federated_results,
-          base::Bind(&CredentialManagerDispatcher::SendCredential,
-                     base::Unretained(this), pending_request_->id))) {
-    STLDeleteElements(&local_results);
-    STLDeleteElements(&federated_results);
-    SendCredential(pending_request_->id, CredentialInfo());
-  }
+  // This will result in a callback to
+  // PendingRequestTask::OnGetPasswordStoreResults().
+  store->GetAutofillableLogins(pending_request_.get());
 }
 
 PasswordStore* CredentialManagerDispatcher::GetPasswordStore() {
   return client_ ? client_->GetPasswordStore() : nullptr;
 }
 
 bool CredentialManagerDispatcher::IsSavingEnabledForCurrentPage() const {
   // TODO(vasilii): add more, see http://crbug.com/450583.
   return !client_->IsOffTheRecord();
 }
 
 bool CredentialManagerDispatcher::IsZeroClickAllowed() const {
   return !client_->IsOffTheRecord() && client_->IsZeroClickEnabled();
 }
 
 base::WeakPtr<PasswordManagerDriver> CredentialManagerDispatcher::GetDriver() {
   ContentPasswordManagerDriverFactory* driver_factory =
       ContentPasswordManagerDriverFactory::FromWebContents(web_contents());
   DCHECK(driver_factory);
   PasswordManagerDriver* driver =
       driver_factory->GetDriverForFrame(web_contents()->GetMainFrame());
   return driver->AsWeakPtr();
 }
 
 void CredentialManagerDispatcher::SendCredential(int request_id,
                                                  const CredentialInfo& info) {
   DCHECK(pending_request_);
-  DCHECK_EQ(pending_request_->id, request_id);
+  DCHECK_EQ(pending_request_->id(), request_id);
   web_contents()->GetRenderViewHost()->Send(
       new CredentialManagerMsg_SendCredential(
           web_contents()->GetRenderViewHost()->GetRoutingID(),
-          pending_request_->id, info));
+          pending_request_->id(), info));
   pending_request_.reset();
 }
 
 }  // namespace password_manager