Use enum instead of string in MessageLoopFactory::GetMessageLoop*

Use enum instead of string in MessageLoopFactory::GetMessageLoop*

This reinforces that only certain thread/message_loop can be created, which also makes thread/message_loop access easier and more reliable.

Also use scoped_refptr<MessageLoopProxy> instead of raw MessageLoop pointer in various places.

BUG=none
TEST=media_unittest, content_unittest and normal <video> playback.


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

[xhwang, 2012-08-08 17:35:17]

Hello scherkus,

This is what we discussed before. This is a short term change that will make it
easier for the VideoRendererBase to get a hold of the video decoder thread's
message loop.

xhwang

[scherkus (not reviewing), 2012-08-09 17:13:24]

I'm OK with this change as an incremental cleanup CL but we should really,
*really* get away from using MLF

http://codereview.chromium.org/10855051/diff/5002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10855051/diff/5002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2428: message_loop_factory->GetMessageLoop(
inside GVD it grabs the proxy

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
File media/base/message_loop_factory.cc (right):

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.cc:16: static const MessageLoopTypeNamePair
message_loop_type_to_name_mapping[] = {
this should be kMessageLoopXxx

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
File media/base/message_loop_factory.h (right):

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.h:30: enum MessageLoopType {
s/MessageLoop/Type/

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.h:31: AUDIO_DECODER,
we've been using kCamelCase format for constants

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.h:40: MessageLoop*
GetMessageLoop(MessageLoopType type);
can we remove this method entirely and rename the one below to GetMessageLoop()?
folks shouldn't be holding onto raw pointers

I realize this might expand the scope of the change but it's a worthwhile cause
:)

[Ami GONE FROM CHROMIUM, 2012-08-09 17:19:39]

drive-by

http://codereview.chromium.org/10855051/diff/5002/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10855051/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:507:
media::MessageLoopFactory::VIDEO_DECODER),
This change makes it so that a single thread is used for this and
CreateLocalVideoDecoder() above, where before 2 were used.
I'm not sure if that's a problem (i.e. can both methods ever be called with the
same factory, i.e. the same media element?)

[xhwang, 2012-08-10 01:04:28]

+fischman

This CL gets larger than I expected. But most changes are duplicate, so it
shouldn't be too bad for review. PTAL!

http://codereview.chromium.org/10855051/diff/5002/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10855051/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:507:
media::MessageLoopFactory::VIDEO_DECODER),
On 2012/08/09 17:19:39, Ami Fischman wrote:
> This change makes it so that a single thread is used for this and
> CreateLocalVideoDecoder() above, where before 2 were used.
> I'm not sure if that's a problem (i.e. can both methods ever be called with
the
> same factory, i.e. the same media element?)

As per scherkus@, either local or remote video decoder is used in one media
element, not both. So we should be fine here.

http://codereview.chromium.org/10855051/diff/5002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10855051/diff/5002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2428: message_loop_factory->GetMessageLoop(
On 2012/08/09 17:13:24, scherkus wrote:
> inside GVD it grabs the proxy

Done.

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
File media/base/message_loop_factory.cc (right):

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.cc:16: static const MessageLoopTypeNamePair
message_loop_type_to_name_mapping[] = {
On 2012/08/09 17:13:24, scherkus wrote:
> this should be kMessageLoopXxx

Done.

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
File media/base/message_loop_factory.h (right):

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.h:30: enum MessageLoopType {
On 2012/08/09 17:13:24, scherkus wrote:
> s/MessageLoop/Type/

Done.

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.h:31: AUDIO_DECODER,
On 2012/08/09 17:13:24, scherkus wrote:
> we've been using kCamelCase format for constants

Done.

http://codereview.chromium.org/10855051/diff/5002/media/base/message_loop_fac...
media/base/message_loop_factory.h:40: MessageLoop*
GetMessageLoop(MessageLoopType type);
On 2012/08/09 17:13:24, scherkus wrote:
> can we remove this method entirely and rename the one below to
GetMessageLoop()?
> folks shouldn't be holding onto raw pointers
> 
> I realize this might expand the scope of the change but it's a worthwhile
cause
> :)

Done.

[Ami GONE FROM CHROMIUM, 2012-08-10 04:38:06]

LGTM % nits

http://codereview.chromium.org/10855051/diff/1024/media/base/message_loop_fac...
File media/base/message_loop_factory.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/base/message_loop_fac...
media/base/message_loop_factory.cc:53: base::Thread* thread = new
base::Thread(name.c_str());
I think you're working too hard on this :)
l.16-21 and l45-57 can be replaced with:

const char* name = NULL;
switch (type) {
 case kAudioDecoder: 
  name = "AudioDecoderThread"; 
  break;
 case kVideoDecoder: 
  name = "VideoDecoderThread"; 
  break;
 case kPipeline:
  name = "PipelineThread"; 
  break;
}
base::Thread* thread = new base::Thread(name);
CHECK(thread->Start()) << "Failed to start thread: " << name;
threads_.push_back(std::make_pair(type, thread));
return thread;


(the benefits are:
1) compiler checks all the types are handled
2) the literal names are present exactly where they are used
3) it's simpler/shorter)

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:28: typedef
base::Callback<scoped_refptr<base::MessageLoopProxy>()> MessageLoopCB;
s/MessageLoopCB/MessageLoopFactoryCB/

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.h:27: typedef
base::Callback<scoped_refptr<base::MessageLoopProxy>()> MessageLoopCB;
ditto (here and elsewhere)

http://codereview.chromium.org/10855051/diff/1024/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:124: 
DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());

[scherkus (not reviewing), 2012-08-10 17:00:09]

lgtm w/ nits

thanks for going the extra mile :)

http://codereview.chromium.org/10855051/diff/1024/media/base/message_loop_fac...
File media/base/message_loop_factory.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/base/message_loop_fac...
media/base/message_loop_factory.cc:53: base::Thread* thread = new
base::Thread(name.c_str());
On 2012/08/10 04:38:06, Ami Fischman wrote:
> I think you're working too hard on this :)
> l.16-21 and l45-57 can be replaced with:
> 
> const char* name = NULL;
> switch (type) {
>  case kAudioDecoder: 
>   name = "AudioDecoderThread"; 
>   break;
>  case kVideoDecoder: 
>   name = "VideoDecoderThread"; 
>   break;
>  case kPipeline:
>   name = "PipelineThread"; 
>   break;
> }
> base::Thread* thread = new base::Thread(name);
> CHECK(thread->Start()) << "Failed to start thread: " << name;
> threads_.push_back(std::make_pair(type, thread));
> return thread;
> 
> 
> (the benefits are:
> 1) compiler checks all the types are handled
> 2) the literal names are present exactly where they are used
> 3) it's simpler/shorter)

+1

http://codereview.chromium.org/10855051/diff/1024/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/base/pipeline.cc#newc...
media/base/pipeline.cc:551: //                 and all the Closures are
converted to PipelineStatusCB.
don't indent -- people with long ldaps get cheated on usable whitespace!

[xhwang, 2012-08-10 19:33:32]

http://codereview.chromium.org/10855051/diff/1024/media/base/message_loop_fac...
File media/base/message_loop_factory.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/base/message_loop_fac...
media/base/message_loop_factory.cc:53: base::Thread* thread = new
base::Thread(name.c_str());
On 2012/08/10 04:38:06, Ami Fischman wrote:
> I think you're working too hard on this :)
> l.16-21 and l45-57 can be replaced with:
> 
> const char* name = NULL;
> switch (type) {
>  case kAudioDecoder: 
>   name = "AudioDecoderThread"; 
>   break;
>  case kVideoDecoder: 
>   name = "VideoDecoderThread"; 
>   break;
>  case kPipeline:
>   name = "PipelineThread"; 
>   break;
> }
> base::Thread* thread = new base::Thread(name);
> CHECK(thread->Start()) << "Failed to start thread: " << name;
> threads_.push_back(std::make_pair(type, thread));
> return thread;
> 
> 
> (the benefits are:
> 1) compiler checks all the types are handled
> 2) the literal names are present exactly where they are used
> 3) it's simpler/shorter)

Done.

http://codereview.chromium.org/10855051/diff/1024/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/base/pipeline.cc#newc...
media/base/pipeline.cc:551: //                 and all the Closures are
converted to PipelineStatusCB.
On 2012/08/10 17:00:09, scherkus wrote:
> don't indent -- people with long ldaps get cheated on usable whitespace!

Done.

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:28: typedef
base::Callback<scoped_refptr<base::MessageLoopProxy>()> MessageLoopCB;
On 2012/08/10 04:38:06, Ami Fischman wrote:
> s/MessageLoopCB/MessageLoopFactoryCB/

Done.

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/10855051/diff/1024/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.h:27: typedef
base::Callback<scoped_refptr<base::MessageLoopProxy>()> MessageLoopCB;
On 2012/08/10 04:38:06, Ami Fischman wrote:
> ditto (here and elsewhere)

Done.

http://codereview.chromium.org/10855051/diff/1024/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10855051/diff/1024/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:124: 
On 2012/08/10 04:38:06, Ami Fischman wrote:
> DCHECK(gvd_loop_proxy_->BelongsToCurrentThread());

Done.

[xhwang, 2012-08-10 19:39:23]

Hello brettw,

Can you do an OWNERS review on these two files?

content/renderer/render_view_impl.cc
webkit/support/test_media_stream_client.cc

Thanks,
xhwang

[brettw, 2012-08-10 22:50:23]

Those two files LGTM

[commit-bot: I haz the power, 2012-08-11 00:57:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10855051/9003

[commit-bot: I haz the power, 2012-08-11 00:57:26]

Failed to apply patch for content/renderer/render_view_impl.cc:
While running patch -p1 --forward --force;
patching file content/renderer/render_view_impl.cc
Hunk #2 FAILED at 2417.
1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.cc.rej

[commit-bot: I haz the power, 2012-08-11 17:22:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10855051/5008

[commit-bot: I haz the power, 2012-08-12 02:29:05]

Failed to apply patch for
content/renderer/media/renderer_gpu_video_decoder_factories.cc:
While running patch -p1 --forward --force;
patching file content/renderer/media/renderer_gpu_video_decoder_factories.cc
Hunk #3 FAILED at 67.
1 out of 6 hunks FAILED -- saving rejects to file
content/renderer/media/renderer_gpu_video_decoder_factories.cc.rej

[commit-bot: I haz the power, 2012-08-12 06:30:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10855051/6008

[tae420, 2012-08-12 07:47:59]

ส่ท

[commit-bot: I haz the power, 2012-08-12 14:33:40]

List of reviewers changed. tae420@hotmail.com did a drive-by without LGTM'ing!

[commit-bot: I haz the power, 2012-08-12 21:37:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10855051/6008

[commit-bot: I haz the power, 2012-08-12 22:51:22]

Change committed as 151204