Replace RunLoop's Delegate::Client by a ShouldQuitWhenIdleCallback.

The Client interface was diluted down to a single method at this point.
Different implementations of overriding Delegate's Run() methods will
need different behaviors from ShouldQuitWhenIdle(). Making it easily
overrridable is key.

One such example behaviour is when overriding a MessageLoopForUI/IO.
When waiting inside a Run() with no more tasks, control needs to
remain in the hands of the overridden MessageLoop as it may receive
work first (from the system) and therefore shouldn't quit-when-idle
when the overriding Delegate is out of work (the overriding Delegate
can let it know to wake by posting a task to it if it gets work
first).

R=thestig@chromium.org

Bug: 708584
Change-Id: I27a449bc3be5858b0e8d4d6482714523ad5e2b67
Reviewed-on: https://chromium-review.googlesource.com/817962
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524097}

base/message_loop/message_loop.cc

@@ -303,7 +303,7 @@ void MessageLoop::BindToCurrentThread() {
       internal::ScopedSetSequenceLocalStorageMapForCurrentThread>(
       &sequence_local_storage_map_);
 
-  run_loop_client_ = RunLoop::RegisterDelegateForCurrentThread(this);
+  RunLoop::RegisterDelegateForCurrentThread(this);
 }
 
 std::string MessageLoop::GetThreadName() const {
@@ -491,7 +491,7 @@ bool MessageLoop::DoIdleWork() {
   if (ProcessNextDelayedNonNestableTask())
     return true;
 
-  if (run_loop_client_->ShouldQuitWhenIdle())
+  if (ShouldQuitWhenIdle())
     pump_->Quit();
 
   // When we return we will do a kernel wait for more tasks.

base/message_loop/message_loop.h

@@ -398,9 +398,6 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate,
   // Whether task observers are allowed.
   bool allow_task_observers_ = true;
 
-  // An interface back to RunLoop state accessible by this RunLoop::Delegate.
-  RunLoop::Delegate::Client* run_loop_client_ = nullptr;
-
   // Holds data stored through the SequenceLocalStorageSlot API.
   internal::SequenceLocalStorageMap sequence_local_storage_map_;
 

base/run_loop.cc

@@ -32,7 +32,12 @@ void ProxyToTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner,
 
 }  // namespace
 
-RunLoop::Delegate::Delegate() {
+RunLoop::Delegate::Delegate()
+    : should_quit_when_idle_callback_(base::BindRepeating(
+          [](Delegate* self) {
+            return self->active_run_loops_.top()->quit_when_idle_received_;
+          },
+          Unretained(this))) {
   // The Delegate can be created on another thread. It is only bound in
   // RegisterDelegateForCurrentThread().
   DETACH_FROM_THREAD(bound_thread_checker_);
@@ -47,18 +52,12 @@ RunLoop::Delegate::~Delegate() {
     tls_delegate.Get().Set(nullptr);
 }
 
-bool RunLoop::Delegate::Client::ShouldQuitWhenIdle() const {
-  DCHECK_CALLED_ON_VALID_THREAD(outer_->bound_thread_checker_);
-  DCHECK(outer_->bound_);
-  return outer_->was_overriden_ ||
-         outer_->active_run_loops_.top()->quit_when_idle_received_;
+bool RunLoop::Delegate::ShouldQuitWhenIdle() {
+  return should_quit_when_idle_callback_.Run();
 }
 
-RunLoop::Delegate::Client::Client(Delegate* outer) : outer_(outer) {}
-
 // static
-RunLoop::Delegate::Client* RunLoop::RegisterDelegateForCurrentThread(
-    Delegate* delegate) {
+void RunLoop::RegisterDelegateForCurrentThread(Delegate* delegate) {
   // Bind |delegate| to this thread.
   DCHECK(!delegate->bound_);
   DCHECK_CALLED_ON_VALID_THREAD(delegate->bound_thread_checker_);
@@ -67,13 +66,13 @@ RunLoop::Delegate::Client* RunLoop::RegisterDelegateForCurrentThread(
   DCHECK(!tls_delegate.Get().Get());
   tls_delegate.Get().Set(delegate);
   delegate->bound_ = true;
-
-  return &delegate->client_interface_;
 }
 
 // static
-std::pair<RunLoop::Delegate::Client*, RunLoop::Delegate*>
-RunLoop::OverrideDelegateForCurrentThreadForTesting(Delegate* delegate) {
+RunLoop::Delegate* RunLoop::OverrideDelegateForCurrentThreadForTesting(
+    Delegate* delegate,
+    Delegate::ShouldQuitWhenIdleCallback
+        overriding_should_quit_when_idle_callback) {
   // Bind |delegate| to this thread.
   DCHECK(!delegate->bound_);
   DCHECK_CALLED_ON_VALID_THREAD(delegate->bound_thread_checker_);
@@ -82,14 +81,15 @@ RunLoop::OverrideDelegateForCurrentThreadForTesting(Delegate* delegate) {
   DCHECK(!IsRunningOnCurrentThread());
 
   // Override the current Delegate (there must be one).
-  Delegate* overriden_delegate = tls_delegate.Get().Get();
-  DCHECK(overriden_delegate);
-  DCHECK(overriden_delegate->bound_);
-  overriden_delegate->was_overriden_ = true;
+  Delegate* overridden_delegate = tls_delegate.Get().Get();
+  DCHECK(overridden_delegate);
+  DCHECK(overridden_delegate->bound_);
+  overridden_delegate->should_quit_when_idle_callback_ =
+      std::move(overriding_should_quit_when_idle_callback);
   tls_delegate.Get().Set(delegate);
   delegate->bound_ = true;
 
-  return std::make_pair(&delegate->client_interface_, overriden_delegate);
+  return overridden_delegate;
 }
 
 RunLoop::RunLoop(Type type)

base/run_loop.h

@@ -152,46 +152,33 @@ class BASE_EXPORT RunLoop {
   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
+  // separate from the underlying implementation of the message loop for this
   // thread. It holds private state used by RunLoops on its associated thread.
   // One and only one RunLoop::Delegate must be registered on a given thread
   // via RunLoop::RegisterDelegateForCurrentThread() before RunLoop instances
   // and RunLoop static methods can be used on it.
   class BASE_EXPORT Delegate {
    public:
+    // A Callback which returns true if the Delegate should return from the
+    // topmost Run() when it becomes idle. The Delegate is responsible for
+    // probing this when it becomes idle.
+    using ShouldQuitWhenIdleCallback = RepeatingCallback<bool(void)>;
+
     Delegate();
     virtual ~Delegate();
 
-    // The client interface provided back to the caller who registers this
-    // Delegate via RegisterDelegateForCurrentThread() or
-    // OverrideDelegateForCurrentThreadForTesting().
-    class BASE_EXPORT Client {
-     public:
-      // Returns true if the Delegate should return from the topmost Run() when
-      // it becomes idle. The Delegate is responsible for probing this when it
-      // becomes idle.
-      bool ShouldQuitWhenIdle() const;
-
-     private:
-      // Only a Delegate can instantiate a Delegate::Client.
-      friend class Delegate;
-      Client(Delegate* outer);
-
-      Delegate* outer_;
-    };
-
     // 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. The only
-    // other trigger for Run() to return is Client::ShouldQuitWhenIdle() which
-    // the Delegate should probe before sleeping when it becomes idle.
-    // |application_tasks_allowed| is true if this is the first Run() call on
-    // the stack or it was made from a nested RunLoop of
-    // Type::kNestableTasksAllowed (otherwise this Run() level should only
-    // process system tasks).
+    // other trigger for Run() to return is the
+    // |should_quit_when_idle_callback_| which the Delegate should probe before
+    // sleeping when it becomes idle. |application_tasks_allowed| is true if
+    // this is the first Run() call on the stack or it was made from a nested
+    // RunLoop of Type::kNestableTasksAllowed (otherwise this Run() level should
+    // only process system tasks).
     virtual void Run(bool application_tasks_allowed) = 0;
     virtual void Quit() = 0;
 
@@ -204,6 +191,11 @@ class BASE_EXPORT RunLoop {
     // system messages.
     virtual void EnsureWorkScheduled() = 0;
 
+   protected:
+    // Returns the result of this Delegate's |should_quit_when_idle_callback_|.
+    // "protected" so it can be invoked only by the Delegate itself.
+    bool ShouldQuitWhenIdle();
+
    private:
     // While the state is owned by the Delegate subclass, only RunLoop can use
     // it.
@@ -218,12 +210,6 @@ class BASE_EXPORT RunLoop {
     RunLoopStack active_run_loops_;
     ObserverList<RunLoop::NestingObserver> nesting_observers_;
 
-    // True if this Delegate was overriden through
-    // OverrideDelegateForCurrentThreadForTesting(). It will from then on always
-    // return from Run() when idle (i.e. its Client's ShouldQuitWhenIdle() will
-    // always be true).
-    bool was_overriden_ = false;
-
 #if DCHECK_IS_ON()
     bool allow_running_for_testing_ = true;
 #endif
@@ -232,28 +218,28 @@ class BASE_EXPORT RunLoop {
     // RegisterDelegateForCurrentThread().
     bool bound_ = false;
 
+    ShouldQuitWhenIdleCallback should_quit_when_idle_callback_;
+
     // Thread-affine per its use of TLS.
     THREAD_CHECKER(bound_thread_checker_);
 
-    Client client_interface_ = Client(this);
-
     DISALLOW_COPY_AND_ASSIGN(Delegate);
   };
 
   // Registers |delegate| on the current thread. Must be called once and only
   // once per thread before using RunLoop methods on it. |delegate| is from then
-  // on forever bound to that thread (including its destruction). The returned
-  // Delegate::Client is valid as long as |delegate| is kept alive.
-  static Delegate::Client* RegisterDelegateForCurrentThread(Delegate* delegate);
+  // on forever bound to that thread (including its destruction).
+  static void RegisterDelegateForCurrentThread(Delegate* delegate);
 
   // Akin to RegisterDelegateForCurrentThread but overrides an existing Delegate
-  // (there must be one). Returning |delegate|'s client interface like
-  // RegisterDelegateForCurrentThread() as well as the overriden Delegate which
-  // the caller is now in charge of driving. The overriden Delegate's Run()
-  // method will always return when idle (or earlier if the Delegate's Quit()
-  // method is explicitly invoked).
-  static std::pair<Delegate::Client*, Delegate*>
-  OverrideDelegateForCurrentThreadForTesting(Delegate* delegate);
+  // (there must be one). Returning the overridden Delegate which the caller is
+  // now in charge of driving. |override_should_quit_when_idle_callback|
+  // specifies will replace the overridden Delegate's
+  // |should_quit_when_idle_callback_|, giving full control to |delegate|.
+  static Delegate* OverrideDelegateForCurrentThreadForTesting(
+      Delegate* delegate,
+      Delegate::ShouldQuitWhenIdleCallback
+          overriding_should_quit_when_idle_callback);
 
   // Quits the active RunLoop (when idle) -- there must be one. These were
   // introduced as prefered temporary replacements to the long deprecated
@@ -318,9 +304,8 @@ class BASE_EXPORT RunLoop {
   bool running_ = false;
   // Used to record that QuitWhenIdle() was called on this RunLoop, meaning that
   // the Delegate should quit Run() once it becomes idle (it's responsible for
-  // probing this state via Client::ShouldQuitWhenIdle()). This state is stored
-  // here rather than pushed to Delegate via, e.g., Delegate::QuitWhenIdle() to
-  // support nested RunLoops.
+  // probing this state via ShouldQuitWhenIdle()). This state is stored here
+  // rather than pushed to Delegate to support nested RunLoops.
   bool quit_when_idle_received_ = false;
 
   // RunLoop is not thread-safe. Its state/methods, unless marked as such, may

base/run_loop_unittest.cc

@@ -158,10 +158,9 @@ class TestBoundDelegate final : public InjectableTestDelegate {
   // Makes this TestBoundDelegate become the RunLoop::Delegate and
   // ThreadTaskRunnerHandle for this thread.
   void BindToCurrentThread() {
-    DCHECK(!run_loop_client_);
     thread_task_runner_handle_ =
         std::make_unique<ThreadTaskRunnerHandle>(simple_task_runner_);
-    run_loop_client_ = RunLoop::RegisterDelegateForCurrentThread(this);
+    RunLoop::RegisterDelegateForCurrentThread(this);
   }
 
  private:
@@ -178,7 +177,7 @@ class TestBoundDelegate final : public InjectableTestDelegate {
       if (application_tasks_allowed && simple_task_runner_->ProcessSingleTask())
         continue;
 
-      if (run_loop_client_->ShouldQuitWhenIdle())
+      if (ShouldQuitWhenIdle())
         break;
 
       if (RunInjectedClosure())
@@ -205,8 +204,6 @@ class TestBoundDelegate final : public InjectableTestDelegate {
   std::unique_ptr<ThreadTaskRunnerHandle> thread_task_runner_handle_;
 
   bool should_quit_ = false;
-
-  RunLoop::Delegate::Client* run_loop_client_ = nullptr;
 };
 
 // A test RunLoop::Delegate meant to override an existing RunLoop::Delegate.
@@ -219,20 +216,19 @@ class TestOverridingDelegate final : public InjectableTestDelegate {
   // Overrides the existing RunLoop::Delegate and ThreadTaskRunnerHandles on
   // this thread with this TestOverridingDelegate's.
   void TakeOverCurrentThread() {
-    DCHECK(!run_loop_client_);
-
-    overriden_task_runner_ = ThreadTaskRunnerHandle::Get();
-    DCHECK(overriden_task_runner_);
+    overridden_task_runner_ = ThreadTaskRunnerHandle::Get();
+    ASSERT_TRUE(overridden_task_runner_);
     thread_task_runner_handle_override_scope_ =
         ThreadTaskRunnerHandle::OverrideForTesting(
             simple_task_runner_,
             ThreadTaskRunnerHandle::OverrideType::kTakeOverThread);
 
-    auto delegate_override_pair =
-        RunLoop::OverrideDelegateForCurrentThreadForTesting(this);
-    run_loop_client_ = delegate_override_pair.first;
-    overriden_delegate_ = delegate_override_pair.second;
-    DCHECK(overriden_delegate_);
+    // TestOverridingDelegate::Run() is designed with the assumption that the
+    // overridden Delegate's Run() always returns control to it when it becomes
+    // idle.
+    overridden_delegate_ = RunLoop::OverrideDelegateForCurrentThreadForTesting(
+        this, base::BindRepeating([]() { return true; }));
+    ASSERT_TRUE(overridden_delegate_);
   }
 
  private:
@@ -241,15 +237,15 @@ class TestOverridingDelegate final : public InjectableTestDelegate {
       auto pending_tasks = simple_task_runner_->TakePendingTasks();
       if (!pending_tasks.empty()) {
         while (!pending_tasks.empty()) {
-          overriden_task_runner_->PostTask(FROM_HERE,
-                                           std::move(pending_tasks.front()));
+          overridden_task_runner_->PostTask(FROM_HERE,
+                                            std::move(pending_tasks.front()));
           pending_tasks.pop();
         }
-        overriden_delegate_->Run(application_tasks_allowed);
+        overridden_delegate_->Run(application_tasks_allowed);
         continue;
       }
 
-      if (run_loop_client_->ShouldQuitWhenIdle())
+      if (ShouldQuitWhenIdle())
         break;
 
       if (RunInjectedClosure())
@@ -262,11 +258,11 @@ class TestOverridingDelegate final : public InjectableTestDelegate {
 
   void Quit() override {
     should_quit_ = true;
-    overriden_delegate_->Quit();
+    overridden_delegate_->Quit();
   }
 
   void EnsureWorkScheduled() override {
-    overriden_delegate_->EnsureWorkScheduled();
+    overridden_delegate_->EnsureWorkScheduled();
   }
 
   scoped_refptr<SimpleSingleThreadTaskRunner> simple_task_runner_ =
@@ -274,12 +270,10 @@ class TestOverridingDelegate final : public InjectableTestDelegate {
 
   ScopedClosureRunner thread_task_runner_handle_override_scope_;
 
-  scoped_refptr<SingleThreadTaskRunner> overriden_task_runner_;
-  RunLoop::Delegate* overriden_delegate_;
+  scoped_refptr<SingleThreadTaskRunner> overridden_task_runner_;
+  RunLoop::Delegate* overridden_delegate_;
 
   bool should_quit_ = false;
-
-  RunLoop::Delegate::Client* run_loop_client_ = nullptr;
 };
 
 enum class RunLoopTestType {

base/test/test_mock_time_task_runner.cc

@@ -197,7 +197,7 @@ TestMockTimeTaskRunner::TestMockTimeTaskRunner(Time start_time,
                                                Type type)
     : now_(start_time), now_ticks_(start_ticks), tasks_lock_cv_(&tasks_lock_) {
   if (type == Type::kBoundToThread) {
-    run_loop_client_ = RunLoop::RegisterDelegateForCurrentThread(this);
+    RunLoop::RegisterDelegateForCurrentThread(this);
     thread_task_runner_handle_ = std::make_unique<ThreadTaskRunnerHandle>(
         MakeRefCounted<NonOwningProxyTaskRunner>(this));
   }
@@ -387,7 +387,7 @@ void TestMockTimeTaskRunner::Run(bool application_tasks_allowed) {
 
   while (!quit_run_loop_) {
     RunUntilIdle();
-    if (quit_run_loop_ || run_loop_client_->ShouldQuitWhenIdle())
+    if (quit_run_loop_ || ShouldQuitWhenIdle())
       break;
 
     // Peek into |tasks_| to perform one of two things:

base/test/test_mock_time_task_runner.h

@@ -248,10 +248,6 @@ class TestMockTimeTaskRunner : public SingleThreadTaskRunner,
 
   mutable Lock tasks_lock_;
   ConditionVariable tasks_lock_cv_;
-
-  // Members used to in TestMockTimeTaskRunners of Type::kBoundToThread to take
-  // ownership of the thread it was created on.
-  RunLoop::Delegate::Client* run_loop_client_ = nullptr;
   std::unique_ptr<ThreadTaskRunnerHandle> thread_task_runner_handle_;
 
   // Set to true in RunLoop::Delegate::Quit() to signal the topmost