DataItem Callback Conversion - Test callbacks

This CL is the fist in a series to convert DataItem from the deprecated
base::Callback (and Bind) to [Once|Repeating]Callback. See the relation
chain of CLs for the complete change.

This CL starts by converting a set of injectable callbacks used for unit
tests to observe this class. These callbacks may be called repeatedly
after being registered so they are RepeatingCallbacks. However,
ItemFactoryCallback and ItemStoreDeleter both take a callback as a
parameter. The unit test implementations only invoke these parameters
once so we convert them to the "Once" variant.

The RegisteredValuesCallback is a type defined in DataItem and used
elsewhere. For now, we add a "Once" typedef to prevent changing too
much. This will be removed in a downstream patch once all uses of the
ambiguous form are removed.

Bug: 1152268
Change-Id: I694df544b97ede7b1e5aa8af388ca4ed186d8156
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595932Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838226}

extensions/browser/api/lock_screen_data/data_item.h

@@ -37,9 +37,15 @@ class DataItem {
   using ReadCallback =
       base::Callback<void(OperationResult result,
                           std::unique_ptr<std::vector<char>> data)>;
+  // TODO(bokan): The multiple callback versions are temporary while we remove
+  // the base::Callback version in favor of base::OnceCallback.
+  // https://crbug.com/1152268.
   using RegisteredValuesCallback =
       base::Callback<void(OperationResult result,
                           std::unique_ptr<base::DictionaryValue> values)>;
+  using RegisteredValuesOnceCallback =
+      base::OnceCallback<void(OperationResult result,
+                              std::unique_ptr<base::DictionaryValue> values)>;
 
   // Gets all registered data items for the extension with the provided
   // extension ID - the items are returned as a DictionaryValue with keys set

extensions/browser/api/lock_screen_data/lock_screen_item_storage.h

@@ -140,15 +140,16 @@ class LockScreenItemStorage : public ExtensionRegistryObserver {
       ValueStoreMigratorFactoryCallback* factory_callback);
 
   // Used in tests to inject fake data items implementations.
-  using ItemFactoryCallback =
-      base::Callback<std::unique_ptr<DataItem>(const std::string& id,
-                                               const std::string& extension_id,
-                                               const std::string& crypto_key)>;
-  using RegisteredItemsGetter =
-      base::Callback<void(const std::string& extension_id,
-                          const DataItem::RegisteredValuesCallback& callback)>;
-  using ItemStoreDeleter = base::Callback<void(const std::string& extension_id,
-                                               const base::Closure& callback)>;
+  using ItemFactoryCallback = base::RepeatingCallback<std::unique_ptr<DataItem>(
+      const std::string& id,
+      const std::string& extension_id,
+      const std::string& crypto_key)>;
+  using RegisteredItemsGetter = base::RepeatingCallback<void(
+      const std::string& extension_id,
+      DataItem::RegisteredValuesOnceCallback callback)>;
+  using ItemStoreDeleter =
+      base::RepeatingCallback<void(const std::string& extension_id,
+                                   base::OnceClosure callback)>;
   static void SetItemProvidersForTesting(
       RegisteredItemsGetter* item_fetch_callback,
       ItemFactoryCallback* factory_callback,

extensions/browser/api/lock_screen_data/lock_screen_item_storage_unittest.cc

@@ -157,22 +157,20 @@ class ItemRegistry {
   void RemoveAll() { items_.clear(); }
 
   // Gets the set of registered data items.
-  void HandleGetRequest(const DataItem::RegisteredValuesCallback& callback) {
+  void HandleGetRequest(DataItem::RegisteredValuesOnceCallback callback) {
     if (!throttle_get_) {
-      RunCallback(callback);
+      RunCallback(std::move(callback));
       return;
     }
 
     ASSERT_TRUE(pending_callback_.is_null());
-    pending_callback_ = callback;
+    pending_callback_ = std::move(callback);
   }
 
   // Completes a pending |HandleGetRequest| request.
   void RunPendingCallback() {
     ASSERT_FALSE(pending_callback_.is_null());
-    DataItem::RegisteredValuesCallback callback = pending_callback_;
-    pending_callback_.Reset();
-    RunCallback(callback);
+    RunCallback(std::move(pending_callback_));
   }
 
   bool HasPendingCallback() const { return !pending_callback_.is_null(); }
@@ -182,9 +180,10 @@ class ItemRegistry {
   void set_throttle_get(bool throttle_get) { throttle_get_ = throttle_get; }
 
  private:
-  void RunCallback(const DataItem::RegisteredValuesCallback& callback) {
-    callback.Run(fail_ ? OperationResult::kFailed : OperationResult::kSuccess,
-                 ItemsToValue());
+  void RunCallback(DataItem::RegisteredValuesOnceCallback callback) {
+    std::move(callback).Run(
+        fail_ ? OperationResult::kFailed : OperationResult::kSuccess,
+        ItemsToValue());
   }
 
   std::unique_ptr<base::DictionaryValue> ItemsToValue() {
@@ -211,7 +210,7 @@ class ItemRegistry {
   // complete the request.
   bool throttle_get_ = false;
 
-  DataItem::RegisteredValuesCallback pending_callback_;
+  DataItem::RegisteredValuesOnceCallback pending_callback_;
   // Set of registered item ids.
   std::set<std::string> items_;
 
@@ -483,12 +482,12 @@ class LockScreenItemStorageTest : public ExtensionsTest {
     LockScreenItemStorage::SetValueStoreMigratorFactoryForTesting(
         &migrator_factory_);
 
-    item_factory_ = base::Bind(&LockScreenItemStorageTest::CreateItem,
-                               base::Unretained(this));
-    registered_items_getter_ = base::Bind(
+    item_factory_ = base::BindRepeating(&LockScreenItemStorageTest::CreateItem,
+                                        base::Unretained(this));
+    registered_items_getter_ = base::BindRepeating(
         &LockScreenItemStorageTest::GetRegisteredItems, base::Unretained(this));
-    item_store_deleter_ = base::Bind(&LockScreenItemStorageTest::RemoveAllItems,
-                                     base::Unretained(this));
+    item_store_deleter_ = base::BindRepeating(
+        &LockScreenItemStorageTest::RemoveAllItems, base::Unretained(this));
     LockScreenItemStorage::SetItemProvidersForTesting(
         &registered_items_getter_, &item_factory_, &item_store_deleter_);
 
@@ -735,19 +734,19 @@ class LockScreenItemStorageTest : public ExtensionsTest {
   }
 
   void GetRegisteredItems(const std::string& extension_id,
-                          const DataItem::RegisteredValuesCallback& callback) {
+                          DataItem::RegisteredValuesOnceCallback callback) {
     if (extension()->id() != extension_id) {
-      callback.Run(OperationResult::kUnknownExtension, nullptr);
+      std::move(callback).Run(OperationResult::kUnknownExtension, nullptr);
       return;
     }
-    item_registry_->HandleGetRequest(callback);
+    item_registry_->HandleGetRequest(std::move(callback));
   }
 
   void RemoveAllItems(const std::string& extension_id,
-                      const base::Closure& callback) {
+                      base::OnceClosure callback) {
     ASSERT_EQ(extension()->id(), extension_id);
     item_registry_->RemoveAll();
-    callback.Run();
+    std::move(callback).Run();
   }
 
   std::unique_ptr<LockScreenItemStorage> lock_screen_item_storage_;