[Passwords] Minor CredentialCache Clean-Ups

This change performs minor clean ups to CredentialCache and the related
OriginCredentialStore.

Bug: 945300
Change-Id: Ic714c41d345aa1178acc838d01a6d16bc9a11a47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702416Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678226}

chrome/browser/password_manager/password_accessory_controller_impl.cc

@@ -241,7 +241,7 @@ void PasswordAccessoryControllerImpl::RefreshSuggestionsForField(
 
   if (autofill::IsFillable(focused_field_type)) {
     base::span<const CredentialPair> suggestions =
-        credential_cache_->GetCredentialStore(origin)->GetCredentials();
+        credential_cache_->GetCredentialStore(origin).GetCredentials();
     info_to_add.reserve(suggestions.size());
     for (const auto& pair : suggestions) {
       if (pair.is_public_suffix_match &&
@@ -372,15 +372,14 @@ bool PasswordAccessoryControllerImpl::AppearsInSuggestions(
     bool is_password,
     const url::Origin& origin) const {
   if (origin.opaque())
-    return false;  // Don't proceed for invalid origins.;
-  for (const CredentialPair& pair :
-       credential_cache_->GetCredentialStore(origin)->GetCredentials()) {
-    const base::string16& candidate =
-        is_password ? pair.password : pair.username;
-    if (candidate == suggestion)
-      return true;
-  }
-  return false;
+    return false;  // Don't proceed for invalid origins.
+
+  const auto& credentials =
+      credential_cache_->GetCredentialStore(origin).GetCredentials();
+  return std::any_of(
+      credentials.begin(), credentials.end(), [&](const auto& pair) {
+        return suggestion == (is_password ? pair.password : pair.username);
+      });
 }
 
 base::WeakPtr<ManualFillingController>

chrome/browser/password_manager/password_accessory_controller_impl.h

@@ -118,7 +118,6 @@ class PasswordAccessoryControllerImpl
   base::WeakPtr<ManualFillingController> GetManualFillingController();
 
   url::Origin GetFocusedFrameOrigin() const;
-  std::vector<password_manager::CredentialPair> GetSuggestions() const;
 
   // ------------------------------------------------------------------------
   // Members - Make sure to NEVER store state related to a single frame here!

components/password_manager/core/browser/credential_cache.cc

@@ -12,6 +12,7 @@
 
 #include "components/autofill/core/common/autofill_features.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/origin_credential_store.h"
 
 namespace password_manager {
 
@@ -22,27 +23,26 @@ void CredentialCache::SaveCredentialsForOrigin(
     const std::map<base::string16, const autofill::PasswordForm*>& best_matches,
     const url::Origin& origin) {
   std::vector<CredentialPair> credentials;
-  std::transform(best_matches.begin(), best_matches.end(),
-                 std::back_inserter(credentials), [](const auto& pair) {
-                   return CredentialPair(pair.second->username_value,
-                                         pair.second->password_value,
-                                         pair.second->origin,
-                                         pair.second->is_public_suffix_match);
-                 });
+  credentials.reserve(best_matches.size());
+  for (const auto& pair : best_matches) {
+    const auto& form = *pair.second;
+    credentials.emplace_back(form.username_value, form.password_value,
+                             form.origin, form.is_public_suffix_match);
+  }
   // Sort by origin (but keep the existing username order).
   std::stable_sort(credentials.begin(), credentials.end(),
                    [](const CredentialPair& lhs, const CredentialPair& rhs) {
                      return lhs.origin_url < rhs.origin_url;
                    });
   // Move credentials with exactly matching origins to the top.
-  std::stable_partition(credentials.begin(), credentials.end(),
-                        [o = origin.GetURL()](const CredentialPair& pair) {
-                          return pair.origin_url == o;
-                        });
-  GetOrCreateCredentialStore(origin)->SaveCredentials(std::move(credentials));
+  const GURL url = origin.GetURL();
+  std::stable_partition(
+      credentials.begin(), credentials.end(),
+      [&url](const CredentialPair& pair) { return pair.origin_url == url; });
+  GetOrCreateCredentialStore(origin).SaveCredentials(std::move(credentials));
 }
 
-const OriginCredentialStore* CredentialCache::GetCredentialStore(
+const OriginCredentialStore& CredentialCache::GetCredentialStore(
     const url::Origin& origin) {
   return GetOrCreateCredentialStore(origin);
 }
@@ -51,15 +51,9 @@ void CredentialCache::ClearCredentials() {
   origin_credentials_.clear();
 }
 
-OriginCredentialStore* CredentialCache::GetOrCreateCredentialStore(
+OriginCredentialStore& CredentialCache::GetOrCreateCredentialStore(
     const url::Origin& origin) {
-  auto it = origin_credentials_.find(origin);
-  if (it != origin_credentials_.end())
-    return it->second.get();
-  auto iter =
-      origin_credentials_
-          .insert({origin, std::make_unique<OriginCredentialStore>(origin)})
-          .first;
-  return iter->second.get();
+  return origin_credentials_.emplace(origin, origin).first->second;
 }
+
 }  // namespace password_manager

components/password_manager/core/browser/credential_cache.h

@@ -6,13 +6,8 @@
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIAL_CACHE_H_
 
 #include <map>
-#include <memory>
-#include <utility>
-#include <vector>
 
-#include "base/macros.h"
 #include "base/strings/string16.h"
-#include "components/password_manager/core/browser/origin_credential_store.h"
 #include "url/origin.h"
 
 namespace autofill {
@@ -21,13 +16,15 @@ struct PasswordForm;
 
 namespace password_manager {
 
+class OriginCredentialStore;
+
 // This class caches and provides credential stores for different origins.
 class CredentialCache {
  public:
   CredentialCache();
-  ~CredentialCache();
   CredentialCache(const CredentialCache&) = delete;
   CredentialCache& operator=(const CredentialCache&) = delete;
+  ~CredentialCache();
 
   // Saves credentials for an origin so that they can be used in the sheet.
   void SaveCredentialsForOrigin(
@@ -36,17 +33,16 @@ class CredentialCache {
 
   // Returns the credential store for a given origin. If it does not exist, an
   // empty store will be created.
-  const OriginCredentialStore* GetCredentialStore(const url::Origin& origin);
+  const OriginCredentialStore& GetCredentialStore(const url::Origin& origin);
 
   // Removes all credentials for all origins.
   void ClearCredentials();
 
  private:
-  OriginCredentialStore* GetOrCreateCredentialStore(const url::Origin& origin);
+  OriginCredentialStore& GetOrCreateCredentialStore(const url::Origin& origin);
 
   // Contains the store for credential of each requested origin.
-  std::map<url::Origin, std::unique_ptr<OriginCredentialStore>>
-      origin_credentials_;
+  std::map<url::Origin, OriginCredentialStore> origin_credentials_;
 };
 
 }  // namespace password_manager

components/password_manager/core/browser/credential_cache_unittest.cc

@@ -34,11 +34,11 @@ class CredentialCacheTest : public testing::Test {
 };
 
 TEST_F(CredentialCacheTest, ReturnsSameStoreForSameOriginOnly) {
-  EXPECT_EQ(cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite))),
-            cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite))));
+  EXPECT_EQ(&cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite))),
+            &cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite))));
 
-  EXPECT_NE(cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite))),
-            cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite2))));
+  EXPECT_NE(&cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite))),
+            &cache()->GetCredentialStore(Origin::Create(GURL(kExampleSite2))));
 }
 
 TEST_F(CredentialCacheTest, StoresCredentialsSortedByAplhabetAndOrigins) {
@@ -57,7 +57,7 @@ TEST_F(CredentialCacheTest, StoresCredentialsSortedByAplhabetAndOrigins) {
       origin);
 
   EXPECT_THAT(
-      cache()->GetCredentialStore(origin)->GetCredentials(),
+      cache()->GetCredentialStore(origin).GetCredentials(),
       testing::ElementsAre(
 
           // Alphabetical entries of the exactly matching https://example.com:
@@ -96,11 +96,11 @@ TEST_F(CredentialCacheTest, StoredCredentialsForIndependentOrigins) {
   cache()->SaveCredentialsForOrigin(
       {CreateEntry("Abe", "B4dPW", GURL(kExampleSite2), false).first}, origin2);
 
-  EXPECT_THAT(cache()->GetCredentialStore(origin)->GetCredentials(),
+  EXPECT_THAT(cache()->GetCredentialStore(origin).GetCredentials(),
               testing::ElementsAre(
                   CredentialPair(ASCIIToUTF16("Ben"), ASCIIToUTF16("S3cur3"),
                                  GURL(kExampleSite), /*is_psl_match*/ false)));
-  EXPECT_THAT(cache()->GetCredentialStore(origin2)->GetCredentials(),
+  EXPECT_THAT(cache()->GetCredentialStore(origin2).GetCredentials(),
               testing::ElementsAre(
                   CredentialPair(ASCIIToUTF16("Abe"), ASCIIToUTF16("B4dPW"),
                                  GURL(kExampleSite2), /*is_psl_match*/ false)));
@@ -111,13 +111,13 @@ TEST_F(CredentialCacheTest, ClearsCredentials) {
   cache()->SaveCredentialsForOrigin(
       {CreateEntry("Ben", "S3cur3", GURL(kExampleSite), false).first},
       Origin::Create(GURL(kExampleSite)));
-  ASSERT_THAT(cache()->GetCredentialStore(origin)->GetCredentials(),
+  ASSERT_THAT(cache()->GetCredentialStore(origin).GetCredentials(),
               testing::ElementsAre(
                   CredentialPair(ASCIIToUTF16("Ben"), ASCIIToUTF16("S3cur3"),
                                  GURL(kExampleSite), /*is_psl_match*/ false)));
 
   cache()->ClearCredentials();
-  EXPECT_EQ(cache()->GetCredentialStore(origin)->GetCredentials().size(), 0u);
+  EXPECT_EQ(cache()->GetCredentialStore(origin).GetCredentials().size(), 0u);
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/origin_credential_store.cc

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
-#include "url/gurl.h"
 
 namespace password_manager {
 namespace {
@@ -34,10 +33,10 @@ CredentialPair::CredentialPair(const CredentialPair&) = default;
 CredentialPair& CredentialPair::operator=(CredentialPair&&) = default;
 CredentialPair& CredentialPair::operator=(const CredentialPair&) = default;
 
-bool CredentialPair::operator==(const CredentialPair& rhs) const {
-  return username == rhs.username && password == rhs.password &&
-         origin_url == rhs.origin_url &&
-         is_public_suffix_match == rhs.is_public_suffix_match;
+bool operator==(const CredentialPair& lhs, const CredentialPair& rhs) {
+  return lhs.username == rhs.username && lhs.password == rhs.password &&
+         lhs.origin_url == rhs.origin_url &&
+         lhs.is_public_suffix_match == rhs.is_public_suffix_match;
 }
 
 std::ostream& operator<<(std::ostream& os, const CredentialPair& pair) {
@@ -49,7 +48,7 @@ std::ostream& operator<<(std::ostream& os, const CredentialPair& pair) {
 }
 
 OriginCredentialStore::OriginCredentialStore(url::Origin origin)
-    : origin_(origin) {}
+    : origin_(std::move(origin)) {}
 OriginCredentialStore::~OriginCredentialStore() = default;
 
 void OriginCredentialStore::SaveCredentials(
@@ -58,7 +57,7 @@ void OriginCredentialStore::SaveCredentials(
 }
 
 base::span<const CredentialPair> OriginCredentialStore::GetCredentials() const {
-  return base::make_span<>(credentials_);
+  return credentials_;
 }
 
 void OriginCredentialStore::ClearCredentials() {

components/password_manager/core/browser/origin_credential_store.h

@@ -5,12 +5,9 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_ORIGIN_CREDENTIAL_STORE_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_ORIGIN_CREDENTIAL_STORE_H_
 
-#include <map>
-#include <utility>
 #include <vector>
 
 #include "base/containers/span.h"
-#include "base/macros.h"
 #include "base/strings/string16.h"
 #include "url/gurl.h"
 #include "url/origin.h"
@@ -27,14 +24,15 @@ struct CredentialPair {
   CredentialPair(const CredentialPair&);
   CredentialPair& operator=(CredentialPair&&);
   CredentialPair& operator=(const CredentialPair&);
-  bool operator==(const CredentialPair& rhs) const;
 
   base::string16 username;
   base::string16 password;
   GURL origin_url;  // Could be android:// which url::Origin doesn't support.
-  bool is_public_suffix_match;
+  bool is_public_suffix_match = false;
 };
 
+bool operator==(const CredentialPair& lhs, const CredentialPair& rhs);
+
 std::ostream& operator<<(std::ostream& os, const CredentialPair& pair);
 
 // This class stores credential pairs originating from the same origin. The
@@ -43,9 +41,9 @@ std::ostream& operator<<(std::ostream& os, const CredentialPair& pair);
 class OriginCredentialStore {
  public:
   explicit OriginCredentialStore(url::Origin origin);
-  ~OriginCredentialStore();
   OriginCredentialStore(const OriginCredentialStore&) = delete;
   OriginCredentialStore& operator=(const OriginCredentialStore&) = delete;
+  ~OriginCredentialStore();
 
   // Saves credentials so that they can be used in the UI.
   void SaveCredentials(std::vector<CredentialPair> credentials);