base: Make it possible to replace the MessageLoop's task runner

base/message_loop/message_loop.h

Index: base/message_loop/message_loop.h
diff --git a/base/message_loop/message_loop.h b/base/message_loop/message_loop.h
index b1b219a2e99d59f5944d9ee0344029820a475452..dee513e77fd86338e7ae1a2397e5e4077b920b8e 100644
--- a/base/message_loop/message_loop.h
+++ b/base/message_loop/message_loop.h
@@ -12,6 +12,7 @@
 #include "base/basictypes.h"
 #include "base/callback_forward.h"
 #include "base/debug/task_annotator.h"
+#include "base/gtest_prod_util.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -37,6 +38,7 @@
 
 namespace base {
 
+class BindableSingleThreadTaskRunner;
 class HistogramBase;
 class RunLoop;
 class ThreadTaskRunnerHandle;
@@ -296,9 +298,17 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   const std::string& thread_name() const { return thread_name_; }
 
   // Gets the TaskRunner associated with this message loop.
-  // TODO(skyostil): Change this to return a const reference to a refptr
-  // once the internal type matches what is being returned (crbug.com/465354).
-  scoped_refptr<SingleThreadTaskRunner> task_runner() { return task_runner_; }
+  scoped_refptr<BindableSingleThreadTaskRunner> task_runner() {

[danakj, 2015/06/29 19:07:23]

What happened to the TODO?

[danakj, 2015/06/29 19:09:05]

Should the fact that this is Bindable really be public? Should this instead
return a STTR?

[alex clarke (OOO till 29th), 2015/06/30 10:51:43]

I agree the fact it's Bindable feels like an implementation detail that ideally
shouldn't be public.

I think we have the following options:

1. Return const scoped_refptr<BindableSingleThreadTaskRunner>&
2. Add a base_task_runner_ as well and return const
scoped_refptr<SingleThreadTaskRunner>& here
3. Regard returning a const reference as micro-optimization and leave it as is,
and remove the todo.

I think I prefer #1 or #3, WDYT?

[alex clarke (OOO till 29th), 2015/06/30 10:51:44]

That was a mistake.  I note it compiles if we change it to:

const scoped_refptr<BindableSingleThreadTaskRunner>& task_runner() {
  return task_runner_;
}

+    return task_runner_;
+  }
+
+  // Sets a new TaskRunner for this message loop and returns the previous one.
+  // If the message loop was already bound to a thread, this function must be
+  // called on that thread and the task runner must also belong to that thread.
+  // Note that changing the task runner will also affect the
+  // ThreadTaskRunnerHandle for the target thread.
+  scoped_refptr<BindableSingleThreadTaskRunner> SwapTaskRunner(
+      scoped_refptr<BindableSingleThreadTaskRunner> task_runner);

[kinuko, 2015/06/30 02:26:55]

Would we want to call this before the message loop's bound to a thread (other
than in test)?  Creating an unbound message loop is a private construction
method, if there're not such cases just asserting that the loop's already bound
might be simpler/clearer?

[alex clarke (OOO till 29th), 2015/06/30 10:51:44]

Apparently yes, see SchedulerMessageLoopDelegate::SetDefaultTaskRunner in
https://codereview.chromium.org/987193002/
I think it would have been cleaner, but it seems the method is needed. 

 
   // Enables or disables the recursive task processing. This happens in the case
   // of recursive message loops. Some unwanted message loop may occurs when
@@ -401,6 +411,7 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   friend class internal::IncomingTaskQueue;
   friend class ScheduleWorkTest;
   friend class Thread;
+  FRIEND_TEST_ALL_PREFIXES(MessageLoopTest, SwapTaskRunnerBeforeBinding);
 
   using MessagePumpFactoryCallback = Callback<scoped_ptr<MessagePump>()>;
 
@@ -415,7 +426,7 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   // thread the message loop runs on, before calling Run().
   // Before BindToCurrentThread() is called only Post*Task() functions can
   // be called on the message loop.
-  scoped_ptr<MessageLoop> CreateUnbound(
+  static scoped_ptr<MessageLoop> CreateUnbound(

[danakj, 2015/06/29 19:07:23]

Can you make this static in a separate patch?

[kinuko, 2015/06/30 02:26:55]

This's already in ToT, you can drop it from this patch (sorry for the
confusion).

[alex clarke (OOO till 29th), 2015/06/30 10:51:43]

Acknowledged.

[alex clarke (OOO till 29th), 2015/06/30 10:51:43]

Acknowledged.

       Type type,
       MessagePumpFactoryCallback pump_factory);
 
@@ -523,7 +534,7 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   scoped_refptr<internal::IncomingTaskQueue> incoming_task_queue_;
 
   // The task runner associated with this message loop.
-  scoped_refptr<internal::MessageLoopTaskRunner> task_runner_;
+  scoped_refptr<BindableSingleThreadTaskRunner> task_runner_;
   scoped_ptr<ThreadTaskRunnerHandle> thread_task_runner_handle_;
 
   template <class T, class R> friend class base::subtle::DeleteHelperInternal;