Remove remaining calls to deprecated MessageLoop methods.

media/test/pipeline_integration_test.cc

Index: media/test/pipeline_integration_test.cc
diff --git a/media/test/pipeline_integration_test.cc b/media/test/pipeline_integration_test.cc
index 2cd26bd8431ad268ee157359f97459347b75dfd3..7ca3894ca7f839bcc66779e4fc654d25b9de1701 100644
--- a/media/test/pipeline_integration_test.cc
+++ b/media/test/pipeline_integration_test.cc
@@ -14,6 +14,7 @@
 #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"
@@ -789,7 +790,7 @@ class PipelineIntegrationTest : public PipelineIntegrationTestHost {
           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_;
   }
 
@@ -1250,7 +1251,7 @@ TEST_F(PipelineIntegrationTest, MediaSource_Remove_Updates_BufferedRanges) {
 
   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());
@@ -1286,7 +1287,7 @@ TEST_F(PipelineIntegrationTest, MediaSource_FillUp_Buffer) {
     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 &&
@@ -1343,7 +1344,7 @@ TEST_F(PipelineIntegrationTest,
 
   source.EndOfStream();
 
-  message_loop_.Run();
+  base::RunLoop().Run();
   EXPECT_EQ(CHUNK_DEMUXER_ERROR_APPEND_FAILED, pipeline_status_);
 
   EXPECT_EQ(1u, pipeline_->GetBufferedTimeRanges().size());