wm: Change the DispatcherClient interface.

Instead of having RunWithDispatcher() to start a nested message-loop, and a
corresponding QuitNestedMessageLoop() to terminate it, have a DispatcherRunLoop
object that is similar to base::RunLoop for starting a nested message loop for
a specified DispatcherClient.

BUG=none
R=sky@chromium.org

Review URL: https://codereview.chromium.org/280483003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273634 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 153 insertions, 81 deletions

diff --git a/ui/views/controls/menu/menu_controller_unittest.cc b/ui/views/controls/menu/menu_controller_unittest.cc
index 28424c9..1c98f95 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -74,22 +74,29 @@ class TestDispatcherClient : public aura::client::DispatcherClient {
   }
 
   // aura::client::DispatcherClient:
-  virtual void RunWithDispatcher(
-      base::MessagePumpDispatcher* dispatcher) OVERRIDE {
+  virtual void PrepareNestedLoopClosures(
+      base::MessagePumpDispatcher* dispatcher,
+      base::Closure* run_closure,
+      base::Closure* quit_closure) OVERRIDE {
+    scoped_ptr<base::RunLoop> run_loop(new base::RunLoop());
+    *quit_closure = run_loop->QuitClosure();
+    *run_closure = base::Bind(&TestDispatcherClient::RunNestedDispatcher,
+                              base::Unretained(this),
+                              base::Passed(&run_loop),
+                              dispatcher);
+  }
+
+ private:
+  void RunNestedDispatcher(scoped_ptr<base::RunLoop> run_loop,
+                           base::MessagePumpDispatcher* dispatcher) {
     base::AutoReset<base::MessagePumpDispatcher*> reset_dispatcher(&dispatcher_,
                                                                    dispatcher);
     base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
     base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    base::RunLoop run_loop;
-    quit_callback_ = run_loop.QuitClosure();
-    run_loop.Run();
+    run_loop->Run();
   }
 
-  virtual void QuitNestedMessageLoop() OVERRIDE { quit_callback_.Run(); }
-
- private:
   base::MessagePumpDispatcher* dispatcher_;
-  base::Closure quit_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDispatcherClient);
 };
diff --git a/ui/views/controls/menu/menu_message_loop_aura.cc b/ui/views/controls/menu/menu_message_loop_aura.cc
index f49920b..0d7e99a 100644
--- a/ui/views/controls/menu/menu_message_loop_aura.cc
+++ b/ui/views/controls/menu/menu_message_loop_aura.cc
@@ -130,6 +130,12 @@ void MenuMessageLoopAura::Run(MenuController* controller,
   // |owner_| may be NULL.
   owner_ = owner;
   aura::Window* root = GetOwnerRootWindow(owner_);
+  // It is possible for the same MenuMessageLoopAura to start a nested
+  // message-loop while it is already running a nested loop. So make sure the
+  // quit-closure gets reset to the outer loop's quit-closure once the innermost
+  // loop terminates.
+  base::AutoReset<base::Closure> reset_quit_closure(&message_loop_quit_,
+                                                    base::Closure());
 
 #if defined(OS_WIN)
   internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
@@ -137,12 +143,15 @@ void MenuMessageLoopAura::Run(MenuController* controller,
     scoped_ptr<ActivationChangeObserverImpl> observer;
     if (!nested_menu)
       observer.reset(new ActivationChangeObserverImpl(controller, root));
-    aura::client::GetDispatcherClient(root)
-        ->RunWithDispatcher(&nested_dispatcher);
+    aura::client::DispatcherRunLoop run_loop(
+        aura::client::GetDispatcherClient(root), &nested_dispatcher);
+    message_loop_quit_ = run_loop.QuitClosure();
+    run_loop.Run();
   } else {
     base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
     base::MessageLoop::ScopedNestableTaskAllower allow(loop);
     base::RunLoop run_loop(&nested_dispatcher);
+    message_loop_quit_ = run_loop.QuitClosure();
     run_loop.Run();
   }
 #else
@@ -158,11 +167,15 @@ void MenuMessageLoopAura::Run(MenuController* controller,
     scoped_ptr<ActivationChangeObserverImpl> observer;
     if (!nested_menu)
       observer.reset(new ActivationChangeObserverImpl(controller, root));
-    aura::client::GetDispatcherClient(root)->RunWithDispatcher(NULL);
+    aura::client::DispatcherRunLoop run_loop(
+        aura::client::GetDispatcherClient(root), NULL);
+    message_loop_quit_ = run_loop.QuitClosure();
+    run_loop.Run();
   } else {
     base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
     base::MessageLoop::ScopedNestableTaskAllower allow(loop);
     base::RunLoop run_loop;
+    message_loop_quit_ = run_loop.QuitClosure();
     run_loop.Run();
   }
   nested_dispatcher_ = old_dispatcher.Pass();
@@ -176,14 +189,8 @@ bool MenuMessageLoopAura::ShouldQuitNow() const {
 }
 
 void MenuMessageLoopAura::QuitNow() {
-  if (owner_) {
-    // It's safe to invoke QuitNestedMessageLoop() multiple times, it only
-    // effects the current loop.
-    aura::Window* root = owner_->GetNativeWindow()->GetRootWindow();
-    aura::client::GetDispatcherClient(root)->QuitNestedMessageLoop();
-  } else {
-    base::MessageLoop::current()->QuitNow();
-  }
+  CHECK(!message_loop_quit_.is_null());
+  message_loop_quit_.Run();
   // Restore the previous dispatcher.
   nested_dispatcher_.reset();
 }
diff --git a/ui/views/controls/menu/menu_message_loop_aura.h b/ui/views/controls/menu/menu_message_loop_aura.h
index 76d95e3..49e8a39 100644
--- a/ui/views/controls/menu/menu_message_loop_aura.h
+++ b/ui/views/controls/menu/menu_message_loop_aura.h
@@ -5,10 +5,15 @@
 #ifndef UI_VIEWS_CONTROLS_MENU_MENU_MESSAGE_LOOP_AURA_H_
 #define UI_VIEWS_CONTROLS_MENU_MENU_MESSAGE_LOOP_AURA_H_
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "ui/views/controls/menu/menu_message_loop.h"
 
+namespace base {
+class MessagePumpDispatcher;
+}
+
 namespace ui {
 class ScopedEventDispatcher;
 }
@@ -37,6 +42,7 @@ class MenuMessageLoopAura : public MenuMessageLoop {
   Widget* owner_;
 
   scoped_ptr<ui::ScopedEventDispatcher> nested_dispatcher_;
+  base::Closure message_loop_quit_;
 
   DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura);
 };
diff --git a/ui/views/widget/desktop_aura/desktop_dispatcher_client.cc b/ui/views/widget/desktop_aura/desktop_dispatcher_client.cc
index 9962eaa..4b8ca2d 100644
--- a/ui/views/widget/desktop_aura/desktop_dispatcher_client.cc
+++ b/ui/views/widget/desktop_aura/desktop_dispatcher_client.cc
@@ -5,44 +5,29 @@
 #include "ui/views/widget/desktop_aura/desktop_dispatcher_client.h"
 
 #include "base/auto_reset.h"
+#include "base/bind.h"
 #include "base/run_loop.h"
 
 namespace views {
 
-DesktopDispatcherClient::DesktopDispatcherClient()
-    : weak_ptr_factory_(this) {}
+DesktopDispatcherClient::DesktopDispatcherClient() {
+}
 
 DesktopDispatcherClient::~DesktopDispatcherClient() {
 }
 
-void DesktopDispatcherClient::RunWithDispatcher(
-    base::MessagePumpDispatcher* nested_dispatcher) {
-  // TODO(erg): This class has been copypastad from
-  // ash/accelerators/nested_dispatcher_controller.cc. I have left my changes
-  // commented out because I don't entirely understand the implications of the
-  // change.
-  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-  base::MessageLoopForUI::ScopedNestableTaskAllower allow_nested(loop);
-
-  base::Closure old_quit_closure = quit_closure_;
+void DesktopDispatcherClient::PrepareNestedLoopClosures(
+    base::MessagePumpDispatcher* dispatcher,
+    base::Closure* run_closure,
+    base::Closure* quit_closure) {
 #if defined(OS_WIN)
-  base::RunLoop run_loop(nested_dispatcher);
+  scoped_ptr<base::RunLoop> run_loop(new base::RunLoop(dispatcher));
 #else
-  base::RunLoop run_loop;
+  scoped_ptr<base::RunLoop> run_loop(new base::RunLoop());
 #endif
-
-  quit_closure_ = run_loop.QuitClosure();
-  base::WeakPtr<DesktopDispatcherClient> alive(weak_ptr_factory_.GetWeakPtr());
-  run_loop.Run();
-  if (alive) {
-    weak_ptr_factory_.InvalidateWeakPtrs();
-    quit_closure_ = old_quit_closure;
-  }
-}
-
-void DesktopDispatcherClient::QuitNestedMessageLoop() {
-  CHECK(!quit_closure_.is_null());
-  quit_closure_.Run();
+  *quit_closure = run_loop->QuitClosure();
+  *run_closure =
+      base::Bind(&base::RunLoop::Run, base::Owned(run_loop.release()));
 }
 
 }  // namespace views
diff --git a/ui/views/widget/desktop_aura/desktop_dispatcher_client.h b/ui/views/widget/desktop_aura/desktop_dispatcher_client.h
index 5351d8a..284e81c 100644
--- a/ui/views/widget/desktop_aura/desktop_dispatcher_client.h
+++ b/ui/views/widget/desktop_aura/desktop_dispatcher_client.h
@@ -7,7 +7,6 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
-#include "base/memory/weak_ptr.h"
 #include "ui/views/views_export.h"
 #include "ui/wm/public/dispatcher_client.h"
 
@@ -20,17 +19,12 @@ class VIEWS_EXPORT DesktopDispatcherClient
   DesktopDispatcherClient();
   virtual ~DesktopDispatcherClient();
 
-  virtual void RunWithDispatcher(
-      base::MessagePumpDispatcher* dispatcher) OVERRIDE;
-  virtual void QuitNestedMessageLoop() OVERRIDE;
+  virtual void PrepareNestedLoopClosures(
+      base::MessagePumpDispatcher* dispatcher,
+      base::Closure* run_closure,
+      base::Closure* quit_closure) OVERRIDE;
 
  private:
-  base::Closure quit_closure_;
-
-  // Used to keep track of whether the client has been destroyed while the
-  // nested loop was running.
-  base::WeakPtrFactory<DesktopDispatcherClient> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(DesktopDispatcherClient);
 };
 
diff --git a/ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc b/ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
index 30dc551..4469364 100644
--- a/ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
+++ b/ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
@@ -189,8 +189,8 @@ TEST_F(DesktopNativeWidgetAuraTest, DontAccessContentWindowDuringDestruction) {
 }
 
 void QuitNestedLoopAndCloseWidget(scoped_ptr<Widget> widget,
-                                  aura::client::DispatcherClient* client) {
-  client->QuitNestedMessageLoop();
+                                  base::Closure* quit_runloop) {
+  quit_runloop->Run();
 }
 
 // Verifies that a widget can be destroyed when running a nested message-loop.
@@ -211,9 +211,13 @@ TEST_F(DesktopNativeWidgetAuraTest, WidgetCanBeDestroyedFromNestedLoop) {
   // Post a task that terminates the nested loop and destroyes the widget. This
   // task will be executed from the nested loop initiated with the call to
   // |RunWithDispatcher()| below.
+  aura::client::DispatcherRunLoop run_loop(client, NULL);
+  base::Closure quit_runloop = run_loop.QuitClosure();
   message_loop()->PostTask(FROM_HERE,
-      base::Bind(&QuitNestedLoopAndCloseWidget, base::Passed(&widget), client));
-  client->RunWithDispatcher(NULL);
+                           base::Bind(&QuitNestedLoopAndCloseWidget,
+                                      base::Passed(&widget),
+                                      base::Unretained(&quit_runloop)));
+  run_loop.Run();
 }
 
 }  // namespace views
diff --git a/ui/wm/core/nested_accelerator_controller.cc b/ui/wm/core/nested_accelerator_controller.cc
index 782ac6d..e4ace4f 100644
--- a/ui/wm/core/nested_accelerator_controller.cc
+++ b/ui/wm/core/nested_accelerator_controller.cc
@@ -5,6 +5,7 @@
 #include "ui/wm/core/nested_accelerator_controller.h"
 
 #include "base/auto_reset.h"
+#include "base/bind.h"
 #include "base/run_loop.h"
 #include "ui/wm/core/nested_accelerator_delegate.h"
 #include "ui/wm/core/nested_accelerator_dispatcher.h"
@@ -20,28 +21,36 @@ NestedAcceleratorController::NestedAcceleratorController(
 NestedAcceleratorController::~NestedAcceleratorController() {
 }
 
-void NestedAcceleratorController::RunWithDispatcher(
-    base::MessagePumpDispatcher* nested_dispatcher) {
-  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-  base::MessageLoopForUI::ScopedNestableTaskAllower allow_nested(loop);
-
+void NestedAcceleratorController::PrepareNestedLoopClosures(
+    base::MessagePumpDispatcher* nested_dispatcher,
+    base::Closure* run_closure,
+    base::Closure* quit_closure) {
   scoped_ptr<NestedAcceleratorDispatcher> old_accelerator_dispatcher =
       accelerator_dispatcher_.Pass();
   accelerator_dispatcher_ = NestedAcceleratorDispatcher::Create(
       dispatcher_delegate_.get(), nested_dispatcher);
 
-  // TODO(jbates) crbug.com/134753 Find quitters of this RunLoop and have them
-  //              use run_loop.QuitClosure().
   scoped_ptr<base::RunLoop> run_loop = accelerator_dispatcher_->CreateRunLoop();
-  base::AutoReset<base::Closure> reset_closure(&quit_closure_,
-                                               run_loop->QuitClosure());
+  *quit_closure =
+      base::Bind(&NestedAcceleratorController::QuitNestedMessageLoop,
+                 base::Unretained(this),
+                 run_loop->QuitClosure());
+  *run_closure = base::Bind(&NestedAcceleratorController::RunNestedMessageLoop,
+                            base::Unretained(this),
+                            base::Passed(&run_loop),
+                            base::Passed(&old_accelerator_dispatcher));
+}
+
+void NestedAcceleratorController::RunNestedMessageLoop(
+    scoped_ptr<base::RunLoop> run_loop,
+    scoped_ptr<NestedAcceleratorDispatcher> old_accelerator_dispatcher) {
   run_loop->Run();
   accelerator_dispatcher_ = old_accelerator_dispatcher.Pass();
 }
 
-void NestedAcceleratorController::QuitNestedMessageLoop() {
-  CHECK(!quit_closure_.is_null());
-  quit_closure_.Run();
+void NestedAcceleratorController::QuitNestedMessageLoop(
+    const base::Closure& quit_runloop) {
+  quit_runloop.Run();
   accelerator_dispatcher_.reset();
 }
 
diff --git a/ui/wm/core/nested_accelerator_controller.h b/ui/wm/core/nested_accelerator_controller.h
index 2adfcbc..2826d72e 100644
--- a/ui/wm/core/nested_accelerator_controller.h
+++ b/ui/wm/core/nested_accelerator_controller.h
@@ -26,12 +26,16 @@ class WM_EXPORT NestedAcceleratorController
   virtual ~NestedAcceleratorController();
 
   // aura::client::DispatcherClient:
-  virtual void RunWithDispatcher(
-      base::MessagePumpDispatcher* dispatcher) OVERRIDE;
-  virtual void QuitNestedMessageLoop() OVERRIDE;
+  virtual void PrepareNestedLoopClosures(
+      base::MessagePumpDispatcher* dispatcher,
+      base::Closure* run_closure,
+      base::Closure* quit_closure) OVERRIDE;
 
  private:
-  base::Closure quit_closure_;
+  void RunNestedMessageLoop(scoped_ptr<base::RunLoop> run_loop,
+                            scoped_ptr<NestedAcceleratorDispatcher> dispatcher);
+  void QuitNestedMessageLoop(const base::Closure& quit_runloop);
+
   scoped_ptr<NestedAcceleratorDispatcher> accelerator_dispatcher_;
   scoped_ptr<NestedAcceleratorDelegate> dispatcher_delegate_;
 
diff --git a/ui/wm/core/nested_accelerator_controller_unittest.cc b/ui/wm/core/nested_accelerator_controller_unittest.cc
index 996b2a4..fae9f8e 100644
--- a/ui/wm/core/nested_accelerator_controller_unittest.cc
+++ b/ui/wm/core/nested_accelerator_controller_unittest.cc
@@ -180,7 +180,9 @@ TEST_F(NestedAcceleratorTest, AssociatedWindowAboveLockScreen) {
   scoped_ptr<ui::ScopedEventDispatcher> override_dispatcher =
       ui::PlatformEventSource::GetInstance()->OverrideDispatcher(
           &inner_dispatcher);
-  aura::client::GetDispatcherClient(root_window())->RunWithDispatcher(NULL);
+  aura::client::DispatcherRunLoop run_loop(
+      aura::client::GetDispatcherClient(root_window()), NULL);
+  run_loop.Run();
   EXPECT_EQ(1, inner_dispatcher.num_key_events_dispatched());
 }
 
@@ -199,7 +201,9 @@ TEST_F(NestedAcceleratorTest, AcceleratorsHandled) {
   scoped_ptr<ui::ScopedEventDispatcher> override_dispatcher =
       ui::PlatformEventSource::GetInstance()->OverrideDispatcher(
           &inner_dispatcher);
-  aura::client::GetDispatcherClient(root_window())->RunWithDispatcher(NULL);
+  aura::client::DispatcherRunLoop run_loop(
+      aura::client::GetDispatcherClient(root_window()), NULL);
+  run_loop.Run();
   EXPECT_EQ(0, inner_dispatcher.num_key_events_dispatched());
   EXPECT_EQ(1, target.accelerator_pressed_count());
 }
diff --git a/ui/wm/public/dispatcher_client.cc b/ui/wm/public/dispatcher_client.cc
index cec4a8a..e3be7ca 100644
--- a/ui/wm/public/dispatcher_client.cc
+++ b/ui/wm/public/dispatcher_client.cc
@@ -4,6 +4,7 @@
 
 #include "ui/wm/public/dispatcher_client.h"
 
+#include "base/callback.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_property.h"
 
@@ -12,6 +13,28 @@ DECLARE_WINDOW_PROPERTY_TYPE(aura::client::DispatcherClient*);
 namespace aura {
 namespace client {
 
+DispatcherRunLoop::DispatcherRunLoop(DispatcherClient* client,
+                                     base::MessagePumpDispatcher* dispatcher) {
+  client->PrepareNestedLoopClosures(dispatcher, &run_closure_, &quit_closure_);
+}
+
+DispatcherRunLoop::~DispatcherRunLoop() {
+}
+
+void DispatcherRunLoop::Run() {
+  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+  base::MessageLoopForUI::ScopedNestableTaskAllower allow_nested(loop);
+  run_closure_.Run();
+}
+
+base::Closure DispatcherRunLoop::QuitClosure() {
+  return quit_closure_;
+}
+
+void DispatcherRunLoop::Quit() {
+  quit_closure_.Run();
+}
+
 DEFINE_LOCAL_WINDOW_PROPERTY_KEY(DispatcherClient*, kDispatcherClientKey, NULL);
 
 void SetDispatcherClient(Window* root_window, DispatcherClient* client) {
diff --git a/ui/wm/public/dispatcher_client.h b/ui/wm/public/dispatcher_client.h
index 9fd9c94..a301e92 100644
--- a/ui/wm/public/dispatcher_client.h
+++ b/ui/wm/public/dispatcher_client.h
@@ -5,6 +5,8 @@
 #ifndef UI_WM_PUBLIC_DISPATCHER_CLIENT_H_
 #define UI_WM_PUBLIC_DISPATCHER_CLIENT_H_
 
+#include "base/callback.h"
+#include "base/macros.h"
 #include "base/message_loop/message_pump_dispatcher.h"
 #include "ui/aura/aura_export.h"
 
@@ -12,12 +14,39 @@ namespace aura {
 class Window;
 namespace client {
 
+class DispatcherClient;
+
+// A base::RunLoop like object for running a nested message-loop with a
+// specified DispatcherClient and a MessagePumpDispatcher.
+class AURA_EXPORT DispatcherRunLoop {
+ public:
+  DispatcherRunLoop(DispatcherClient* client,
+                    base::MessagePumpDispatcher* dispatcher);
+  ~DispatcherRunLoop();
+
+  void Run();
+  base::Closure QuitClosure();
+  void Quit();
+
+ private:
+  base::Closure run_closure_;
+  base::Closure quit_closure_;
+
+  DISALLOW_COPY_AND_ASSIGN(DispatcherRunLoop);
+};
+
 // An interface implemented by an object which handles nested dispatchers.
 class AURA_EXPORT DispatcherClient {
  public:
-  virtual void RunWithDispatcher(base::MessagePumpDispatcher* dispatcher) = 0;
+  virtual ~DispatcherClient() {}
+
+ protected:
+  friend class DispatcherRunLoop;
 
-  virtual void QuitNestedMessageLoop() = 0;
+  virtual void PrepareNestedLoopClosures(
+      base::MessagePumpDispatcher* dispatcher,
+      base::Closure* run_closure,
+      base::Closure* quit_closure) = 0;
 };
 
 AURA_EXPORT void SetDispatcherClient(Window* root_window,