Revert of Do Perfect Forwarding from base::Bind to BindState storage (patchset #2 id:20001 of https://codereview.chromium.org/1644603003/ )

Reason for revert:
This CL broke Windows build at an instantiation for:
e:\b\build\slave\win_x64_gn__dbg_\build\src\components\mus\gles2\command_buffer_local.cc(195)

The error message is:
e:\b\build\slave\win_x64_gn__dbg_\build\src\base\bind_internal.h(291) : warning C4267: 'argument' : conversion from 'size_t' to 'const uint32_t', possible loss of data

https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29/builds/17854/steps/compile/logs/stdio

Original issue's description:
> Do Perfect Forwarding from base::Bind to BindState storage
>
> Replace StorageType usage with std::decay and do Perfect Forwarding from
> base::Bind to BindState storage as a preparation to store a move-only
> type into BindState.
>
> The difference of StorageType and std::decay is essentially array
> handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
> OTOH std::decay converts it to "T*".
>
> BUG=554299
>
> Committed: https://crrev.com/cdec75e36dd5ec9c5643f7b473e166a3acef81ea
> Cr-Commit-Position: refs/heads/master@{#372900}

TBR=danakj@chromium.org,thakis@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=554299

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

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

3 files changed, 35 insertions, 11 deletions

diff --git a/base/bind.h b/base/bind.h
index c6b7393..770e457 100644
--- a/base/bind.h
+++ b/base/bind.h
@@ -6,6 +6,7 @@
 #define BASE_BIND_H_
 
 #include "base/bind_internal.h"
+#include "base/callback_internal.h"
 
 // -----------------------------------------------------------------------------
 // Usage documentation
@@ -51,8 +52,9 @@ base::Callback<
     typename internal::BindState<
         typename internal::FunctorTraits<Functor>::RunnableType,
         typename internal::FunctorTraits<Functor>::RunType,
-        typename std::decay<Args>::type...>::UnboundRunType>
-Bind(Functor functor, Args&&... args) {
+        typename internal::CallbackParamTraits<Args>::StorageType...>
+            ::UnboundRunType>
+Bind(Functor functor, const Args&... args) {
   // Type aliases for how to store and run the functor.
   using RunnableType = typename internal::FunctorTraits<Functor>::RunnableType;
   using RunType = typename internal::FunctorTraits<Functor>::RunType;
@@ -87,11 +89,11 @@ Bind(Functor functor, Args&&... args) {
       "a parameter is a refcounted type and needs scoped_refptr");
 
   using BindState = internal::BindState<
-      RunnableType, RunType, typename std::decay<Args>::type...>;
+      RunnableType, RunType,
+      typename internal::CallbackParamTraits<Args>::StorageType...>;
 
   return Callback<typename BindState::UnboundRunType>(
-      new BindState(internal::MakeRunnable(functor),
-                    std::forward<Args>(args)...));
+      new BindState(internal::MakeRunnable(functor), args...));
 }
 
 }  // namespace base
diff --git a/base/bind_internal.h b/base/bind_internal.h
index ce6b5d6..ac7cd00 100644
--- a/base/bind_internal.h
+++ b/base/bind_internal.h
@@ -104,8 +104,7 @@ template <bool is_method, typename... Args>
 struct BindsArrayToFirstArg : false_type {};
 
 template <typename T, typename... Args>
-struct BindsArrayToFirstArg<true, T, Args...>
-    : is_array<typename std::remove_reference<T>::type> {};
+struct BindsArrayToFirstArg<true, T, Args...> : is_array<T> {};
 
 // HasRefCountedParamAsRawPtr is the same to HasRefCountedTypeAsRawPtr except
 // when |is_method| is true HasRefCountedParamAsRawPtr skips the first argument.
@@ -402,12 +401,11 @@ struct BindState<Runnable, R(Args...), BoundArgs...> final
                               InvokeHelperType, UnboundForwardRunType>;
   using UnboundRunType = MakeFunctionType<R, UnboundArgs>;
 
-  template <typename... ForwardArgs>
-  BindState(const Runnable& runnable, ForwardArgs&&... bound_args)
+  BindState(const Runnable& runnable, const BoundArgs&... bound_args)
       : BindStateBase(&Destroy),
         runnable_(runnable),
         ref_(bound_args...),
-        bound_args_(std::forward<ForwardArgs>(bound_args)...) {}
+        bound_args_(bound_args...) {}
 
   RunnableType runnable_;
   MaybeScopedRefPtr<HasIsMethodTag<Runnable>::value, BoundArgs...> ref_;
diff --git a/base/callback_internal.h b/base/callback_internal.h
index 3e8ee82..227a041 100644
--- a/base/callback_internal.h
+++ b/base/callback_internal.h
@@ -136,7 +136,16 @@ struct CallbackParamTraitsForNonMoveOnlyType;
 // http://connect.microsoft.com/VisualStudio/feedbackdetail/view/957801/compilation-error-with-variadic-templates
 //
 // This is a typetraits object that's used to take an argument type, and
-// extract a suitable type for forwarding arguments.
+// extract a suitable type for storing and forwarding arguments.
+//
+// In particular, it strips off references, and converts arrays to
+// pointers for storage; and it avoids accidentally trying to create a
+// "reference of a reference" if the argument is a reference type.
+//
+// This array type becomes an issue for storage because we are passing bound
+// parameters by const reference. In this case, we end up passing an actual
+// array type in the initializer list which C++ does not allow.  This will
+// break passing of C-string literals.
 template <typename T>
 struct CallbackParamTraits
     : std::conditional<IsMoveOnlyType<T>::value,
@@ -147,6 +156,18 @@ struct CallbackParamTraits
 template <typename T>
 struct CallbackParamTraitsForNonMoveOnlyType {
   using ForwardType = const T&;
+  using StorageType = T;
+};
+
+// The Storage should almost be impossible to trigger unless someone manually
+// specifies type of the bind parameters.  However, in case they do,
+// this will guard against us accidentally storing a reference parameter.
+//
+// The ForwardType should only be used for unbound arguments.
+template <typename T>
+struct CallbackParamTraitsForNonMoveOnlyType<T&> {
+  using ForwardType = T&;
+  using StorageType = T;
 };
 
 // Note that for array types, we implicitly add a const in the conversion. This
@@ -157,12 +178,14 @@ struct CallbackParamTraitsForNonMoveOnlyType {
 template <typename T, size_t n>
 struct CallbackParamTraitsForNonMoveOnlyType<T[n]> {
   using ForwardType = const T*;
+  using StorageType = const T*;
 };
 
 // See comment for CallbackParamTraits<T[n]>.
 template <typename T>
 struct CallbackParamTraitsForNonMoveOnlyType<T[]> {
   using ForwardType = const T*;
+  using StorageType = const T*;
 };
 
 // Parameter traits for movable-but-not-copyable scopers.
@@ -181,6 +204,7 @@ struct CallbackParamTraitsForNonMoveOnlyType<T[]> {
 template <typename T>
 struct CallbackParamTraitsForMoveOnlyType {
   using ForwardType = T;
+  using StorageType = T;
 };
 
 // CallbackForward() is a very limited simulation of C++11's std::forward()