Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more.

base/run_loop.h

Index: base/run_loop.h
diff --git a/base/run_loop.h b/base/run_loop.h
index 12976fce5a38acd0adf36d64868f2db2d64d2fb4..8bc811be3c41b79e764cdbdc9dfe5320be5452e7 100644
--- a/base/run_loop.h
+++ b/base/run_loop.h
@@ -5,11 +5,14 @@
 #ifndef BASE_RUN_LOOP_H_
 #define BASE_RUN_LOOP_H_
 
+#include <stack>
+#include <vector>
+
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/message_loop/message_loop.h"
+#include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
 #include "build/build_config.h"
 
@@ -22,25 +25,28 @@ class MessagePumpForUI;
 class MessagePumpUIApplication;
 #endif
 
-// Helper class to Run a nested MessageLoop. Please do not use nested
-// MessageLoops in production code! If you must, use this class instead of
-// calling MessageLoop::Run/Quit directly. RunLoop::Run can only be called once
-// per RunLoop lifetime. Create a RunLoop on the stack and call Run/Quit to run
-// a nested MessageLoop.
+// Helper class to run the RunLoop::Delegate associated with the current thread.
+// A RunLoop::Delegate must have been bound to this thread (ref.
+// RunLoop::Delegate::BindToCurrentThread()) prior to using any of RunLoop's
+// member and static methods. RunLoop::Run can only be called once per RunLoop
+// lifetime. Create a RunLoop on the stack and call Run/Quit to run a nested
+// RunLoop but please do not use nested loops in production code!
 class BASE_EXPORT RunLoop {
  public:
   RunLoop();
   ~RunLoop();
 
-  // Run the current MessageLoop. This blocks until Quit is called. Before
+  // Run the current RunLoop::Delegate. This blocks until Quit is called. Before
   // calling Run, be sure to grab the QuitClosure in order to stop the
-  // MessageLoop asynchronously. MessageLoop::QuitWhenIdle and QuitNow will also
-  // trigger a return from Run, but those are deprecated.
+  // RunLoop::Delegate asynchronously. MessageLoop::QuitWhenIdle and QuitNow
+  // will also trigger a return from Run (if RunLoop::Delegate happens to be a
+  // MessageLoop...), but those are deprecated.
   void Run();
 
-  // Run the current MessageLoop until it doesn't find any tasks or messages in
-  // the queue (it goes idle). WARNING: This may never return! Only use this
-  // when repeating tasks such as animated web pages have been shut down.
+  // Run the current RunLoop::Delegate until it doesn't find any tasks or
+  // messages in its queue (it goes idle). WARNING: This may never return! Only
+  // use this when repeating tasks such as animated web pages have been shut
+  // down.
   void RunUntilIdle();
 
   bool running() const {
@@ -76,11 +82,6 @@ class BASE_EXPORT RunLoop {
   base::Closure QuitClosure();
   base::Closure QuitWhenIdleClosure();
 
-  // Cleans pre-existing TLS state.
-  // TODO(gab): Remove this in favor of managing TLS through RunLoop::Delegate
-  // as part of the RunLoop<=>MessageLoop split in http://crbug.com/703346.
-  static void ResetTLSState();
-
   // Returns true if there is an active RunLoop on this thread.
   static bool IsRunningOnCurrentThread();
 
@@ -109,7 +110,69 @@ class BASE_EXPORT RunLoop {
   // Add/RemoveNestingObserverOnCurrentThread() on this thread will crash.
   static void DisallowNestingOnCurrentThread();
 
+  // A RunLoop::Delegate is a generic interface that allows RunLoop to be
+  // separate from the uderlying implementation of the message loop for this
+  // thread, it has two functions:
+  //   1) it implements a private API used by RunLoop to ask the Delegate to
+  //      run/quit.
+  //   2) it holds thread-local state used by every RunLoop on its associated
+  //      thread. The advantage of having that state stored on the Delegate
+  //      itself instead of in a TLS slot in run_loop.cc is that it guarantees
+  //      it's cleaned up when the RunLoop::Delegate is destroyed (required in
+  //      unittests).
+  class BASE_EXPORT Delegate {

[danakj, 2017/05/15 16:23:13]

While its less # of classes to do this, this Delegate is combining two things
into one which I feel is awkward:
1. Operations that RunLoop wants to delegate to another class (Run/Quit).
2. Private APIs for the delegate to use from RunLoop.

IOW half the methods are for RunLoop to use and half are for the Delegate to
use.

What if we split them and did something like

class Delegate {
 Init(Private*)
 Run() = 0;
 Quit() = 0;
}

class Private {
 GetTopMostRunLoop() { ... }
 IsNested() { ... }
}

etc..

This also I think gets us more like having interface (abstract APIs) separate
from implementations which has a better code smell to me.

wdyt?

[gab, 2017/05/15 17:28:35]

Hmmm, I don't see how this would work, what is MessageLoop implementing?

Are you okay with the fact that the Delegate holds state that is really meant to
be used by RunLoop? I agree it's a bit of a weird indirection but it's the best
ownership model I could think of. The state is really all on the Delegate (only
one per thread). RunLoop instances are merely dedicated drivers of that state.

As for the protected Delegate methods that allow MessageLoop to poke back into
that private RunLoop state which it owns but doesn't have access to...
IsNested() could be implemented MessageLoop side, it's just that we already the
information so poking at it directly seemed simpler. GetTopMostRunLoop() will go
away as I split this even further.

+   protected:
+    Delegate();
+    ~Delegate();
+
+    // Returns the RunLoop with the topmost active Run() call on the stack.
+    // TODO(gab): Break the inter-dependency between MessageLoop and RunLoop
+    // further. http://crbug.com/703346
+    RunLoop* GetTopMostRunLoop() const;
+
+    // Returns true if this Delegate is currently in nested runs.
+    // Code that needs to query RunLoop::IsNestedOnCurrentThread() frequently
+    // may want to consider storing RunLoop::Delegate::current() in a variable
+    // and accessing the value through this method instead.
+    bool IsNested() const;
+
+    // Binds this Delegate to the current thread. After this point, RunLoops may
+    // be instantiated on this thread and they will be associated to this
+    // Delegate. It will bound unbound in its destructor (which must occur on
+    // the same thread this call was made).
+    virtual void BindToCurrentThread();
+
+   private:
+    friend class RunLoop;

[danakj, 2017/05/15 16:23:13]

This isn't needed since the class is nested right?

[gab, 2017/05/15 17:28:35]

In Java you're right I think but in C++ it's required.

+
+    // Used by RunLoop to inform its Delegate to Run/Quit. Implementations are
+    // expected to keep on running synchronously from the Run() call until the
+    // eventual matching Quit() call. Upon receiving a Quit() call it should
+    // return from the Run() call as soon as possible without executing
+    // remaining tasks/messages. Run() calls can nest in which case each Quit()
+    // call should result in the topmost active Run() call returning.
+    virtual void Run() = 0;
+    virtual void Quit() = 0;
+
+    // A vector-based stack is more memory efficient than the default deque-
+    // based stack as the active RunLoop stack isn't expected to ever have more
+    // than a few entries.
+    using RunLoopStack = std::stack<RunLoop*, std::vector<RunLoop*>>;
+
+    bool allow_nesting_ = true;
+    RunLoopStack active_run_loops_;
+    ObserverList<RunLoop::NestingObserver> nesting_observers_;
+
+    bool bound_ = false;
+
+    THREAD_CHECKER(bound_thread_checker_);
+
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
+  };
+
  private:
+  // TODO(gab): Break the inter-dependency between MessageLoop and RunLoop
+  // further. http://crbug.com/703346
   friend class MessageLoop;
 #if defined(OS_ANDROID)
   // Android doesn't support the blocking MessageLoop::Run, so it calls
@@ -127,7 +190,9 @@ class BASE_EXPORT RunLoop {
   bool BeforeRun();
   void AfterRun();
 
-  MessageLoop* loop_;
+  // A copy of RunLoop::Delegate for this thread for quick access without using
+  // TLS.
+  Delegate* delegate_;
 
   bool run_called_ = false;
   bool quit_called_ = false;