Directly call ChunkDemuxer::Initialize completion callback.

media/filters/chunk_demuxer_unittest.cc

Index: media/filters/chunk_demuxer_unittest.cc
diff --git a/media/filters/chunk_demuxer_unittest.cc b/media/filters/chunk_demuxer_unittest.cc
index 0d7c9a1c0d04b668ef4789ac40576f72dcad1290..de21a2e3fe82dc7bfe62e41e12428a4eb10d0a75 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -243,6 +243,10 @@ static void OnSeekDone_OKExpected(bool* called, PipelineStatus status) {
   *called = true;
 }
 
+static void OnPipelineStatus_NOTREACHED(PipelineStatus status) {
+  NOTREACHED();
+}
+
 class ChunkDemuxerTest : public ::testing::Test {
  protected:
   enum CodecsIndex {
@@ -786,10 +790,9 @@ class ChunkDemuxerTest : public ::testing::Test {
     ASSERT_FALSE(AppendData(garbage_cluster.get(), garbage_cluster_size));
   }
 
-  void InitDoneCalled(PipelineStatus expected_status,
-                      PipelineStatus status) {
-    EXPECT_EQ(status, expected_status);
-  }
+  MOCK_METHOD1(DemuxerInitialized, void(PipelineStatus));

[alokp, 2016/09/16 22:10:16]

Converted InitDoneCalled into a mock method so that the sequence of calls can be
verified with testing::InSequence.

Without the changes in ChunkDemuxer, Init and ErrorWhileParsingClusterAfterInit
tests will fail.

+
+  void InitDoneCB(PipelineStatus status) { DemuxerInitialized(status); }
 
   PipelineStatusCB CreateInitDoneCB(const base::TimeDelta& expected_duration,
                                     PipelineStatus expected_status) {
@@ -799,9 +802,8 @@ class ChunkDemuxerTest : public ::testing::Test {
   }
 
   PipelineStatusCB CreateInitDoneCB(PipelineStatus expected_status) {
-    return base::Bind(&ChunkDemuxerTest::InitDoneCalled,
-                      base::Unretained(this),
-                      expected_status);
+    EXPECT_CALL(*this, DemuxerInitialized(expected_status));
+    return base::Bind(&ChunkDemuxerTest::InitDoneCB, base::Unretained(this));
   }
 
   enum StreamFlags {
@@ -1676,9 +1678,7 @@ TEST_F(ChunkDemuxerTest, InitSegmentSetsNeedRandomAccessPointFlag) {
 // is called before all the initialization segments are appended.
 TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppended) {
   EXPECT_CALL(*this, DemuxerOpened());
-  demuxer_->Initialize(
-      &host_, CreateInitDoneCB(
-          kDefaultDuration(), DEMUXER_ERROR_COULD_NOT_OPEN), true);
+  demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true);

[alokp, 2016/09/16 22:10:16]

init_cb should not be called here since the demuxer is shutdown before it is
initialized (which is considered completed only after initialization segment is
fully parsed). This is verified by NOTREACHED.

[xhwang, 2016/09/16 23:41:47]

hmm, we typically don't like outstanding callbacks and require all callbacks to
be fired. So if the demuxer is destroyed before it finishes demuxer init, it
should fire the callback with an error status, rather than just hold the
callback.

[alokp, 2016/09/17 00:06:05]

This patch does not change this behavior. ChunkDemuxer never fired the init_cb
if destroyed before init completes. I had to change the completion callback here
because CreateInitDoneCB now expects InitDoneCB to be called, which ChunkDemuxer
does not. So I made it explicit. If we want to change this behavior, it should
be done in a separate patch.

[xhwang, 2016/09/17 03:58:24]

The demuxer API is a bit old and it's not very clear about whether the callback
should always be fired. I agree such changes (if any) don't belong to this CL.
But then I guess for a demuxer implementation, it's legit to either fire the
callback, or not fire the callback, when it's destructed before init is
finished. With that, I wonder what's the value of asserting that the callback
will not be called in these tests? I kinda feel we are testing implementation
detail, not the API.

Can we just use Bind(&DemuxerInitialized) here but don't expect it to be called?
If we "regress" that the callback is actually fired, we'll get a warning about
uninterested call, but will not fail.

[alokp, 2016/09/17 05:43:14]

I see. I misunderstood your previous comment. I agree about not over-testing.
Done.

 
   EXPECT_EQ(AddId("audio", HAS_AUDIO), ChunkDemuxer::kOk);
   EXPECT_EQ(AddId("video", HAS_VIDEO), ChunkDemuxer::kOk);
@@ -1692,9 +1692,7 @@ TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppended) {
 
 TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppendedText) {
   EXPECT_CALL(*this, DemuxerOpened());
-  demuxer_->Initialize(
-      &host_, CreateInitDoneCB(
-          kDefaultDuration(), DEMUXER_ERROR_COULD_NOT_OPEN), true);
+  demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true);

[alokp, 2016/09/16 22:10:16]

ditto

 
   EXPECT_EQ(AddId("audio", HAS_AUDIO), ChunkDemuxer::kOk);
   EXPECT_EQ(AddId("video_and_text", HAS_VIDEO), ChunkDemuxer::kOk);
@@ -1765,8 +1763,8 @@ TEST_F(ChunkDemuxerTest, AppendDataAfterSeek) {
 
 // Test that parsing errors are handled for clusters appended after init.
 TEST_F(ChunkDemuxerTest, ErrorWhileParsingClusterAfterInit) {
+  InSequence s;

[alokp, 2016/09/16 22:10:16]

Added to verify that init_cb is called before OnDemuxerError. If we add
InSequence without making the changes in DemuxerError, this test will fail.

   ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
-  ASSERT_TRUE(AppendCluster(kDefaultFirstCluster()));
 
   EXPECT_MEDIA_LOG(StreamParsingFailed());
   EXPECT_CALL(host_, OnDemuxerError(CHUNK_DEMUXER_ERROR_APPEND_FAILED));
@@ -3221,7 +3219,7 @@ TEST_F(ChunkDemuxerTest, EndOfStreamStillSetAfterSeek) {
 
 TEST_F(ChunkDemuxerTest, GetBufferedRangesBeforeInitSegment) {
   EXPECT_CALL(*this, DemuxerOpened());
-  demuxer_->Initialize(&host_, CreateInitDoneCB(PIPELINE_OK), true);
+  demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true);
   ASSERT_EQ(AddId("audio", HAS_AUDIO), ChunkDemuxer::kOk);
   ASSERT_EQ(AddId("video", HAS_VIDEO), ChunkDemuxer::kOk);