Implemented injected message loops for PipelineImpl.

Implemented injected message loops for PipelineImpl.

For now both the player and WebMediaPlayerImpl create a thread and inject its message loop into the pipeline.  The end result is more-or-less the same as what we have today, but we could end up moving the pipeline onto the render thread.

BUG=16008
TEST=layout tests, media_unittests should pass

[scherkus (not reviewing), 2009-07-10 02:06:26]

Depends on http://codereview.chromium.org/149423

Early layout tests results show that we now pass a few more!

[Alpha Left Google, 2009-07-10 18:13:31]

I just have some nits.

http://codereview.chromium.org/155338/diff/1001/1002
File media/base/pipeline.h (right):

http://codereview.chromium.org/155338/diff/1001/1002#newcode76
Line 76: // the callback has finished executing.
why is it acceptable to call Start multiple starts if Stop is not?

http://codereview.chromium.org/155338/diff/1001/1002#newcode80
Line 80: // TODO(scherkus): it shouldn't be acceptable to call Start() again
after you
I'm referring to this comment in the previous comment.

http://codereview.chromium.org/155338/diff/1001/1002#newcode83
Line 83: virtual void Stop(PipelineCallback* stop_callback) = 0;
so stop is asynchronous?

http://codereview.chromium.org/155338/diff/1001/1002#newcode94
Line 94: virtual bool IsRunning() const = 0;
how is this differs from IsInitialized()?

http://codereview.chromium.org/155338/diff/1001/1003
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/155338/diff/1001/1003#newcode351
Line 351: message_loop()->PostTask(FROM_HERE,
You can just execute InitializeTask() on this thread.

http://codereview.chromium.org/155338/diff/1001/1003#newcode376
Line 376: DCHECK_EQ(PlatformThread::CurrentId(), thread_id_);
DCHECK can be made stronger if you use:
DCHECK(MessageLoop()::current() == message_loop());

Since this can assert our assumption that message_loop has a longer life time
than this object. And if message_loop goes away, MessageLoop()::current() will
be NULL but we may still  have a thread id. You may want to consider changing
all DCHECK to message_loop as this object is not dealing with thread directly.

http://codereview.chromium.org/155338/diff/1001/1004
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/155338/diff/1001/1004#newcode179
Line 179: class PipelineThread : public
base::RefCountedThreadSafe<PipelineThread> {
Since this class is not a real thread anymore, why not call it PipelineInternal?

[scherkus (not reviewing), 2009-07-10 19:25:22]

may want to do a quick review again

I got rid of thread_id_ and did some comment/code cleanup... I might move
PipelineInternal to a separate file

http://codereview.chromium.org/155338/diff/1001/1002
File media/base/pipeline.h (right):

http://codereview.chromium.org/155338/diff/1001/1002#newcode76
Line 76: // the callback has finished executing.
On 2009/07/10 18:13:31, Alpha wrote:
> why is it acceptable to call Start multiple starts if Stop is not?

right now you can call Start() Stop() Start() Stop() on the same object

we're thinking that when you call Stop() the object should be discarded and not
used again

i'll try to fix up the comments

http://codereview.chromium.org/155338/diff/1001/1002#newcode83
Line 83: virtual void Stop(PipelineCallback* stop_callback) = 0;
On 2009/07/10 18:13:31, Alpha wrote:
> so stop is asynchronous?

yes, will fix up comment

http://codereview.chromium.org/155338/diff/1001/1002#newcode94
Line 94: virtual bool IsRunning() const = 0;
On 2009/07/10 18:13:31, Alpha wrote:
> how is this differs from IsInitialized()?

you might have started the pipeline, but initialization failed

the issue I had writing tests was there was no way to tell if you started the
pipeline when initialization failed

http://codereview.chromium.org/155338/diff/1001/1003
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/155338/diff/1001/1003#newcode351
Line 351: message_loop()->PostTask(FROM_HERE,
On 2009/07/10 18:13:31, Alpha wrote:
> You can just execute InitializeTask() on this thread.

Done.

http://codereview.chromium.org/155338/diff/1001/1003#newcode376
Line 376: DCHECK_EQ(PlatformThread::CurrentId(), thread_id_);
On 2009/07/10 18:13:31, Alpha wrote:
> DCHECK can be made stronger if you use:
> DCHECK(MessageLoop()::current() == message_loop());
> 
> Since this can assert our assumption that message_loop has a longer life time
> than this object. And if message_loop goes away, MessageLoop()::current() will
> be NULL but we may still  have a thread id. You may want to consider changing
> all DCHECK to message_loop as this object is not dealing with thread directly.

yeah I used thread ids before because I wasn't sure how MessageLoop::current()
would work with unit tests

turns out it works great because creating a MessageLoop manually (on the stack)
will still set MessageLoop::current(), which is exactly what we want.

Good suggestion!

http://codereview.chromium.org/155338/diff/1001/1004
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/155338/diff/1001/1004#newcode179
Line 179: class PipelineThread : public
base::RefCountedThreadSafe<PipelineThread> {
On 2009/07/10 18:13:31, Alpha wrote:
> Since this class is not a real thread anymore, why not call it
PipelineInternal?

Done.

[Alpha Left Google, 2009-07-10 21:03:00]

LGTM.