Give DisconnectWindow some TLC.

- Update the DisconnectWindow interface specification to clarify corner cases.
- Update the DisconnectCallback semantics to match the DisconnectWindow interface spec.
  - In particular, don't invoke the DisconnectCallback on Hide() or deletion.
- On Windows, invoke the DisconnectCallback if WM_DESTROY is unexpectedly received.

BUG=166056,166514


Review URL: https://chromiumcodereview.appspot.com/11583023

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

5 files changed, 34 insertions, 33 deletions

diff --git a/remoting/host/disconnect_window.h b/remoting/host/disconnect_window.h
index 51269b7..1bc053b 100644
--- a/remoting/host/disconnect_window.h
+++ b/remoting/host/disconnect_window.h
@@ -20,20 +20,17 @@ class DisconnectWindow {
     kMaximumConnectedNameWidthInPixels = 400
   };
 
-  // DisconnectCallback is called when the user clicks on the Disconnect button
-  // to disconnect the session. This callback is provided as a parameter to the
-  // Show() method, and will be triggered on the UI thread.
-  typedef base::Callback<void(void)> DisconnectCallback;
-
   virtual ~DisconnectWindow() {}
 
-  // Shows the disconnect window allowing the user to disconnect the session.
-  // Returns false if the window could not be shown for any reason. The
-  // disconnect callback will be invoked if the user clicks the disconnect
-  // button, or if the window is closed for any reason other than Hide() being
-  // called
+  // Shows the disconnect window, allowing the user to disconnect the session.
+  // The window will display text from |ui_strings| and |username|.
+  // |disconnect_callback| will be invoked on the calling UI thread when the
+  // user chooses to disconnect, or if the window is closed by any means other
+  // than Hide(), or deletion of the DisconnectWindow instance.
+  // Show returns false if the window cannot be shown, in which case the
+  // callback will not be invoked.
   virtual bool Show(const UiStrings& ui_strings,
-                    const DisconnectCallback& disconnect_callback,
+                    const base::Closure& disconnect_callback,
                     const std::string& username) = 0;
 
   // Hides the disconnect window. The disconnect callback will not be invoked.
diff --git a/remoting/host/disconnect_window_gtk.cc b/remoting/host/disconnect_window_gtk.cc
index 3a58d35..4f818b1 100644
--- a/remoting/host/disconnect_window_gtk.cc
+++ b/remoting/host/disconnect_window_gtk.cc
@@ -22,7 +22,7 @@ class DisconnectWindowGtk : public DisconnectWindow {
   virtual ~DisconnectWindowGtk();
 
   virtual bool Show(const UiStrings& ui_strings,
-                    const DisconnectCallback& disconnect_callback,
+                    const base::Closure& disconnect_callback,
                     const std::string& username) OVERRIDE;
   virtual void Hide() OVERRIDE;
 
@@ -36,7 +36,7 @@ class DisconnectWindowGtk : public DisconnectWindow {
 
   void CreateWindow(const UiStrings& ui_strings);
 
-  DisconnectCallback disconnect_callback_;
+  base::Closure disconnect_callback_;
   GtkWidget* disconnect_window_;
   GtkWidget* message_;
   GtkWidget* button_;
@@ -56,6 +56,7 @@ DisconnectWindowGtk::DisconnectWindowGtk()
 }
 
 DisconnectWindowGtk::~DisconnectWindowGtk() {
+  Hide();
 }
 
 void DisconnectWindowGtk::CreateWindow(const UiStrings& ui_strings) {
@@ -129,7 +130,7 @@ void DisconnectWindowGtk::CreateWindow(const UiStrings& ui_strings) {
 }
 
 bool DisconnectWindowGtk::Show(const UiStrings& ui_strings,
-                               const DisconnectCallback& disconnect_callback,
+                               const base::Closure& disconnect_callback,
                                const std::string& username) {
   DCHECK(disconnect_callback_.is_null());
   DCHECK(!disconnect_callback.is_null());
diff --git a/remoting/host/disconnect_window_mac.mm b/remoting/host/disconnect_window_mac.mm
index a7b47c2..1f504a3 100644
--- a/remoting/host/disconnect_window_mac.mm
+++ b/remoting/host/disconnect_window_mac.mm
@@ -40,7 +40,7 @@ DisconnectWindowMac::DisconnectWindowMac()
 }
 
 DisconnectWindowMac::~DisconnectWindowMac() {
-  [window_controller_ close];
+  Hide();
 }
 
 bool DisconnectWindowMac::Show(const UiStrings& ui_strings,
diff --git a/remoting/host/disconnect_window_win.cc b/remoting/host/disconnect_window_win.cc
index a31e11d..446806a 100644
--- a/remoting/host/disconnect_window_win.cc
+++ b/remoting/host/disconnect_window_win.cc
@@ -41,8 +41,9 @@ class DisconnectWindowWin : public DisconnectWindow {
   DisconnectWindowWin();
   virtual ~DisconnectWindowWin();
 
+  // DisconnectWindow interface.
   virtual bool Show(const UiStrings& ui_strings,
-                    const DisconnectCallback& disconnect_callback,
+                    const base::Closure& disconnect_callback,
                     const std::string& username) OVERRIDE;
   virtual void Hide() OVERRIDE;
 
@@ -56,19 +57,17 @@ private:
   bool BeginDialog(const UiStrings& ui_strings,
                    const std::string& username);
 
-  // Unregisters the disconnect hot key and closed the dialog.
+  // Closes the dialog, unregisters the hot key and invokes the disconnect
+  // callback, if set.
   void EndDialog();
 
-  // Invokes the disconnect callback passed to Show().
-  void InvokeDiconnectCallback();
-
   // Trys to position the dialog window above the taskbar.
   void SetDialogPosition();
 
   // Applies localization string and resizes the dialog.
   bool SetStrings(const UiStrings& strings, const string16& username);
 
-  DisconnectCallback disconnect_callback_;
+  base::Closure disconnect_callback_;
   HWND hwnd_;
   bool has_hotkey_;
   base::win::ScopedGDIObject<HPEN> border_pen_;
@@ -97,11 +96,11 @@ DisconnectWindowWin::DisconnectWindowWin()
 }
 
 DisconnectWindowWin::~DisconnectWindowWin() {
-  EndDialog();
+  Hide();
 }
 
 bool DisconnectWindowWin::Show(const UiStrings& ui_strings,
-                               const DisconnectCallback& disconnect_callback,
+                               const base::Closure& disconnect_callback,
                                const std::string& username) {
   DCHECK(disconnect_callback_.is_null());
   DCHECK(!disconnect_callback.is_null());
@@ -111,12 +110,14 @@ bool DisconnectWindowWin::Show(const UiStrings& ui_strings,
   if (BeginDialog(ui_strings, username)) {
     return true;
   } else {
-    EndDialog();
+    Hide();
     return false;
   }
 }
 
 void DisconnectWindowWin::Hide() {
+  // Clear the |disconnect_callback_| so it won't be invoked by EndDialog().
+  disconnect_callback_.Reset();
   EndDialog();
 }
 
@@ -154,7 +155,6 @@ BOOL DisconnectWindowWin::OnDialogMessage(HWND hwnd, UINT message,
       switch (LOWORD(wparam)) {
         case IDC_DISCONNECT:
           EndDialog();
-          InvokeDiconnectCallback();
           return TRUE;
       }
       return FALSE;
@@ -162,6 +162,11 @@ BOOL DisconnectWindowWin::OnDialogMessage(HWND hwnd, UINT message,
     // Ensure we don't try to use the HWND anymore.
     case WM_DESTROY:
       hwnd_ = NULL;
+
+      // Ensure that the disconnect callback is invoked even if somehow our
+      // window gets destroyed.
+      EndDialog();
+
       return TRUE;
 
     // Ensure the dialog stays visible if the work area dimensions change.
@@ -178,7 +183,6 @@ BOOL DisconnectWindowWin::OnDialogMessage(HWND hwnd, UINT message,
     // Handle the disconnect hot-key.
     case WM_HOTKEY:
       EndDialog();
-      InvokeDiconnectCallback();
       return TRUE;
 
     // Let the window be draggable by its client area by responding
@@ -266,14 +270,13 @@ void DisconnectWindowWin::EndDialog() {
 
   if (hwnd_) {
     ::DestroyWindow(hwnd_);
-    hwnd_ = NULL;
+    DCHECK(hwnd_ == NULL);
   }
 
-  disconnect_callback_.Reset();
-}
-
-void DisconnectWindowWin::InvokeDiconnectCallback() {
-  disconnect_callback_.Run();
+  if (!disconnect_callback_.is_null()) {
+    disconnect_callback_.Run();
+    disconnect_callback_.Reset();
+  }
 }
 
 void DisconnectWindowWin::SetDialogPosition() {
diff --git a/remoting/host/host_mock_objects.h b/remoting/host/host_mock_objects.h
index 277bafb..8a2fae6 100644
--- a/remoting/host/host_mock_objects.h
+++ b/remoting/host/host_mock_objects.h
@@ -27,7 +27,7 @@ class MockDisconnectWindow : public DisconnectWindow {
   virtual ~MockDisconnectWindow();
 
   MOCK_METHOD3(Show, bool(const UiStrings& ui_strings,
-                          const DisconnectCallback& disconnect_callback,
+                          const base::Closure& disconnect_callback,
                           const std::string& username));
   MOCK_METHOD0(Hide, void());
 };