Remove MessageLoop methods from Filter interface

Remove MessageLoop methods from Filter interface to
separate Filter management from MessageLoop management.
This sets the stage for filters to share threads in the
future which will reduce resource consumption when
multiple <video> tags are on the same page.

BUG=69451
TEST=None

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

[scherkus (not reviewing), 2011-01-13 06:28:12]

this is really cool!  I like the idea of pulling more of the filter
configuration outside of pipeline (even if it means each "application" has to do
some extra up-front configuration -- I think that can be addressed with default
configs or what have you)

one comment about using ref-counted thread safe though

other style gotchas (I didn't comment on any of them but will do so in a follow
up review):
  - one-arg constructors should have explicit (some ctors now need explicit,
some now don't)
  - I think there's a few spots where we can forward declare MessageLoop instead
of header file
  - switching to std::string for MessageLoopFactory (probably reasonable to do
so)
  - ctor initializer list (the : thing) goes on next line

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

http://codereview.chromium.org/6171009/diff/1/media/base/message_loop_factory...
media/base/message_loop_factory.h:16: public
base::RefCountedThreadSafe<MessageLoopFactory> {
does this actually have to be ref-counted?

looking at the usage in player_x11, player_wtl, render_view, etc... it looks
like we can rely on passing ownership as it only seems to be used during filter
creation + then can get tossed away (if need be)

[acolwell GONE FROM CHROMIUM, 2011-01-13 23:15:00]

Applied suggested changes and fixed a variety of style issues.

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

http://codereview.chromium.org/6171009/diff/1/media/base/message_loop_factory...
media/base/message_loop_factory.h:16: public
base::RefCountedThreadSafe<MessageLoopFactory> {
I'll remove it for now since there isn't a need yet. I believe I added this when
I was playing around with thread sharing and having the RenderView own the
factory.
On 2011/01/13 06:28:13, scherkus wrote:
> does this actually have to be ref-counted?
> 
> looking at the usage in player_x11, player_wtl, render_view, etc... it looks
> like we can rely on passing ownership as it only seems to be used during
filter
> creation + then can get tossed away (if need be)

[scherkus (not reviewing), 2011-01-13 23:52:50]

LGTM + maybe attempt to clean up the message loop functions if at all possible +
nits

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
File media/base/message_loop_factory.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory.h:5: #ifndef MEDIA_BASE_MESSAGE_LOOP_FACTORY_H
nit: header guards end with trailing _

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory.h:21: // Null is returned if |name| is null,
|name| is an empty string, or a new
Null -> NULL

also |name| can no longer be NULL

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
File media/base/message_loop_factory_impl.cc (right):

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.cc:36: base::Thread* thread =
thread_map_[std::string(name)];
no need for std::string ctor here

also do we want to use thread_map_.find() instead?  otherwise we'll create an
entry

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.cc:43: message_loop =
thread->message_loop();
wondering if we can clean up these nested ifs by just doing:

if (thread)
  return thread->message_loop();

thread = new Thread();
if (thread->Start()) {
  // blah..
  return thread->message_loop();
}

LOG(ERROR) << "OMG"
return NULL;

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.cc:45: delete thread;
maybe LOG(ERROR)?

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
File media/base/message_loop_factory_impl.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.h:28: mutable Lock lock_;
nit: is mutable needed?  not seeing any const methods

http://codereview.chromium.org/6171009/diff/5001/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:147: MessageLoop* message_loop();
who's calling this method today?

http://codereview.chromium.org/6171009/diff/5001/media/filters/omx_video_deco...
File media/filters/omx_video_decoder.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/filters/omx_video_deco...
media/filters/omx_video_decoder.h:39: MessageLoop* message_loop();
nit: add blank line after this

http://codereview.chromium.org/6171009/diff/5001/media/tools/player_wtl/view.h
File media/tools/player_wtl/view.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/tools/player_wtl/view....
media/tools/player_wtl/view.h:12: #include "base/logging.h"
nit: is this needed?

[acolwell GONE FROM CHROMIUM, 2011-01-14 01:14:12]

Applied suggested changes and removed message_loop() methods where possible.

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
File media/base/message_loop_factory.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory.h:5: #ifndef MEDIA_BASE_MESSAGE_LOOP_FACTORY_H
On 2011/01/13 23:52:50, scherkus wrote:
> nit: header guards end with trailing _

Done.

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory.h:21: // Null is returned if |name| is null,
|name| is an empty string, or a new
On 2011/01/13 23:52:50, scherkus wrote:
> Null -> NULL
> 
> also |name| can no longer be NULL

Done.

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
File media/base/message_loop_factory_impl.cc (right):

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.cc:36: base::Thread* thread =
thread_map_[std::string(name)];
Done. Changed code to use find.

On 2011/01/13 23:52:50, scherkus wrote:
> no need for std::string ctor here
> 
> also do we want to use thread_map_.find() instead?  otherwise we'll create an
> entry

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.cc:43: message_loop =
thread->message_loop();
On 2011/01/13 23:52:50, scherkus wrote:
> wondering if we can clean up these nested ifs by just doing:
> 
> if (thread)
>   return thread->message_loop();
> 
> thread = new Thread();
> if (thread->Start()) {
>   // blah..
>   return thread->message_loop();
> }
> 
> LOG(ERROR) << "OMG"
> return NULL;

Done.

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.cc:45: delete thread;
On 2011/01/13 23:52:50, scherkus wrote:
> maybe LOG(ERROR)?

Done.

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
File media/base/message_loop_factory_impl.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/base/message_loop_fact...
media/base/message_loop_factory_impl.h:28: mutable Lock lock_;
On 2011/01/13 23:52:50, scherkus wrote:
> nit: is mutable needed?  not seeing any const methods

Done.

http://codereview.chromium.org/6171009/diff/5001/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:147: MessageLoop* message_loop();
FFmpegDemuxerStream. Some rework could be done to eliminate the need, but that
should be in a separate CL.
On 2011/01/13 23:52:50, scherkus wrote:
> who's calling this method today?

http://codereview.chromium.org/6171009/diff/5001/media/filters/omx_video_deco...
File media/filters/omx_video_decoder.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/filters/omx_video_deco...
media/filters/omx_video_decoder.h:39: MessageLoop* message_loop();
Removed method since it is no longer needed.
On 2011/01/13 23:52:50, scherkus wrote:
> nit: add blank line after this

http://codereview.chromium.org/6171009/diff/5001/media/tools/player_wtl/view.h
File media/tools/player_wtl/view.h (right):

http://codereview.chromium.org/6171009/diff/5001/media/tools/player_wtl/view....
media/tools/player_wtl/view.h:12: #include "base/logging.h"
Yes. It is for a DCHECK() that is in this file.
On 2011/01/13 23:52:50, scherkus wrote:
> nit: is this needed?

[scherkus (not reviewing), 2011-01-14 01:15:37]

nice!  LGTM!