Refactor CredentialManagerImpl::Get. It should use PasswordStore::GetLogins().

components/password_manager/core/browser/credential_manager_pending_request_task.h

Index: components/password_manager/core/browser/credential_manager_pending_request_task.h
diff --git a/components/password_manager/core/browser/credential_manager_pending_request_task.h b/components/password_manager/core/browser/credential_manager_pending_request_task.h
index 00ad0bcc72ef2b047ee267fcc5de0295e3f7751f..3c96d4d59383a2214eb90bc744b5e2fc59754d1a 100644
--- a/components/password_manager/core/browser/credential_manager_pending_request_task.h
+++ b/components/password_manager/core/browser/credential_manager_pending_request_task.h
@@ -5,7 +5,6 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_REQUEST_TASK_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_MANAGER_PENDING_REQUEST_TASK_H_
 
-#include <set>
 #include <string>
 #include <vector>
 
@@ -37,9 +36,6 @@ class CredentialManagerPendingRequestTaskDelegate {
   // Retrieves the current page origin.
   virtual GURL GetOrigin() const = 0;
 
-  // Retrieves a synthetic PasswordForm for the current page origin.
-  virtual PasswordStore::FormDigest GetSynthesizedFormForOrigin() const = 0;
-
   // Returns the PasswordManagerClient.
   virtual PasswordManagerClient* client() const = 0;
 
@@ -60,10 +56,8 @@ class CredentialManagerPendingRequestTask : public PasswordStoreConsumer {
       CredentialManagerPendingRequestTaskDelegate* delegate,
       const SendCredentialCallback& callback,
       bool request_zero_click_only,
-      const GURL& request_origin,
       bool include_passwords,
-      const std::vector<GURL>& request_federations,
-      const std::vector<std::string>& affiliated_realms);
+      const std::vector<GURL>& request_federations);
   ~CredentialManagerPendingRequestTask() override;
 
   SendCredentialCallback send_callback() const { return send_callback_; }
@@ -79,8 +73,7 @@ class CredentialManagerPendingRequestTask : public PasswordStoreConsumer {
   const bool zero_click_only_;
   const GURL origin_;
   const bool include_passwords_;
-  std::set<std::string> federations_;
-  std::set<std::string> affiliated_realms_;
+  std::vector<std::string> federations_;

[vabr (Chromium), 2016/11/23 08:57:56]

I'm not sure I understand why you made this a vector instead of a set, when it
is used for lookups (forcing you to use sort and binary_search explicitly).

We should also comment here that the vector is kept sorted if we want to keep it
that way.

[vasilii, 2016/11/23 09:40:54]

sorted vector is faster than the set. An alternative would be unordered_set. For
0-2 values it also will be slower.

[vabr (Chromium), 2016/11/23 09:51:03]

Thanks for explaining!

So this is the tradeoff for using vector:
Benefit -- better performance
Drawback -- increased fragility and verbosity of the code; if future changes
miss the requirement of keeping the vector sorted, the code will have bugs

Do we know how much performance gain we get from using vector? To me it seems
neither like a hotspot, nor like we expect large enough sizes to make a
difference here.

I'm happy to stand corrected on that observation, but until I do, I'd suggest to
have an unordered or just plain set to the vector. 

[vasilii, 2016/11/23 10:15:13]

Reverted.

 
   DISALLOW_COPY_AND_ASSIGN(CredentialManagerPendingRequestTask);
 };