media::BindToLoop() is born, with example uses to slim down WebMediaPlayerProxy.

media::BindToLoop() is born, with example uses to slim down WebMediaPlayerProxy.

WMPP could use further slimming-down / flamethrowering, but this pass just removes
the lowest-hanging fruit, where WMPP is used as a silent trampoline to WMPI on
the render thread.


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

[Ami GONE FROM CHROMIUM, 2012-08-15 23:50:12]

scherkus/acolwell: please review.
ajwong: FYI.

This is a semi-speculative CL.  I like the resulting code in WMPI (and the
resulting *lack* of code in WMPP), but it might cross some people's complexity
thresholds.

[acolwell GONE FROM CHROMIUM, 2012-08-16 00:14:56]

Overall I like this change. The actual callback function name gets lost in all
the BindToLoop & base::Bind boiler-plate. A helper macro could fix that, but
probably isn't worth it.

[Ami GONE FROM CHROMIUM, 2012-08-16 03:02:46]

On 2012/08/16 00:14:56, acolwell wrote:
> Overall I like this change. The actual callback function name gets lost in all
> the BindToLoop & base::Bind boiler-plate. A helper macro could fix that, but
> probably isn't worth it.

I don't think it'd be worth exposing a macro from bind_to_loop.h, but it's an
interesting idea for wmpi.cc.  Done in patchset #2.

[acolwell GONE FROM CHROMIUM, 2012-08-16 16:05:02]

\o/ LGTM

[awong, 2012-08-16 17:19:21]

unitests? :D

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
File media/base/bind_to_loop.h (right):

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:28: static void TrampolineRun(
Should these be in an internal namespace?

Also, should these be out-of-lined?

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:29: scoped_refptr<base::MessageLoopProxy> loop, const
base::Closure& cb) {
Can |loop| be a const ref?

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:42: A1 a1) {
We're not unwrapping A1 into a ForwardType like Callback does.  It also doesn't
use CallbackForward() to pass the actual |a1| into the later Bind().

I think this might break some things such as
  - array types
  - move-only types
  - non-const reference

I'm guessing you may not care, but can we verify and call it out in the comment
as an expected "won't work?"

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:69: static void TrampolineRun(
If you use the funciton-signature trick to make TrampolineRun a single-argument
template, I think you can remove all the specializations of BindToLoop.  You'd
have to embed TrampileRun into a class or struct though.

template <typename T> struct Trampoline;
template <typename A1, typename A2>
struct TrampolineHelper<void(A1, A2)> {
  Callback<void(A1, A2)> Do(
     scoped_refptr<base::MessageLoopProxy> loop,
     const base::Callback<void(A1, A2)>& cb,
     A1 a1, A2 a2)
}

and then just one

template <typename T>
static base::Callback<T> BindToLoop(
    scoped_refptr<base::MessageLoopProxy> loop,
    const base::Callback<T>& cb) {
  return base::Bind(&TrampolineHelper::Do, loop, cb);
}

[scherkus (not reviewing), 2012-08-16 19:01:33]

+1 to what ajwong said

[Ami GONE FROM CHROMIUM, 2012-08-16 20:25:53]

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
File media/base/bind_to_loop.h (right):

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:6: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
Now with tests!

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:28: static void TrampolineRun(
The "static"s are leftover from prototyping.  Gone.

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:28: static void TrampolineRun(
On 2012/08/16 17:19:21, awong wrote:
> Should these be in an internal namespace?

You mean put TrampolineRun in media::internal but leave BindToLoop in media?  I
didn't think it was worth it when there were $ARITY BTL's, but now that there's
only one using the struct trick, done.

> Also, should these be out-of-lined?

Which "these"?  The arity>0 cases are templatized, so must appear in the .h. 
Moving just the arity==0 cases out-of-line doesn't seem worthwhile to me.

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:29: scoped_refptr<base::MessageLoopProxy> loop, const
base::Closure& cb) {
On 2012/08/16 17:19:21, awong wrote:
> Can |loop| be a const ref?

Done.

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:42: A1 a1) {
On 2012/08/16 17:19:21, awong wrote:
> We're not unwrapping A1 into a ForwardType like Callback does.  It also
doesn't
> use CallbackForward() to pass the actual |a1| into the later Bind().
> 
> I think this might break some things such as
>   - array types
>   - move-only types
>   - non-const reference
> 
> I'm guessing you may not care, but can we verify and call it out in the
comment
> as an expected "won't work?"

I'd rather things worked across the board, and I was able to make scoped_ptr
(move-only) work by adding CallbackForward.

As discussed offline, ForwardType doesn't work here, necessitating arg copy and
excluding arrays (added comment).

You mentioned non-const refs; aren't those banned from Bind() in the first
place?

https://chromiumcodereview.appspot.com/10855188/diff/1008/media/base/bind_to_...
media/base/bind_to_loop.h:69: static void TrampolineRun(
On 2012/08/16 17:19:21, awong wrote:
> If you use the funciton-signature trick to make TrampolineRun a
single-argument
> template, I think you can remove all the specializations of BindToLoop.  You'd
> have to embed TrampileRun into a class or struct though.
> 
> template <typename T> struct Trampoline;
> template <typename A1, typename A2>
> struct TrampolineHelper<void(A1, A2)> {
>   Callback<void(A1, A2)> Do(
>      scoped_refptr<base::MessageLoopProxy> loop,
>      const base::Callback<void(A1, A2)>& cb,
>      A1 a1, A2 a2)
> }
> 
> and then just one
> 
> template <typename T>
> static base::Callback<T> BindToLoop(
>     scoped_refptr<base::MessageLoopProxy> loop,
>     const base::Callback<T>& cb) {
>   return base::Bind(&TrampolineHelper::Do, loop, cb);
> }

Nice, done.

[scherkus (not reviewing), 2012-08-16 22:05:49]

leaving it to ajwong to review the pump stuff

I've got one q: it looks like WMPI is already WeakPtr<T>!

https://chromiumcodereview.appspot.com/10855188/diff/10002/webkit/media/webme...
File webkit/media/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/10855188/diff/10002/webkit/media/webme...
webkit/media/webmediaplayer_impl.h:90: public
base::SupportsWeakPtr<WebMediaPlayerImpl> {
uhm?

[Ami GONE FROM CHROMIUM, 2012-08-16 22:11:11]

https://chromiumcodereview.appspot.com/10855188/diff/10002/webkit/media/webme...
File webkit/media/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/10855188/diff/10002/webkit/media/webme...
webkit/media/webmediaplayer_impl.h:90: public
base::SupportsWeakPtr<WebMediaPlayerImpl> {
On 2012/08/16 22:05:49, scherkus wrote:
> uhm?

Nice catch :)
Done.

[scherkus (not reviewing), 2012-08-16 22:13:47]

lgtm -- as discussed offline we can switch to factory weakptr in followup cl

[awong, 2012-08-17 18:27:18]

LGTM w/nits

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.pump
File media/base/bind_to_loop.h.pump (right):

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.p...
media/base/bind_to_loop.h.pump:19: #include "base/callback_internal.h" // Avoid
re-inventing CallbackForward/ForwardType.
80-chars

Also you don't use ForwardType

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.p...
media/base/bind_to_loop.h.pump:47: static void Do(
Rename to Run()?

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.p...
media/base/bind_to_loop.h.pump:58: 
drope one newline?

[Ami GONE FROM CHROMIUM, 2012-08-17 23:55:46]

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.pump
File media/base/bind_to_loop.h.pump (right):

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.p...
media/base/bind_to_loop.h.pump:19: #include "base/callback_internal.h" // Avoid
re-inventing CallbackForward/ForwardType.
On 2012/08/17 18:27:18, awong wrote:
> 80-chars
> 
> Also you don't use ForwardType

Done.

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.p...
media/base/bind_to_loop.h.pump:47: static void Do(
On 2012/08/17 18:27:18, awong wrote:
> Rename to Run()?

Done.

http://codereview.chromium.org/10855188/diff/9008/media/base/bind_to_loop.h.p...
media/base/bind_to_loop.h.pump:58: 
On 2012/08/17 18:27:18, awong wrote:
> drope one newline?

Done.

[commit-bot: I haz the power, 2012-08-18 00:51:16]

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

[commit-bot: I haz the power, 2012-08-18 00:51:18]

Failed to apply patch for webkit/media/webmediaplayer_impl.cc:
While running patch -p1 --forward --force;
patching file webkit/media/webmediaplayer_impl.cc
Hunk #3 succeeded at 165 (offset 1 line).
Hunk #4 succeeded at 262 (offset 1 line).
Hunk #5 succeeded at 341 (offset 1 line).
Hunk #6 FAILED at 971.
1 out of 6 hunks FAILED -- saving rejects to file
webkit/media/webmediaplayer_impl.cc.rej

[commit-bot: I haz the power, 2012-08-18 01:24:37]

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

[commit-bot: I haz the power, 2012-08-18 04:10:21]

Change committed as 152239