Fix Up SingletonHwnd Observer Lifetime Issues

ui/gfx/win/singleton_hwnd.cc

Index: ui/gfx/win/singleton_hwnd.cc
diff --git a/ui/gfx/win/singleton_hwnd.cc b/ui/gfx/win/singleton_hwnd.cc
index fc4d11f2d5e0d8146d9a3fa5c0f666e2e3c86676..2db1668fe7831a32aa948fa7ae4e3dbf483b8dfd 100644
--- a/ui/gfx/win/singleton_hwnd.cc
+++ b/ui/gfx/win/singleton_hwnd.cc
@@ -9,19 +9,44 @@
 
 namespace gfx {
 
-// static
-SingletonHwnd* SingletonHwnd::GetInstance() {
-  return Singleton<SingletonHwnd>::get();
+// SingletonHwnd::Observer
+SingletonHwnd::Observer::Observer() {}
+
+SingletonHwnd::Observer::~Observer() {
+  ClearWndProc();
 }
 
-void SingletonHwnd::AddObserver(Observer* observer) {
-  observer_list_.AddObserver(observer);
+void SingletonHwnd::Observer::SetWndProc(const WndProc& wndProc) {
+  if (wndProc_.is_null())

[sky, 2015/04/24 20:58:23]

Is there a reason you can't add observer in the constructor and avoid having
conditionally adding/removing like you are? Same goes for the wnd_proc. Is there
a reason it can't be passed in the constructor? If someone wants to reset the
wnd_proc they can destroy the observer.

[robliao, 2015/04/24 21:49:03]

A critical part of SingletonHwndObserver is that it's bound to the lifetime of
the requester. This means it either needs to be a member of the requester (and
it just works) or the requester needs to notify the SingletonHwndObserver of its
destruction (the error prone part).

I wanted this to just work, which means it needed to be either a direct member
or a scoped_ptr member. All of the classes involved indicate that they will
observe at some point in time, but not necessarily at the beginning. I figured
if that was the case, might as well make this an inline member of the class and
save ourselves a 'new' call. 

Since we're inlined, to support the case where we may not listen all the time,
setting and clearing the WndProc seemed like a natural way to indicate that. The
code to handle it is simple enough and it avoids the need to notify
SingletonHwnd if the wnd_proc simply changed.

[sky, 2015/04/27 15:11:28]

I would prefer a scoped_ptr. It's make the observer easier to implement and the
caller is already dealing with lazy setup.

[robliao, 2015/04/27 22:45:46]

Done.

+    SingletonHwnd::GetInstance()->AddObserver(this);
+
+  wndProc_ = wndProc;
+
+  if (wndProc_.is_null())
+    SingletonHwnd::GetInstance()->RemoveObserver(this);
 }
 
-void SingletonHwnd::RemoveObserver(Observer* observer) {
-  if (!hwnd())
+void SingletonHwnd::Observer::ClearWndProc() {
+  SetWndProc(WndProc());
+}
+
+void SingletonHwnd::Observer::OnWndProc(HWND hwnd,
+                                        UINT message,
+                                        WPARAM wparam,
+                                        LPARAM lparam) {
+  if (wndProc_.is_null())
     return;
-  observer_list_.RemoveObserver(observer);
+
+  wndProc_.Run(hwnd, message, wparam, lparam);
+
+  if (message == WM_NCDESTROY) {
+    ClearWndProc();
+  }
+}
+
+// static
+SingletonHwnd* SingletonHwnd::GetInstance() {
+  return Singleton<SingletonHwnd>::get();
 }
 
 BOOL SingletonHwnd::ProcessWindowMessage(HWND window,
@@ -47,6 +72,18 @@ SingletonHwnd::SingletonHwnd() {
 }
 
 SingletonHwnd::~SingletonHwnd() {
+  // WindowImpl will clean up the hwnd value on WM_NCDESTROY.
+  DestroyWindow(hwnd());
+  DCHECK(!observer_list_.might_have_observers());
+}
+
+void SingletonHwnd::AddObserver(Observer* observer) {
+  DCHECK(hwnd());
+  observer_list_.AddObserver(observer);
+}
+
+void SingletonHwnd::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
 }
 
 }  // namespace gfx