Fix RunOnceClosure<N>

The existing RunOnceClosure<N> implementation wouldn't compile if the
Nth argument was a OnceClosure passed by value. Use the same action
machinery as RunOnceCallback<N> to fix this.

Bug: none
Change-Id: I4fe5fe187ead7bedd084b26575e74da7fab3f7ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1925556Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719610}

base/test/gmock_callback_support.h

@@ -15,9 +15,52 @@
 namespace base {
 namespace test {
 
+// TODO(crbug.com/752720): Simplify using std::apply once C++17 is available.
+template <typename CallbackFunc, typename ArgTuple, size_t... I>
+decltype(auto) RunOnceCallbackUnwrapped(CallbackFunc&& f,
+                                        ArgTuple&& t,
+                                        std::index_sequence<I...>) {
+  return std::move(f).Run(std::get<I>(t)...);
+}
+
+// TODO(crbug.com/752720): Simplify using std::apply once C++17 is available.
+template <typename CallbackFunc, typename ArgTuple, size_t... I>
+decltype(auto) RunRepeatingCallbackUnwrapped(CallbackFunc&& f,
+                                             ArgTuple&& t,
+                                             std::index_sequence<I...>) {
+  return f.Run(std::get<I>(t)...);
+}
+
+// Functor used for RunOnceClosure<N>() and RunOnceCallback<N>() actions.
+template <size_t I, typename... Vals>
+struct RunOnceCallbackAction {
+  std::tuple<Vals...> vals;
+
+  template <typename... Args>
+  decltype(auto) operator()(Args&&... args) {
+    constexpr size_t size = std::tuple_size<decltype(vals)>::value;
+    return RunOnceCallbackUnwrapped(
+        std::get<I>(std::forward_as_tuple(std::forward<Args>(args)...)),
+        std::move(vals), std::make_index_sequence<size>{});
+  }
+};
+
+// Functor used for RunClosure<N>() and RunCallback<N>() actions.
+template <size_t I, typename... Vals>
+struct RunRepeatingCallbackAction {
+  std::tuple<Vals...> vals;
+
+  template <typename... Args>
+  decltype(auto) operator()(Args&&... args) {
+    constexpr size_t size = std::tuple_size<decltype(vals)>::value;
+    return RunRepeatingCallbackUnwrapped(
+        std::get<I>(std::forward_as_tuple(std::forward<Args>(args)...)),
+        std::move(vals), std::make_index_sequence<size>{});
+  }
+};
+
 // Matchers for base::{Once,Repeating}Callback and
 // base::{Once,Repeating}Closure.
-
 MATCHER(IsNullCallback, "a null callback") {
   return (arg.is_null());
 }
@@ -26,33 +69,32 @@ MATCHER(IsNotNullCallback, "a non-null callback") {
   return (!arg.is_null());
 }
 
-// The RunClosure<N>() action invokes Run() method on the N-th (0-based)
-// argument of the mock function.
-
-ACTION_TEMPLATE(RunClosure,
-                HAS_1_TEMPLATE_PARAMS(int, k),
-                AND_0_VALUE_PARAMS()) {
-  ::testing::get<k>(args).Run();
-}
-
+// The Run[Once]Closure() action invokes the Run() method on the closure
+// provided when the action is constructed.
 ACTION_P(RunClosure, closure) {
   closure.Run();
 }
 
-ACTION_TEMPLATE(RunOnceClosure,
-                HAS_1_TEMPLATE_PARAMS(int, k),
-                AND_0_VALUE_PARAMS()) {
-  std::move(::testing::get<k>(args)).Run();
-}
-
+// TODO(kylechar): This action can't take a OnceClosure by value, fix that.
 ACTION_P(RunOnceClosure, closure) {
   std::move(closure).Run();
 }
 
-// Implementation of the Run(Once)Callback gmock action.
-//
-// The RunCallback<N>(p1, p2, ..., p_k) action invokes Run() method on the N-th
-// (0-based) argument of the mock function, with arguments p1, p2, ..., p_k.
+// The Run[Once]Closure<N>() action invokes the Run() method on the N-th
+// (0-based) argument of the mock function.
+template <size_t I>
+RunRepeatingCallbackAction<I> RunClosure() {
+  return {};
+}
+
+template <size_t I>
+RunOnceCallbackAction<I> RunOnceClosure() {
+  return {};
+}
+
+// The Run[Once]Callback<N>(p1, p2, ..., p_k) action invokes the Run() method on
+// the N-th (0-based) argument of the mock function, with arguments p1, p2, ...,
+// p_k.
 //
 // Notes:
 //
@@ -74,28 +116,6 @@ ACTION_P(RunOnceClosure, closure) {
 //   reference of the copy, instead of the original temporary object,
 //   to the callback.  This makes it easy for a user to define an
 //   RunCallback action from temporary values and have it performed later.
-
-// TODO(crbug.com/752720): Simplify using std::apply once C++17 is available.
-template <typename CallbackFunc, typename ArgTuple, size_t... I>
-decltype(auto) RunCallbackUnwrapped(CallbackFunc&& f,
-                                    ArgTuple&& t,
-                                    std::index_sequence<I...>) {
-  return std::move(f).Run(std::get<I>(t)...);
-}
-
-template <size_t I, typename... Vals>
-struct RunOnceCallbackAction {
-  std::tuple<Vals...> vals;
-
-  template <typename... Args>
-  decltype(auto) operator()(Args&&... args) {
-    constexpr size_t size = std::tuple_size<decltype(vals)>::value;
-    return RunCallbackUnwrapped(
-        std::get<I>(std::forward_as_tuple(std::forward<Args>(args)...)),
-        std::move(vals), std::make_index_sequence<size>{});
-  }
-};
-
 template <size_t I, typename... Vals>
 RunOnceCallbackAction<I, std::decay_t<Vals>...> RunOnceCallback(
     Vals&&... vals) {
@@ -103,7 +123,8 @@ RunOnceCallbackAction<I, std::decay_t<Vals>...> RunOnceCallback(
 }
 
 template <size_t I, typename... Vals>
-RunOnceCallbackAction<I, std::decay_t<Vals>...> RunCallback(Vals&&... vals) {
+RunRepeatingCallbackAction<I, std::decay_t<Vals>...> RunCallback(
+    Vals&&... vals) {
   return {std::forward_as_tuple(std::forward<Vals>(vals)...)};
 }
 

base/test/gmock_callback_support_unittest.cc

@@ -34,7 +34,19 @@ TEST(GmockCallbackSupportTest, IsNotNullCallback) {
   check.Call(base::BindRepeating(&SetBool));
 }
 
-TEST(GmockCallbackSupportTest, RunClosure) {
+TEST(GmockCallbackSupportTest, IsNullOnceCallback) {
+  MockFunction<void(TestOnceCallback)> mock;
+  EXPECT_CALL(mock, Call(IsNullCallback()));
+  mock.Call(TestOnceCallback());
+}
+
+TEST(GmockCallbackSupportTest, IsNotNullOnceCallback) {
+  MockFunction<void(TestOnceCallback)> mock;
+  EXPECT_CALL(mock, Call(IsNotNullCallback()));
+  mock.Call(base::BindOnce(&SetBool));
+}
+
+TEST(GmockCallbackSupportTest, RunClosure0) {
   MockFunction<void(const base::RepeatingClosure&)> check;
   bool dst = false;
   EXPECT_CALL(check, Call(IsNotNullCallback())).WillOnce(RunClosure<0>());
@@ -42,6 +54,17 @@ TEST(GmockCallbackSupportTest, RunClosure) {
   EXPECT_TRUE(dst);
 }
 
+TEST(GmockCallbackSupportTest, RunClosureByRefNotReset) {
+  // Check that RepeatingClosure isn't reset by RunClosure<N>().
+  MockFunction<void(base::RepeatingClosure&)> check;
+  bool dst = false;
+  EXPECT_CALL(check, Call(IsNotNullCallback())).WillOnce(RunClosure<0>());
+  auto closure = base::BindRepeating(&SetBool, true, &dst);
+  check.Call(closure);
+  EXPECT_TRUE(dst);
+  EXPECT_FALSE(closure.is_null());
+}
+
 TEST(GmockCallbackSupportTest, RunCallback0) {
   MockFunction<void(const TestCallback&)> check;
   bool dst = false;
@@ -82,15 +105,15 @@ TEST(GmockCallbackSupportTest, RunCallbackPassByValue) {
   EXPECT_TRUE(dst);
 }
 
-TEST(GmockCallbackSupportTest, RunOnceClosure) {
+TEST(GmockCallbackSupportTest, RunOnceClosure0) {
   MockFunction<void(base::OnceClosure)> check;
   bool dst = false;
-  EXPECT_CALL(check, Call(IsNotNullCallback())).WillOnce(RunOnceCallback<0>());
+  EXPECT_CALL(check, Call(IsNotNullCallback())).WillOnce(RunOnceClosure<0>());
   check.Call(base::BindOnce(&SetBool, true, &dst));
   EXPECT_TRUE(dst);
 }
 
-TEST(GmockCallbackSupportTest, RunOnceCallback) {
+TEST(GmockCallbackSupportTest, RunOnceCallback0) {
   MockFunction<void(TestOnceCallback)> check;
   bool dst = false;
   bool src = true;