Teach BindToLoop to create callbacks that accept scoped parameters.

Teach BindToLoop to create callbacks that accept scoped parameters.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163729

[Ami GONE FROM CHROMIUM, 2012-10-10 20:59:39]

ajwong: I hope to move BindToLoop to base/ to make the poking at
base::internal::PassedWrapper less questionable, but I'm curious what you think
of this CL as it is.

[awong, 2012-10-11 00:11:23]

If we're going to do this, this is the only way to do it that I know of so LGTM
from that perspective.

[awong, 2012-10-11 00:18:22]

http://codereview.chromium.org/11092054/diff/1/media/base/bind_to_loop.h
File media/base/bind_to_loop.h (right):

http://codereview.chromium.org/11092054/diff/1/media/base/bind_to_loop.h#newc...
media/base/bind_to_loop.h:37: T& TrampolineForward(T& t) { return t; }
Crazy idea...can we do anything really trick with TrampolineForward so that we
don't need to expose PassedWrapper?

Maybe somehow specialize a struct on T that can can infer the type of Passed()
and expose a funciton pointer to the right version???

[Ami GONE FROM CHROMIUM, 2012-10-11 07:20:38]

http://codereview.chromium.org/11092054/diff/1/media/base/bind_to_loop.h
File media/base/bind_to_loop.h (right):

http://codereview.chromium.org/11092054/diff/1/media/base/bind_to_loop.h#newc...
media/base/bind_to_loop.h:37: T& TrampolineForward(T& t) { return t; }
On 2012/10/11 00:18:22, awong wrote:
> Crazy idea...can we do anything really trick with TrampolineForward so that we
> don't need to expose PassedWrapper?
> 
> Maybe somehow specialize a struct on T that can can infer the type of Passed()
> and expose a funciton pointer to the right version???

Played with this for a while and failed at it (and the things I was trying would
be far more heinous in my eyes than the peek behind base::Passed()'s curtain).

[Ami GONE FROM CHROMIUM, 2012-10-23 06:43:53]

FTR, convos about moving this to base/ have stalled, but this CL is still an
improvement in terms of poking at internals less, so I'm inclined to land it.

Esp. since I found out tonight that the lack of the facility was causing yet
more verbosity elsewhere (see PS#4).  Adding xhwang@ for media/ review.

[xhwang, 2012-10-23 21:37:03]

lgtm % nits

https://codereview.chromium.org/11092054/diff/13001/media/base/bind_to_loop.h...
File media/base/bind_to_loop.h.pump (right):

https://codereview.chromium.org/11092054/diff/13001/media/base/bind_to_loop.h...
media/base/bind_to_loop.h.pump:80: 
Did you put two empty lines here on purpose?

https://codereview.chromium.org/11092054/diff/13001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11092054/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:99: #define BIND_TO_RENDER_LOOP_2(function,
arg1, arg2)                 \
remove extra spaces?

https://codereview.chromium.org/11092054/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:271:
BIND_TO_RENDER_LOOP_2(&WebMediaPlayerImpl::OnNeedKey, "", ""));
Thanks for fixing this :)

[Ami GONE FROM CHROMIUM, 2012-10-23 21:58:07]

Thanks for the review.

https://codereview.chromium.org/11092054/diff/13001/media/base/bind_to_loop.h...
File media/base/bind_to_loop.h.pump (right):

https://codereview.chromium.org/11092054/diff/13001/media/base/bind_to_loop.h...
media/base/bind_to_loop.h.pump:80: 
On 2012/10/23 21:37:03, xhwang wrote:
> Did you put two empty lines here on purpose?

Mimicing the callback pump file.  I don't think it matters.

https://codereview.chromium.org/11092054/diff/13001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/11092054/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:99: #define BIND_TO_RENDER_LOOP_2(function,
arg1, arg2)                 \
On 2012/10/23 21:37:03, xhwang wrote:
> remove extra spaces?

Done.

[commit-bot: I haz the power, 2012-10-23 21:59:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/11092054/20001

[commit-bot: I haz the power, 2012-10-24 00:12:06]

Change committed as 163729