Fix Up SingletonHwnd Observer Lifetime Issues

ui/gfx/win/singleton_hwnd.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/win/singleton_hwnd.h"
 
 #include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
 
 namespace gfx {
 
+// SingletonHwnd::Observer
+SingletonHwnd::Observer::Observer() {}
+
+SingletonHwnd::Observer::~Observer() {
+  ClearWndProc();
+}
+
+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::Observer::ClearWndProc() {
+  SetWndProc(WndProc());
+}
+
+void SingletonHwnd::Observer::OnWndProc(HWND hwnd,
+                                        UINT message,
+                                        WPARAM wparam,
+                                        LPARAM lparam) {
+  if (wndProc_.is_null())
+    return;
+
+  wndProc_.Run(hwnd, message, wparam, lparam);
+
+  if (message == WM_NCDESTROY) {
+    ClearWndProc();
+  }
+}
+
 // static
 SingletonHwnd* SingletonHwnd::GetInstance() {
   return Singleton<SingletonHwnd>::get();
 }
 
-void SingletonHwnd::AddObserver(Observer* observer) {
-  observer_list_.AddObserver(observer);
-}
-
-void SingletonHwnd::RemoveObserver(Observer* observer) {
-  if (!hwnd())
-    return;
-  observer_list_.RemoveObserver(observer);
-}
-
 BOOL SingletonHwnd::ProcessWindowMessage(HWND window,
                                          UINT message,
                                          WPARAM wparam,
                                          LPARAM lparam,
                                          LRESULT& result,
                                          DWORD msg_map_id) {
   FOR_EACH_OBSERVER(Observer,
                     observer_list_,
                     OnWndProc(window, message, wparam, lparam));
   return false;
 }
 
 SingletonHwnd::SingletonHwnd() {
   if (!base::MessageLoopForUI::IsCurrent()) {
     // Creating this window in (e.g.) a renderer inhibits shutdown on
     // Windows. See http://crbug.com/230122 and http://crbug.com/236039.
     DLOG(ERROR) << "Cannot create windows on non-UI thread!";
     return;
   }
   WindowImpl::Init(NULL, Rect());
 }
 
 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