Remove remaining calls to deprecated MessageLoop methods.

media/test/pipeline_integration_test.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "media/base/cdm_callback_promise.h"
 #include "media/base/cdm_context.h"
 #include "media/base/cdm_key_information.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/media.h"
@@ skipping 755 matching lines @@
 
     pipeline_->Start(demuxer_.get(), CreateRenderer(), this,
                      base::Bind(&PipelineIntegrationTest::OnStatusCallback,
                                 base::Unretained(this)));
 
     if (encrypted_media) {
       source->set_encrypted_media_init_data_cb(
           base::Bind(&FakeEncryptedMedia::OnEncryptedMediaInitData,
                      base::Unretained(encrypted_media)));
     }
-    message_loop_.Run();

[gab, 2016/06/30 20:28:18]

Feels a bit weird to replace a member call by a static call (especially when the
member itself is not fully removed). Should the exposed member be a RunLoop? Or
should it just be a private MessageLoop member in the fixture and the tests use
ThreadTaskRunnerHandle::Get() and base:RunLoop() statically everywhere?


Edit after offline discussion (writing here for posterity):

In tests, we agree that it makes for MessageLoop to be a private member and
everyone use the static methods (TaskRunnerHandle to post / RunLoop to run).
This is similar to TestBrowserThreadBundle's existing paradigm, the MessageLoop
member merely instantiates something to allow tests that need to post/run tasks
to do so. Using static methods is better because that member could easily be
swapped out for something else that also supports these static methods (e.g. an
eventual TestTaskScheduler).

Changing all tests at scale to use this paradigm is a huge undertaking and isn't
easily automated. As such, replacing the member Run() call with
base::RunLoop()::Run() is acceptable (it's actually the same thing in practice,
MessageLoop::Run() used to do just that and not actually guarantee to run the
loop of the actual instance..!) as it at least allows the removal of
MessageLoop::Run() and will force future tests to use the aforementioned
paradigm.

Where would be a good place in the codebase to document the recommended
paradigm?

[fdoray, 2016/07/04 13:59:46]

MessageLoop::Run() and MessageLoop::RunUntilIdle() will be removed so everybody
will be forced to use RunLoop.

We should add a comment on MessageLoop::task_runner() to say that
ThreadTaskRunnerHandle::Get() must be used instead of
MessageLoop::current()->task_runner().

Could we add a presubmit check to make sure that MessageLoop is always private
in tests? I don't see a good reason to make it public/protected:
- ThreadTaskRunnerHandle should be used to post tasks.
- RunLoop should be used to run the MessageLoop.
- Special methods (AddTaskObserver, AddDestructionObserver,
WatchFileDescriptor...) are not thread-safe. MessageLoop::current()->... should
be used to call these methods.

[gab, 2016/07/04 14:47:06]

Hmm I don't think there's precedent for such a presubmit check (ensuring a
specific section of class definition is used for a member), but maybe it could
make sense. Only if the desired paradigm is documented somewhere in the codebase
and this is merely intended to point users who missed that documentation back to
it.

(definitely as a follow-up CL, not in this one, though; just to make sure we're
on the same page)

[fdoray, 2016/07/04 16:33:59]

Will document the desired paradigm in a separate CL.

+    base::RunLoop().Run();
     return pipeline_status_;
   }
 
   // Verifies that seeking works properly for ChunkDemuxer when the
   // seek happens while there is a pending read on the ChunkDemuxer
   // and no data is available.
   bool TestSeekDuringRead(const std::string& filename,
                           const std::string& mimetype,
                           int initial_append_size,
                           base::TimeDelta start_seek_time,
@@ skipping 440 matching lines @@
   MockMediaSource source(input_filename, kWebM, kAppendWholeFile);
   EXPECT_EQ(PIPELINE_OK, StartPipelineWithMediaSource(&source));
 
   auto buffered_ranges = pipeline_->GetBufferedTimeRanges();
   EXPECT_EQ(1u, buffered_ranges.size());
   EXPECT_EQ(0, buffered_ranges.start(0).InMilliseconds());
   EXPECT_EQ(k320WebMFileDurationMs, buffered_ranges.end(0).InMilliseconds());
 
   source.RemoveRange(base::TimeDelta::FromMilliseconds(1000),
                      base::TimeDelta::FromMilliseconds(k320WebMFileDurationMs));
-  message_loop_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
 
   buffered_ranges = pipeline_->GetBufferedTimeRanges();
   EXPECT_EQ(1u, buffered_ranges.size());
   EXPECT_EQ(0, buffered_ranges.start(0).InMilliseconds());
   EXPECT_EQ(1001, buffered_ranges.end(0).InMilliseconds());
 
   source.Shutdown();
   Stop();
 }
 
@@ skipping 15 matching lines @@
   EXPECT_EQ(1u, buffered_ranges.size());
   do {
     // Advance media_time to the end of the currently buffered data
     base::TimeDelta media_time = buffered_ranges.end(0);
     source.Seek(media_time);
     // Ask MediaSource to evict buffered data if buffering limit has been
     // reached (the data will be evicted from the front of the buffered range).
     source.EvictCodedFrames(media_time, file->data_size());
     ASSERT_TRUE(
         source.AppendAtTime(media_time, file->data(), file->data_size()));
-    message_loop_.RunUntilIdle();
+    base::RunLoop().RunUntilIdle();
 
     buffered_ranges = pipeline_->GetBufferedTimeRanges();
   } while (buffered_ranges.size() == 1 &&
            buffered_ranges.start(0) == base::TimeDelta::FromSeconds(0));
 
   EXPECT_EQ(1u, buffered_ranges.size());
   source.Shutdown();
   Stop();
 }
 
@@ skipping 36 matching lines @@
 
   scoped_refptr<DecoderBuffer> second_file =
       ReadTestDataFile("bear-640x360-av_enc-av.webm");
 
   ASSERT_FALSE(source.AppendAtTime(base::TimeDelta::FromSeconds(kAppendTimeSec),
                                    second_file->data(),
                                    second_file->data_size()));
 
   source.EndOfStream();
 
-  message_loop_.Run();
+  base::RunLoop().Run();
   EXPECT_EQ(CHUNK_DEMUXER_ERROR_APPEND_FAILED, pipeline_status_);
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());
   EXPECT_EQ(0, pipeline_->GetBufferedTimeRanges().start(0).InMilliseconds());
   // The second video was not added, so its time has not been added.
   EXPECT_EQ(k320WebMFileDurationMs,
             pipeline_->GetBufferedTimeRanges().end(0).InMilliseconds());
 
   Play();
 
@@ skipping 964 matching lines @@
 
 TEST_F(PipelineIntegrationTest, BasicPlaybackPositiveStartTime) {
   ASSERT_EQ(PIPELINE_OK, Start("nonzero-start-time.webm"));
   Play();
   ASSERT_TRUE(WaitUntilOnEnded());
   ASSERT_EQ(base::TimeDelta::FromMicroseconds(396000),
             demuxer_->GetStartTime());
 }
 
 }  // namespace media