Readd base::UnwrapTraits to support rvalue-reference wrappers

Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=554299
Committed: https://crrev.com/001315c4accb9d245badf6244c42d861610d630e
Cr-Commit-Position: refs/heads/master@{#415008}

[tzik, 2016-08-17 09:49:26]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 09:49:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-08-17 14:15:54]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 14:16:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 14:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 14:25:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[tzik, 2016-08-17 14:26:00]

Description was changed from

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

BUG=
==========

to

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=
==========

[tzik, 2016-08-17 14:32:29]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 14:32:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-08-17 14:35:14]

tzik@chromium.org changed reviewers:
+ yutak@chromium.org

[tzik, 2016-08-17 14:37:05]

Description was changed from

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=
==========

to

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=554299
==========

[commit-bot: I haz the power, 2016-08-17 17:23:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 17:23:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tzik, 2016-08-18 04:19:46]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org

[tzik, 2016-08-18 04:19:46]

PTAL

[Yuta Kitamura, 2016-08-18 05:02:34]

wtf/ LGTM

[dcheng, 2016-08-19 04:32:15]

https://codereview.chromium.org/2250373002/diff/40001/base/bind_helpers_unitt...
File base/bind_helpers_unittest.cc (right):

https://codereview.chromium.org/2250373002/diff/40001/base/bind_helpers_unitt...
base/bind_helpers_unittest.cc:16: EXPECT_EQ(&i, internal::Unwrap(unretained));
I feel like we should be testing this at a higher level. Can we verify that this
works by testing some behavior of base::Bind itself?

[tzik, 2016-08-22 14:30:21]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 14:34:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 18:36:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 18:36:07]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-08-23 02:59:59]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 03:00:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-08-23 03:47:13]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 03:47:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-08-23 04:00:16]

https://codereview.chromium.org/2250373002/diff/40001/base/bind_helpers_unitt...
File base/bind_helpers_unittest.cc (right):

https://codereview.chromium.org/2250373002/diff/40001/base/bind_helpers_unitt...
base/bind_helpers_unittest.cc:16: EXPECT_EQ(&i, internal::Unwrap(unretained));
On 2016/08/19 04:32:15, dcheng wrote:
> I feel like we should be testing this at a higher level. Can we verify that
this
> works by testing some behavior of base::Bind itself?

Ok, I updated bind_unittest.cc for Repeating and OneShot.

[dcheng, 2016-08-23 05:25:38]

https://codereview.chromium.org/2250373002/diff/100001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/2250373002/diff/100001/base/bind_helpers.h#ne...
base/bind_helpers.h:286: return Unwrapper<T>::Unwrap(std::forward<T>(o));
Just curious, but can't we just invoke the template directly rather than
forwarding it through this helper?

https://codereview.chromium.org/2250373002/diff/100001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

https://codereview.chromium.org/2250373002/diff/100001/base/bind_unittest.cc#...
base/bind_unittest.cc:416: RepeatingClosure cb = BindRepeating(&TakesACallback,
I guess this change now depends on the OneShotCallback / RepeatingCallback CL?
Any way to decouple these CLs?

[commit-bot: I haz the power, 2016-08-23 05:42:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 05:42:24]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-08-23 07:01:09]

https://codereview.chromium.org/2250373002/diff/100001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/2250373002/diff/100001/base/bind_helpers.h#ne...
base/bind_helpers.h:286: return Unwrapper<T>::Unwrap(std::forward<T>(o));
On 2016/08/23 05:25:38, dcheng wrote:
> Just curious, but can't we just invoke the template directly rather than
> forwarding it through this helper?

It's possible, but it makes the caller a bit more complicated.
I.e.
  Unwrap(base::get<indices>(std::forward<BoundArgsTuple>(bound)))...
in Invoker::RunImpl will be
  BindUnwrapTraits<
      typename std::tuple_element<indices, DecayedArgsTuple>::type
  >::Unwrap(std::get<indices>(std::forward<BoundArgsTuple>(bound)))...

And, it's used in DispatchTo{Method,Function} in tuple.h, which I don't want to
touch now.

https://codereview.chromium.org/2250373002/diff/100001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

https://codereview.chromium.org/2250373002/diff/100001/base/bind_unittest.cc#...
base/bind_unittest.cc:416: RepeatingClosure cb = BindRepeating(&TakesACallback,
On 2016/08/23 05:25:38, dcheng wrote:
> I guess this change now depends on the OneShotCallback / RepeatingCallback CL?
> Any way to decouple these CLs?

There was only one caller of Unwrap before the OneShotCallback CL, and it always
called Unwrap with a const ref argument.
So, the CL is needed to test Unwrap from the layer of Bind.

[dcheng, 2016-08-24 20:22:13]

On 2016/08/23 07:01:09, tzik wrote:
> https://codereview.chromium.org/2250373002/diff/100001/base/bind_helpers.h
> File base/bind_helpers.h (right):
> 
>
https://codereview.chromium.org/2250373002/diff/100001/base/bind_helpers.h#ne...
> base/bind_helpers.h:286: return Unwrapper<T>::Unwrap(std::forward<T>(o));
> On 2016/08/23 05:25:38, dcheng wrote:
> > Just curious, but can't we just invoke the template directly rather than
> > forwarding it through this helper?
> 
> It's possible, but it makes the caller a bit more complicated.
> I.e.
>   Unwrap(base::get<indices>(std::forward<BoundArgsTuple>(bound)))...
> in Invoker::RunImpl will be
>   BindUnwrapTraits<
>       typename std::tuple_element<indices, DecayedArgsTuple>::type
>   >::Unwrap(std::get<indices>(std::forward<BoundArgsTuple>(bound)))...
> 
> And, it's used in DispatchTo{Method,Function} in tuple.h, which I don't want
to
> touch now.
> 
> https://codereview.chromium.org/2250373002/diff/100001/base/bind_unittest.cc
> File base/bind_unittest.cc (right):
> 
>
https://codereview.chromium.org/2250373002/diff/100001/base/bind_unittest.cc#...
> base/bind_unittest.cc:416: RepeatingClosure cb =
BindRepeating(&TakesACallback,
> On 2016/08/23 05:25:38, dcheng wrote:
> > I guess this change now depends on the OneShotCallback / RepeatingCallback
CL?
> > Any way to decouple these CLs?
> 
> There was only one caller of Unwrap before the OneShotCallback CL, and it
always
> called Unwrap with a const ref argument.
> So, the CL is needed to test Unwrap from the layer of Bind.

I see. I didn't realize that. Let's separate the new tests back out to the other
CL then =(

LGTM once that's done, sorry for the back and forth.

[tzik, 2016-08-29 01:43:45]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-08-29 01:43:46]

The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2250373002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-29 01:43:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 03:44:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 03:44:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2016-08-29 12:54:06]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2016-08-29 12:54:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 14:45:11]

Description was changed from

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=554299
==========

to

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=554299
==========

[commit-bot: I haz the power, 2016-08-29 14:45:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-29 14:46:39]

Description was changed from

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=554299
==========

to

==========
Readd base::UnwrapTraits to support rvalue-reference wrappers

base::Bind implementation uses the function overload resolution with ADL
to dispatch bound parameters to Unwrap function. However, this works only
when the bound parameter is passed as a const reference. And other form
of bound parameters, such as rvalue-reference, are fallback to default
Unwrap and fails to compile.

This CL introduces UnwrapTraits<> as a traits struct, and converts Unwap()
function to UnwrapTraits<>::Unwrap(), so that the unwrapping function is
looked up by the normalized type name.

BUG=554299

Committed: https://crrev.com/001315c4accb9d245badf6244c42d861610d630e
Cr-Commit-Position: refs/heads/master@{#415008}
==========

[commit-bot: I haz the power, 2016-08-29 14:46:41]

Patchset 8 (id:??) landed as
https://crrev.com/001315c4accb9d245badf6244c42d861610d630e
Cr-Commit-Position: refs/heads/master@{#415008}