Replace MessageLoopProxy with SingleThreadTaskRunner for media/filters/ + associated code.

Replace MessageLoopProxy with SingleThreadTaskRunner for media/filters/ + associated code.

BUG=315922
R=fischman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242175

[scherkus (not reviewing), 2013-11-07 23:54:00]

boom

[Ami GONE FROM CHROMIUM, 2013-11-08 16:54:45]

https://codereview.chromium.org/65803002/diff/20001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/65803002/diff/20001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
What about content/common/gpu/media ?

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

https://codereview.chromium.org/65803002/diff/20001/media/base/bind_to_loop.h...
media/base/bind_to_loop.h.pump:83: static base::Callback<T> BindToLoop(
Mellifluous though "BindToLoop" is, it's now a bit of a misnomer since it
accepts a runner, not a loop.
Also, would it make sense to give it a TaskRunner instead of a STTR?

https://codereview.chromium.org/65803002/diff/20001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/65803002/diff/20001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:46: task_runner_(task_runner),
I find this version slightly implying that task_runner_ might be different to
MLP::current().  Care to add a DCHECK to the ctor that
task_runner_->RunsOnCurrentThread()?

But really, what is this change buying you over just doing
task_runner_(b::MLP::current())
?

https://codereview.chromium.org/65803002/diff/20001/media/filters/gpu_video_a...
File media/filters/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/65803002/diff/20001/media/filters/gpu_video_a...
media/filters/gpu_video_accelerator_factories.h:56: virtual
scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0;
This change (here and elsewhere) is not a semantic no-op.  
Before, a reader of the callsite of GetMessageLoop() knew that the returned
thing was very likely mapped 1:1 with a thread.  The same can't be said for
GetTaskRunner(), since TaskRunner could be backed by multiple threads.  
GetSingleThreadTaskRunner is too long to be a palatable fix for this
shortcoming, though.

I don't have a good suggestion, just wanted to call this out.  Maybe you have
ideas.
Doesn't need to block the CL.

[scherkus (not reviewing), 2013-11-08 21:51:39]

https://codereview.chromium.org/65803002/diff/20001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/65803002/diff/20001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/11/08 16:54:46, Ami Fischman wrote:
> What about content/common/gpu/media ?

I compiled the world and nothing broke. Try bots seem happy too.

Those classes do have MLP-using code that can be updated but I want to leave
content/*/media/* for a future CL.

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

https://codereview.chromium.org/65803002/diff/20001/media/base/bind_to_loop.h...
media/base/bind_to_loop.h.pump:83: static base::Callback<T> BindToLoop(
On 2013/11/08 16:54:46, Ami Fischman wrote:
> Mellifluous though "BindToLoop" is, it's now a bit of a misnomer since it
> accepts a runner, not a loop.
> Also, would it make sense to give it a TaskRunner instead of a STTR?

I think it's still fair to pass in a STTR.

As discussed, I think the real answer here is to nuke BTL() and possibly rename
BTCL().

https://codereview.chromium.org/65803002/diff/20001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/65803002/diff/20001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:46: task_runner_(task_runner),
On 2013/11/08 16:54:46, Ami Fischman wrote:
> I find this version slightly implying that task_runner_ might be different to
> MLP::current().  Care to add a DCHECK to the ctor that
> task_runner_->RunsOnCurrentThread()?
> 
> But really, what is this change buying you over just doing
> task_runner_(b::MLP::current())
> ?

This change was induced by globally removing all message_loop #includes and
fixing the compilation errors.

Reverted back to MLP but narrowed include to m_l_p.h

https://codereview.chromium.org/65803002/diff/20001/media/filters/gpu_video_a...
File media/filters/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/65803002/diff/20001/media/filters/gpu_video_a...
media/filters/gpu_video_accelerator_factories.h:56: virtual
scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0;
On 2013/11/08 16:54:46, Ami Fischman wrote:
> This change (here and elsewhere) is not a semantic no-op.  
> Before, a reader of the callsite of GetMessageLoop() knew that the returned
> thing was very likely mapped 1:1 with a thread.  The same can't be said for
> GetTaskRunner(), since TaskRunner could be backed by multiple threads.  
> GetSingleThreadTaskRunner is too long to be a palatable fix for this
> shortcoming, though.
> 
> I don't have a good suggestion, just wanted to call this out.  Maybe you have
> ideas.
> Doesn't need to block the CL.

I'm OK leaving the name as is since the return type encodes the type of
TaskRunner it is. It's not perfect ... but I also don't feel it's worth
re-encoding that information in the method name.

FWIW (and I know this isn't a compelling argument!), Get{Foo}TaskRunner() seems
to be the pattern that's emerged in Chromium and they all return a variety of
TaskRunners: http://goo.gl/NksxFF

[Ami GONE FROM CHROMIUM, 2013-11-08 22:57:40]

LGTM % add a TODO to BTL->BTCL and (maybe) rename BTCL?

Of course, if it turns out that BTL can't be removed entirely in favor of BTCL,
that leaves the q of what to do about naming.

[scherkus (not reviewing), 2013-12-20 21:12:52]

Committed patchset #4 manually as r242175 (presubmit successful).