Merge Pipeline's kError state with kStopped: a baby step towards bringing sanity to shutdown.

Merge Pipeline's kError state with kStopped: a baby step towards bringing sanity to shutdown.

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

[scherkus (not reviewing), 2012-08-09 19:26:53]

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

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (left):

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#oldc...
media/base/pipeline.cc:75: error_caused_teardown_(false),
this is synonymous with (status_ != PIPELINE_OK && tearing_down_)

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (left):

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:360: EXPECT_CALL(*mocks_->demuxer(), Stop(_))
covered by pipeline_->IsRunning() check in dtor

[scherkus (not reviewing), 2012-08-09 19:28:08]

(this is the somewhat riskier change, but I ran it through the gauntlet of tests
on my machine and it seems to check out just fine!)

[Ami GONE FROM CHROMIUM, 2012-08-09 20:55:41]

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

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:695: // like the error never occurred.
This seems crazy.   Why do we want this?

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:736: if (!running_ || tearing_down_)
This (and other places) read running_ not under lock.  Is that safe?

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:878: // No reason transitioning if we're stopped or
tearing down as teardown
English this up.

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.h#newco...
media/base/pipeline.h:123: // call Stop() multiple times.
why?

http://codereview.chromium.org/10825280/diff/2001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10825280/diff/2001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:117: filter_collection_(collection),
remove started_ from wmpi.h?

[scherkus (not reviewing), 2012-08-09 22:07:49]

let me know if you can't view any of the files -- it seems to work for me but
git cl is saying "base files missing" (?!)

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

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:695: // like the error never occurred.
On 2012/08/09 20:55:41, Ami Fischman wrote:
> This seems crazy.   Why do we want this?

It _is_ a bit crazy but not _that_ crazy -- it basically suppresses the error_cb
from firing since we're trying to stop_cb

Check out my original brainstorming CL on this matter -- it's an open question
as to which CBs should fire:
http://codereview.chromium.org/10828045/diff/7/media/base/pipeline.cc#newcode655

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:736: if (!running_ || tearing_down_)
On 2012/08/09 20:55:41, Ami Fischman wrote:
> This (and other places) read running_ not under lock.  Is that safe?

probably not!

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:878: // No reason transitioning if we're stopped or
tearing down as teardown
On 2012/08/09 20:55:41, Ami Fischman wrote:
> English this up.

Done.

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10825280/diff/2001/media/base/pipeline.h#newco...
media/base/pipeline.h:123: // call Stop() multiple times.
On 2012/08/09 20:55:41, Ami Fischman wrote:
> why?

gah my wording here was bad -- it's more about being able to call Stop() at any
time after a Pipeline has been created rather than calling Stop() a bunch of
times (this way clients such as WMPI don't need to externally track whether
they've called Start())

http://codereview.chromium.org/10825280/diff/2001/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10825280/diff/2001/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:117: filter_collection_(collection),
On 2012/08/09 20:55:41, Ami Fischman wrote:
> remove started_ from wmpi.h?

Done.

[Ami GONE FROM CHROMIUM, 2012-08-09 22:14:19]

lgtm

http://codereview.chromium.org/10825280/diff/12001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10825280/diff/12001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:140: pipeline_ = new
media::Pipeline(pipeline_message_loop, media_log_);
This could be in the initializer list just as well.
(and the member could be a straight-up Pipeline instead of a scoped_ptr<>).

[scherkus (not reviewing), 2012-08-09 22:18:33]

http://codereview.chromium.org/10825280/diff/12001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10825280/diff/12001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:140: pipeline_ = new
media::Pipeline(pipeline_message_loop, media_log_);
On 2012/08/09 22:14:19, Ami Fischman wrote:
> This could be in the initializer list just as well.
> (and the member could be a straight-up Pipeline instead of a scoped_ptr<>).

it's ref-counted!