Support move-only type on base::Callback::Run

Change base::Callback<R(Args...)>::Run argument signature from
CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the
arguments in the rest of the callchain to the bound function.

This implies making an extra copy when a pass-by-value argument of
Callback::Run() is a copyable-but-not-efficiently-movable types.
That is due to broken Perfect Forwarding from the callchain from
Callback::Run() to the bound function. However, Perfect Forwarding
idiom is not fully applicable to the callchain, since we have to
determine the type of a function pointer in the Callback ctor,
before Callback::Run() is called.

Also, this CL converts a copy construction to a move construction
when a pass-by-value argument of Callback::Run() is a general movable
type. E.g. std::string, std::map and others movable class without
DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND.

BUG=554299

Review URL: https://codereview.chromium.org/1709483002

Cr-Commit-Position: refs/heads/master@{#380080}

4 files changed, 48 insertions, 38 deletions

diff --git a/base/bind_helpers.h b/base/bind_helpers.h
index 660348e..a551465 100644
--- a/base/bind_helpers.h
+++ b/base/bind_helpers.h
@@ -563,15 +563,12 @@ static inline internal::OwnedWrapper<T> Owned(T* o) {
 // Both versions of Passed() prevent T from being an lvalue reference. The first
 // via use of enable_if, and the second takes a T* which will not bind to T&.
 template <typename T,
-          typename std::enable_if<internal::IsMoveOnlyType<T>::value &&
-                                  !std::is_lvalue_reference<T>::value>::type* =
+          typename std::enable_if<!std::is_lvalue_reference<T>::value>::type* =
               nullptr>
 static inline internal::PassedWrapper<T> Passed(T&& scoper) {
   return internal::PassedWrapper<T>(std::move(scoper));
 }
-template <typename T,
-          typename std::enable_if<internal::IsMoveOnlyType<T>::value>::type* =
-              nullptr>
+template <typename T>
 static inline internal::PassedWrapper<T> Passed(T* scoper) {
   return internal::PassedWrapper<T>(std::move(*scoper));
 }
diff --git a/base/bind_internal.h b/base/bind_internal.h
index f296bbc..38a5756 100644
--- a/base/bind_internal.h
+++ b/base/bind_internal.h
@@ -339,19 +339,19 @@ template <size_t... bound_indices,
           typename StorageType,
           typename InvokeHelperType,
           typename R,
-          typename... UnboundForwardArgs>
-struct Invoker<IndexSequence<bound_indices...>, StorageType,
-               InvokeHelperType, R(UnboundForwardArgs...)> {
-  static R Run(BindStateBase* base,
-               UnboundForwardArgs... unbound_args) {
+          typename... UnboundArgs>
+struct Invoker<IndexSequence<bound_indices...>,
+               StorageType,
+               InvokeHelperType,
+               R(UnboundArgs...)> {
+  static R Run(BindStateBase* base, UnboundArgs&&... unbound_args) {
     StorageType* storage = static_cast<StorageType*>(base);
     // Local references to make debugger stepping easier. If in a debugger,
     // you really want to warp ahead and step through the
     // InvokeHelper<>::MakeItSo() call below.
     return InvokeHelperType::MakeItSo(
-        storage->runnable_,
-        Unwrap(get<bound_indices>(storage->bound_args_))...,
-        CallbackForward(unbound_args)...);
+        storage->runnable_, Unwrap(get<bound_indices>(storage->bound_args_))...,
+        std::forward<UnboundArgs>(unbound_args)...);
   }
 };
 
@@ -406,18 +406,14 @@ struct BindState<Runnable, R(Args...), BoundArgs...> final
       IsWeakMethod<is_method, typename std::decay<BoundArgs>::type...>;
 
   using BoundIndices = MakeIndexSequence<sizeof...(BoundArgs)>;
-  using UnboundForwardArgs = DropTypeListItem<
-      sizeof...(BoundArgs),
-      TypeList<typename CallbackParamTraits<Args>::ForwardType...>>;
-  using UnboundForwardRunType = MakeFunctionType<R, UnboundForwardArgs>;
   using InvokeHelperType = InvokeHelper<IsWeakCall::value, R, Runnable>;
 
   using UnboundArgs = DropTypeListItem<sizeof...(BoundArgs), TypeList<Args...>>;
 
  public:
-  using InvokerType = Invoker<BoundIndices, StorageType,
-                              InvokeHelperType, UnboundForwardRunType>;
   using UnboundRunType = MakeFunctionType<R, UnboundArgs>;
+  using InvokerType =
+      Invoker<BoundIndices, StorageType, InvokeHelperType, UnboundRunType>;
 
   template <typename... ForwardArgs>
   BindState(const Runnable& runnable, ForwardArgs&&... bound_args)
diff --git a/base/bind_unittest.cc b/base/bind_unittest.cc
index 0595ed3..daf8c12 100644
--- a/base/bind_unittest.cc
+++ b/base/bind_unittest.cc
@@ -928,7 +928,7 @@ TEST_F(BindTest, ArgumentCopies) {
   copies = 0;
   assigns = 0;
   Bind(&VoidPolymorphic<CopyCounter>::Run).Run(counter);
-  EXPECT_EQ(1, copies);
+  EXPECT_EQ(2, copies);
   EXPECT_EQ(0, assigns);
 
   copies = 0;
@@ -967,7 +967,20 @@ TEST_F(BindTest, ArgumentMoves) {
   // TODO(tzik): Support binding move-only type into a non-reference parameter
   // of a variant of Callback.
 
-  // TODO(tzik): Support move-only type forwarding on Callback::Run.
+  move_constructs = 0;
+  move_assigns = 0;
+  Bind(&VoidPolymorphic<MoveCounter>::Run)
+      .Run(MoveCounter(&move_constructs, &move_assigns));
+  EXPECT_EQ(1, move_constructs);
+  EXPECT_EQ(0, move_assigns);
+
+  move_constructs = 0;
+  move_assigns = 0;
+  Bind(&VoidPolymorphic<MoveCounter>::Run)
+      .Run(MoveCounter(DerivedCopyMoveCounter(
+          nullptr, nullptr, &move_constructs, &move_assigns)));
+  EXPECT_EQ(2, move_constructs);
+  EXPECT_EQ(0, move_assigns);
 }
 
 // Argument constructor usage for non-reference movable-copyable
@@ -1006,22 +1019,20 @@ TEST_F(BindTest, ArgumentCopiesAndMoves) {
   Bind(&VoidPolymorphic<CopyMoveCounter>::Run).Run(counter);
   EXPECT_EQ(1, copies);
   EXPECT_EQ(0, assigns);
-  EXPECT_EQ(0, move_constructs);
+  EXPECT_EQ(1, move_constructs);
   EXPECT_EQ(0, move_assigns);
 
-  // TODO(tzik): This case should be done in no copy and one move.
   copies = 0;
   assigns = 0;
   move_constructs = 0;
   move_assigns = 0;
   Bind(&VoidPolymorphic<CopyMoveCounter>::Run)
       .Run(CopyMoveCounter(&copies, &assigns, &move_constructs, &move_assigns));
-  EXPECT_EQ(1, copies);
+  EXPECT_EQ(0, copies);
   EXPECT_EQ(0, assigns);
-  EXPECT_EQ(0, move_constructs);
+  EXPECT_EQ(1, move_constructs);
   EXPECT_EQ(0, move_assigns);
 
-  // TODO(tzik): This case should be done in one copy and one move.
   DerivedCopyMoveCounter derived_counter(&copies, &assigns, &move_constructs,
                                          &move_assigns);
   copies = 0;
@@ -1030,12 +1041,11 @@ TEST_F(BindTest, ArgumentCopiesAndMoves) {
   move_assigns = 0;
   Bind(&VoidPolymorphic<CopyMoveCounter>::Run)
       .Run(CopyMoveCounter(derived_counter));
-  EXPECT_EQ(2, copies);
+  EXPECT_EQ(1, copies);
   EXPECT_EQ(0, assigns);
-  EXPECT_EQ(0, move_constructs);
+  EXPECT_EQ(1, move_constructs);
   EXPECT_EQ(0, move_assigns);
 
-  // TODO(tzik): This case should be done in no copy and two move.
   copies = 0;
   assigns = 0;
   move_constructs = 0;
@@ -1043,9 +1053,9 @@ TEST_F(BindTest, ArgumentCopiesAndMoves) {
   Bind(&VoidPolymorphic<CopyMoveCounter>::Run)
       .Run(CopyMoveCounter(DerivedCopyMoveCounter(
           &copies, &assigns, &move_constructs, &move_assigns)));
-  EXPECT_EQ(1, copies);
+  EXPECT_EQ(0, copies);
   EXPECT_EQ(0, assigns);
-  EXPECT_EQ(1, move_constructs);
+  EXPECT_EQ(2, move_constructs);
   EXPECT_EQ(0, move_assigns);
 }
 
diff --git a/base/callback.h b/base/callback.h
index 3bf0008..bff1dc4 100644
--- a/base/callback.h
+++ b/base/callback.h
@@ -386,18 +386,25 @@ class Callback<R(Args...)> : public internal::CallbackBase {
     return CallbackBase::Equals(other);
   }
 
-  R Run(typename internal::CallbackParamTraits<Args>::ForwardType... args)
-      const {
+  // Run() makes an extra copy compared to directly calling the bound function
+  // if an argument is passed-by-value and is copyable-but-not-movable:
+  // i.e. below copies CopyableNonMovableType twice.
+  //   void F(CopyableNonMovableType) {}
+  //   Bind(&F).Run(CopyableNonMovableType());
+  //
+  // We can not fully apply Perfect Forwarding idiom to the callchain from
+  // Callback::Run() to the target function. Perfect Forwarding requires
+  // knowing how the caller will pass the arguments. However, the signature of
+  // InvokerType::Run() needs to be fixed in the callback constructor, so Run()
+  // cannot template its arguments based on how it's called.
+  R Run(Args... args) const {
     PolymorphicInvoke f =
         reinterpret_cast<PolymorphicInvoke>(polymorphic_invoke_);
-
-    return f(bind_state_.get(), internal::CallbackForward(args)...);
+    return f(bind_state_.get(), std::forward<Args>(args)...);
   }
 
  private:
-  using PolymorphicInvoke =
-      R(*)(internal::BindStateBase*,
-           typename internal::CallbackParamTraits<Args>::ForwardType...);
+  using PolymorphicInvoke = R (*)(internal::BindStateBase*, Args&&...);
 };
 
 }  // namespace base