Replace Pipeline::kEnded state and HasEnded() methods with renderer-specific bools.

Replace Pipeline::kEnded state and HasEnded() methods with renderer-specific bools.

The kEnded state was used to protect against the case where both renderers executed their ended callbacks at the same time, resulting in two tasks posted to the pipeline message loop but the first task would poll each renderer's HasEnded() method and execute the ended logic before the second task executed.

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

[scherkus (not reviewing), 2012-08-08 20:59:58]

A remix of https://chromiumcodereview.appspot.com/10855041/ where I delete more
code but introduce some bools!

https://chromiumcodereview.appspot.com/10834236/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10834236/diff/1/media/base/pipeline.cc...
media/base/pipeline.cc:826: audio_ended_ = false;
FYI I think this is the only spot where we need to reset these but worth sanity
checking

[Ami GONE FROM CHROMIUM, 2012-08-08 21:48:26]

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

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline.cc#newcode535
media/base/pipeline.cc:535:
media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::ENDED));
Why is it useful to media_log this twice?
Alternatively, should each event be tagged w/ an audio/video  bit?

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

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline.h#newcode81
media/base/pipeline.h:81: //         V      Seek()/Stop()                       
   |
why no mention of the On*Ended() callbacks?

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:622: // The ended callback shouldn't run until
both renderers have ended.
Does this test fail if you move the EXPECT_CALL in the line below either up a
line or down a line?

[scherkus (not reviewing), 2012-08-08 22:28:34]

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

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline.cc#newcode535
media/base/pipeline.cc:535:
media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::ENDED));
On 2012/08/08 21:48:26, Ami Fischman wrote:
> Why is it useful to media_log this twice?
> Alternatively, should each event be tagged w/ an audio/video  bit?

Done by dup'ing the the events (I find adding a param to an event doesn't
represent itself as nicely inside chrome://media-internals)

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

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline.h#newcode81
media/base/pipeline.h:81: //         V      Seek()/Stop()                       
   |
On 2012/08/08 21:48:26, Ami Fischman wrote:
> why no mention of the On*Ended() callbacks?

Not sure if they'd be helpful / how I could fit them in.

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline_unittest.cc
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10834236/diff/1/media/base/pipeline_unittest.c...
media/base/pipeline_unittest.cc:622: // The ended callback shouldn't run until
both renderers have ended.
On 2012/08/08 21:48:26, Ami Fischman wrote:
> Does this test fail if you move the EXPECT_CALL in the line below either up a
> line or down a line?

Fails if you move it down a line but not up. Testing that something isn't called
at the right time is typically accomplished by use of InSequence. I prefer not
setting the expectation myself!

I did have to add message loop runs to make sure the ended tasks did get
executed.

[Ami GONE FROM CHROMIUM, 2012-08-08 23:45:46]

lgtm

[acolwell GONE FROM CHROMIUM, 2012-08-08 23:59:55]

LGTM

[commit-bot: I haz the power, 2012-08-09 00:52:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10834236/14

[commit-bot: I haz the power, 2012-08-09 02:08:13]

Try job failure for 10834236-14 (retry) on linux_rel for step
"nacl_integration".
It's a second try, previously, steps "nacl_integration, interactive_ui_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...