Credential Manager: Switch 'request()' to GetAutofillableLogins().

components/password_manager/content/browser/content_credential_manager_dispatcher.cc

Index: components/password_manager/content/browser/content_credential_manager_dispatcher.cc
diff --git a/components/password_manager/content/browser/content_credential_manager_dispatcher.cc b/components/password_manager/content/browser/content_credential_manager_dispatcher.cc
index 186a1e9af45f95d26599536a43c5fbf96870accc..6b4006d913d3ceb2d1f0c5b29ca843d7256ed877 100644
--- a/components/password_manager/content/browser/content_credential_manager_dispatcher.cc
+++ b/components/password_manager/content/browser/content_credential_manager_dispatcher.cc
@@ -21,12 +21,26 @@
 
 namespace password_manager {
 
+struct ContentCredentialManagerDispatcher::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) {}
+
+  int id;
+  bool zero_click_only;
+  GURL origin;
+  std::vector<GURL> federations;
+};
+
 ContentCredentialManagerDispatcher::ContentCredentialManagerDispatcher(
     content::WebContents* web_contents,
     PasswordManagerClient* client)
-    : WebContentsObserver(web_contents),
-      client_(client),
-      pending_request_id_(0) {
+    : WebContentsObserver(web_contents), client_(client) {
   DCHECK(web_contents);
 }
 
@@ -93,43 +107,47 @@ void ContentCredentialManagerDispatcher::OnNotifySignedOut(int request_id) {
 
 void ContentCredentialManagerDispatcher::OnRequestCredential(
     int request_id,
-    bool /* zero_click_only */,
-    const std::vector<GURL>& /* federations */) {
+    bool zero_click_only,
+    const std::vector<GURL>& federations) {
   DCHECK(request_id);
   PasswordStore* store = GetPasswordStore();
-  if (pending_request_id_ || !store) {
+  if (pending_request_ || !store) {
     web_contents()->GetRenderViewHost()->Send(
         new CredentialManagerMsg_RejectCredentialRequest(
-            web_contents()->GetRenderViewHost()->GetRoutingID(),
-            request_id,
-            pending_request_id_
+            web_contents()->GetRenderViewHost()->GetRoutingID(), request_id,
+            pending_request_
                 ? blink::WebCredentialManagerError::ErrorTypePendingRequest
                 : blink::WebCredentialManagerError::
                       ErrorTypePasswordStoreUnavailable));
     return;
   }
 
-  pending_request_id_ = request_id;
-
-  // TODO(mkwst): we should deal with federated login types.
-  autofill::PasswordForm form;
-  form.scheme = autofill::PasswordForm::SCHEME_HTML;
-  form.origin = web_contents()->GetLastCommittedURL().GetOrigin();
-  form.signon_realm = form.origin.spec();
+  pending_request_.reset(new PendingRequestParameters(
+      request_id, zero_click_only,
+      web_contents()->GetLastCommittedURL().GetOrigin(), federations));
 
-  store->GetLogins(form, PasswordStore::DISALLOW_PROMPT, this);
+  store->GetAutofillableLogins(this);
 }
 
 void ContentCredentialManagerDispatcher::OnGetPasswordStoreResults(
     const std::vector<autofill::PasswordForm*>& results) {
-  DCHECK(pending_request_id_);
-
-  if (results.empty() ||
-      !client_->PromptUserToChooseCredentials(results, base::Bind(
-          &ContentCredentialManagerDispatcher::SendCredential,
-          base::Unretained(this),
-          pending_request_id_))) {
-    SendCredential(pending_request_id_, CredentialInfo());
+  DCHECK(pending_request_);
+
+  std::vector<autofill::PasswordForm*> filtered_results;
+  for (autofill::PasswordForm* form : results) {
+    // TODO(mkwst): Extend this filter to include federations.
+    if (form->origin == pending_request_->origin)
+      filtered_results.push_back(form);
+  }
+
+  if (filtered_results.empty() ||

[vabr (Chromium), 2014/12/15 10:47:22]

An optional "BTW" nit:
It is important that in this if-condition, PromptUserToChooseCredentials remains
always the last one (otherwise we might invoke the prompt for bad origins etc.).
If I were rewriting the code, I would probably separate the first two checks
into their own if-block with an early return, to make that clear. But that's
both just an optional subjective suggestion, and it does not be addressed in
this CL.

[Mike West, 2014/12/15 12:59:24]

I'll take a look at that in the next CL when I poke at the filters again.

+      web_contents()->GetLastCommittedURL().GetOrigin() !=
+          pending_request_->origin ||
+      !client_->PromptUserToChooseCredentials(
+          filtered_results,
+          base::Bind(&ContentCredentialManagerDispatcher::SendCredential,
+                     base::Unretained(this), pending_request_->id))) {
+    SendCredential(pending_request_->id, CredentialInfo());
   }
 }
 
@@ -149,14 +167,13 @@ ContentCredentialManagerDispatcher::GetDriver() {
 
 void ContentCredentialManagerDispatcher::SendCredential(
     int request_id, const CredentialInfo& info) {
-  DCHECK(pending_request_id_);
-  DCHECK_EQ(pending_request_id_, request_id);
+  DCHECK(pending_request_);
+  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_ = 0;
+          pending_request_->id, info));
+  pending_request_.reset();
 }
 
 }  // namespace password_manager