[Passwords] Generalize SortKeyIdGenerator

This change generalizes the SortKeyIdGenerator to take arbitrary types
as keys, and allows customization of the id type and key comparator.
This will be useful when we need to generate ids for compromised
credentials as well. To make the name more generic the class is renamed
to IdGenerator.

TBR=schwering@google.com

Bug: 1047726
Change-Id: I1b9e25aa4ccd0a732c810bde20163d46d5a46d67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2080430Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745485}

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc

@@ -155,7 +155,7 @@ void PasswordsPrivateDelegateImpl::ChangeSavedPassword(
     int id,
     base::string16 new_username,
     base::Optional<base::string16> new_password) {
-  const std::string* sort_key = password_id_generator_.TryGetSortKey(id);
+  const std::string* sort_key = password_id_generator_.TryGetKey(id);
   DCHECK(sort_key);
   password_manager_presenter_->ChangeSavedPassword(
       *sort_key, std::move(new_username), std::move(new_password));
@@ -168,7 +168,7 @@ void PasswordsPrivateDelegateImpl::RemoveSavedPassword(int id) {
 }
 
 void PasswordsPrivateDelegateImpl::RemoveSavedPasswordInternal(int id) {
-  const std::string* sort_key = password_id_generator_.TryGetSortKey(id);
+  const std::string* sort_key = password_id_generator_.TryGetKey(id);
   if (sort_key)
     password_manager_presenter_->RemoveSavedPassword(*sort_key);
 }
@@ -180,7 +180,7 @@ void PasswordsPrivateDelegateImpl::RemovePasswordException(int id) {
 }
 
 void PasswordsPrivateDelegateImpl::RemovePasswordExceptionInternal(int id) {
-  const std::string* sort_key = exception_id_generator_.TryGetSortKey(id);
+  const std::string* sort_key = exception_id_generator_.TryGetKey(id);
   if (sort_key)
     password_manager_presenter_->RemovePasswordException(*sort_key);
 }
@@ -218,7 +218,7 @@ void PasswordsPrivateDelegateImpl::RequestPlaintextPassword(
   }
 
   // Request the password. When it is retrieved, ShowPassword() will be called.
-  const std::string* sort_key = password_id_generator_.TryGetSortKey(id);
+  const std::string* sort_key = password_id_generator_.TryGetKey(id);
   if (!sort_key) {
     std::move(callback).Run(base::nullopt);
     return;
@@ -424,7 +424,7 @@ void PasswordsPrivateDelegateImpl::Shutdown() {
   password_manager_presenter_.reset();
 }
 
-SortKeyIdGenerator&
+IdGenerator<std::string>&
 PasswordsPrivateDelegateImpl::GetPasswordIdGeneratorForTesting() {
   return password_id_generator_;
 }

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h

@@ -78,7 +78,7 @@ class PasswordsPrivateDelegateImpl : public PasswordsPrivateDelegate,
   // KeyedService overrides:
   void Shutdown() override;
 
-  SortKeyIdGenerator& GetPasswordIdGeneratorForTesting();
+  IdGenerator<std::string>& GetPasswordIdGeneratorForTesting();
 
 #if defined(UNIT_TEST)
   // Use this in tests to mock the OS-level reauthentication.
@@ -139,8 +139,8 @@ class PasswordsPrivateDelegateImpl : public PasswordsPrivateDelegate,
 
   // Generators that map between sort keys used by |password_manager_presenter_|
   // and ids used by the JavaScript front end.
-  SortKeyIdGenerator password_id_generator_;
-  SortKeyIdGenerator exception_id_generator_;
+  IdGenerator<std::string> password_id_generator_;
+  IdGenerator<std::string> exception_id_generator_;
 
   // Whether SetPasswordList and SetPasswordExceptionList have been called, and
   // whether this class has been initialized, meaning both have been called.

chrome/browser/extensions/api/passwords_private/passwords_private_utils.cc

@@ -23,28 +23,4 @@ api::passwords_private::UrlCollection CreateUrlCollectionFromForm(
   return urls;
 }
 
-SortKeyIdGenerator::SortKeyIdGenerator() = default;
-
-SortKeyIdGenerator::~SortKeyIdGenerator() = default;
-
-int SortKeyIdGenerator::GenerateId(const std::string& sort_key) {
-  auto result = sort_key_cache_.emplace(sort_key, next_id_);
-  if (result.second) {
-    // In case we haven't seen |sort_key| before, add a pointer to the inserted
-    // key and the corresponding id to the |id_cache_|. This insertion should
-    // always succeed.
-    auto iter =
-        id_cache_.emplace_hint(id_cache_.end(), next_id_, &result.first->first);
-    DCHECK_EQ(&result.first->first, iter->second);
-    ++next_id_;
-  }
-
-  return result.first->second;
-}
-
-const std::string* SortKeyIdGenerator::TryGetSortKey(int id) const {
-  auto it = id_cache_.find(id);
-  return it != id_cache_.end() ? it->second : nullptr;
-}
-
 }  // namespace extensions

chrome/browser/extensions/api/passwords_private/passwords_private_utils.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_PASSWORDS_PRIVATE_PASSWORDS_PRIVATE_UTILS_H_
 #define CHROME_BROWSER_EXTENSIONS_API_PASSWORDS_PRIVATE_PASSWORDS_PRIVATE_UTILS_H_
 
+#include <functional>
 #include <map>
 #include <string>
 
@@ -23,34 +24,52 @@ namespace extensions {
 api::passwords_private::UrlCollection CreateUrlCollectionFromForm(
     const autofill::PasswordForm& form);
 
-// This class serves as an identifier for strings created by CreateSortKey() in
-// the password_manager namespace. It is similar to base::IDMap, but also allows
-// to get the id for an already inserted element.
-// TODO(https://crbug.com/778146): This class should be an implementation detail
-// of PasswordManagerPresenter. Move this class there once Android uses a
-// similar logic.
-class SortKeyIdGenerator {
+// This class is an id generator for an arbitrary key type. It is used by both
+// PasswordManagerPresenter and PasswordCheckDelegate to create ids send to the
+// UI. It is similar to base::IDMap, but has the following important
+// differences:
+// - IdGenerator owns a copy of the key data, so that clients don't need to
+//   worry about dangling pointers.
+// - Repeated calls to GenerateId with the same |key| are no-ops, and return the
+//   same ids.
+template <typename KeyT,
+          typename IdT = int32_t,
+          typename KeyCompare = std::less<>>
+class IdGenerator {
  public:
-  SortKeyIdGenerator();
-  ~SortKeyIdGenerator();
-
-  // This method generates an id corresponding to |sort_key|. Additionally it
+  // This method generates an id corresponding to |key|. Additionally it
   // remembers ids generated in the past, so that this method is idempotent.
   // Furthermore, it is guaranteed that different ids are returned for different
-  // |sort_key| arguments. This implies GenerateId(a) == GenerateId(b) if and
-  // only if a == b.
-  int GenerateId(const std::string& sort_key);
+  // |key| arguments. This implies GenerateId(a) == GenerateId(b) if and only if
+  // a == b.
+  IdT GenerateId(const KeyT& key) {
+    auto result = key_cache_.emplace(key, next_id_);
+    if (result.second) {
+      // In case we haven't seen |key| before, add a pointer to the inserted key
+      // and the corresponding id to the |id_cache_|. This insertion should
+      // always succeed.
+      auto iter = id_cache_.emplace_hint(id_cache_.end(), next_id_,
+                                         &result.first->first);
+      DCHECK_EQ(&result.first->first, iter->second);
+      ++next_id_;
+    }
+
+    return result.first->second;
+  }
 
-  // This method tries to return the sort key corresponding to |id|. In case
-  // |id| was not generated by GenerateId() before, this method returns nullptr.
-  // Otherwise it returns a pointer s to a sort key, such that |id| ==
-  // GenerateId(*s).
-  const std::string* TryGetSortKey(int id) const;
+  // This method tries to return the key corresponding to |id|. In case |id| was
+  // not generated by GenerateId() before, this method returns nullptr.
+  // Otherwise it returns a pointer p to a key, such that |id| ==
+  // GenerateId(*p).
+  const KeyT* TryGetKey(IdT id) const {
+    auto it = id_cache_.find(id);
+    return it != id_cache_.end() ? it->second : nullptr;
+  }
 
  private:
-  std::map<std::string, int> sort_key_cache_;
-  base::flat_map<int, const std::string*> id_cache_;
-  int next_id_ = 0;
+  std::map<KeyT, IdT, KeyCompare> key_cache_;
+  base::flat_map<IdT, const KeyT*> id_cache_;
+  IdT next_id_ = 0;
 };
 
 }  // namespace extensions

chrome/browser/extensions/api/passwords_private/passwords_private_utils_unittest.cc

@@ -63,19 +63,19 @@ TEST(CreateUrlCollectionFromFormTest, UrlsFromAndroidFormWithAppName) {
             android_urls.link);
 }
 
-TEST(SortKeyIdGeneratorTest, GenerateIds) {
+TEST(IdGeneratorTest, GenerateIds) {
   using ::testing::Pointee;
   using ::testing::Eq;
 
-  SortKeyIdGenerator id_generator;
+  IdGenerator<std::string> id_generator;
   int foo_id = id_generator.GenerateId("foo");
 
   // Check idempotence.
   EXPECT_EQ(foo_id, id_generator.GenerateId("foo"));
 
-  // Check TryGetSortKey(id) == s iff id == GenerateId(*s).
-  EXPECT_THAT(id_generator.TryGetSortKey(foo_id), Pointee(Eq("foo")));
-  EXPECT_EQ(nullptr, id_generator.TryGetSortKey(foo_id + 1));
+  // Check TryGetKey(id) == s iff id == GenerateId(*s).
+  EXPECT_THAT(id_generator.TryGetKey(foo_id), Pointee(Eq("foo")));
+  EXPECT_EQ(nullptr, id_generator.TryGetKey(foo_id + 1));
 
   // Check that different sort keys result in different ids.
   int bar_id = id_generator.GenerateId("bar");
@@ -83,8 +83,8 @@ TEST(SortKeyIdGeneratorTest, GenerateIds) {
   EXPECT_NE(foo_id, bar_id);
   EXPECT_NE(bar_id, baz_id);
 
-  EXPECT_THAT(id_generator.TryGetSortKey(bar_id), Pointee(Eq("bar")));
-  EXPECT_THAT(id_generator.TryGetSortKey(baz_id), Pointee(Eq("baz")));
+  EXPECT_THAT(id_generator.TryGetKey(bar_id), Pointee(Eq("bar")));
+  EXPECT_THAT(id_generator.TryGetKey(baz_id), Pointee(Eq("baz")));
 }
 
 }  // namespace extensions