Switch MessagePumpWin to use base::win::MessageWindow

base/message_loop/message_pump_win.cc

Index: base/message_loop/message_pump_win.cc
diff --git a/base/message_loop/message_pump_win.cc b/base/message_loop/message_pump_win.cc
index 1e518d8214d0ce155af596253edd2592cee10530..447425aa05e30ea4d227eeb783b3edabe5b19ac4 100644
--- a/base/message_loop/message_pump_win.cc
+++ b/base/message_loop/message_pump_win.cc
@@ -31,8 +31,6 @@ enum MessageLoopProblems {
 
 }  // namespace
 
-static const wchar_t kWndClassFormat[] = L"Chrome_MessagePumpWindow_%p";
-
 // Message sent to get an additional time slice for pumping (processing) another
 // task (a series of such messages creates a continuous task pump).
 static const int kMsgHaveWork = WM_USER + 1;
@@ -43,8 +41,7 @@ static const int kMessageFilterCode = 0x5001;
 //-----------------------------------------------------------------------------
 // MessagePumpWin public:
 
-MessagePumpWin::MessagePumpWin() {
-}
+MessagePumpWin::MessagePumpWin() = default;
 
 void MessagePumpWin::Run(Delegate* delegate) {
   RunState s;
@@ -94,23 +91,19 @@ int MessagePumpWin::GetCurrentDelay() const {
 //-----------------------------------------------------------------------------
 // MessagePumpForUI public:
 
-MessagePumpForUI::MessagePumpForUI()
-    : atom_(0) {
-  InitMessageWnd();
+MessagePumpForUI::MessagePumpForUI() {
+  DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback,

[dcheng, 2016/11/08 23:20:39]

I think this can't be in a DCHECK =)

[robliao, 2016/11/08 23:31:56]

This migrates the previous behavior:
message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0,	
                               HWND_MESSAGE, 0, instance, 0);	
DCHECK(message_hwnd_);

Would you rather this be a CHECK?

[scottmg, 2016/11/08 23:33:54]

DCHECKs are compiled out, with side-effects lost in Debug builds.

[robliao, 2016/11/08 23:47:38]

Doh! Indeed. Fixed. Strange that this passed the bots? O_o

Looks like I was conflating this with the code that helped make sure that the
variable used at least got referenced. I guess the optimizer does the final
cleanup.

[scottmg, 2016/11/08 23:50:26]

Unfortunately the tests on trybots are only release-with-dchecks on, because
running an actual Debug is too slow.

[robliao, 2016/11/09 00:54:01]

Duly noted! Thanks!

For trivia, this line expands to the following in NDEBUG builds...
The "0 ?" below is particularly important for the dead code eliminator to get
rid of the call later on, but we appear to compile with the full expression in
the meantime.

!(0 ? !(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback,
Unretained(this)))) : false) ? (void) 0 : ::logging::LogMessageVoidify() &
(::logging::LogMessage("../../base/message_loop/message_pump_win.cc", 96,
::logging::LOG_INFO ).stream()) << "Check failed: "
"message_window_.Create(Bind(&MessagePumpForUI::MessageCallback,
Unretained(this)))" ". ";

[scottmg, 2016/11/09 01:00:01]

Yeah, that sucks. :( I have the feeling that it's something to do with the wacky
<< logging, so we can't safely compile it out at the preprocessor level. But it
could just be that the macro expansions scare everyone so they don't get
touched.

FWIW, I did just notice a call to ~LogMessage that I didn't expect to see while
investigating crbug.com/614753 where the only cause looks like it'd be a DCHECK.
So maybe the compiler doesn't always get rid of everything? I'm not sure, but it
might be worth investigating a bit more.

+                                Unretained(this))));
 }
 
-MessagePumpForUI::~MessagePumpForUI() {
-  DestroyWindow(message_hwnd_);
-  UnregisterClass(MAKEINTATOM(atom_), CURRENT_MODULE());
-}
+MessagePumpForUI::~MessagePumpForUI() = default;
 
 void MessagePumpForUI::ScheduleWork() {
   if (InterlockedExchange(&work_state_, HAVE_WORK) != READY)
     return;  // Someone else continued the pumping.
 
   // Make sure the MessagePump does some work for us.
-  BOOL ret = PostMessage(message_hwnd_, kMsgHaveWork,
-                         reinterpret_cast<WPARAM>(this), 0);
+  BOOL ret = PostMessage(message_window_.hwnd(), kMsgHaveWork, 0, 0);
   if (ret)
     return;  // There was room in the Window Message queue.
 
@@ -139,18 +132,17 @@ void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
 //-----------------------------------------------------------------------------
 // MessagePumpForUI private:
 
-// static
-LRESULT CALLBACK MessagePumpForUI::WndProcThunk(
-    HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
+bool MessagePumpForUI::MessageCallback(
+    UINT message, WPARAM wparam, LPARAM lparam, LRESULT* result) {
   switch (message) {
     case kMsgHaveWork:
-      reinterpret_cast<MessagePumpForUI*>(wparam)->HandleWorkMessage();
+      HandleWorkMessage();
       break;
     case WM_TIMER:
-      reinterpret_cast<MessagePumpForUI*>(wparam)->HandleTimerMessage();
+      HandleTimerMessage();
       break;
   }
-  return DefWindowProc(hwnd, message, wparam, lparam);
+  return false;
 }
 
 void MessagePumpForUI::DoRunLoop() {
@@ -191,7 +183,7 @@ void MessagePumpForUI::DoRunLoop() {
     // don't want to disturb that timer if it is already in flight.  However,
     // if we did do all remaining delayed work, then lets kill the WM_TIMER.
     if (more_work_is_plausible && delayed_work_time_.is_null())
-      KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
+      KillTimer(message_window_.hwnd(), reinterpret_cast<UINT_PTR>(this));
     if (state_->should_quit)
       break;
 
@@ -209,24 +201,6 @@ void MessagePumpForUI::DoRunLoop() {
   }
 }
 
-void MessagePumpForUI::InitMessageWnd() {
-  // Generate a unique window class name.
-  string16 class_name = StringPrintf(kWndClassFormat, this);
-
-  HINSTANCE instance = CURRENT_MODULE();
-  WNDCLASSEX wc = {0};
-  wc.cbSize = sizeof(wc);
-  wc.lpfnWndProc = base::win::WrappedWindowProc<WndProcThunk>;
-  wc.hInstance = instance;
-  wc.lpszClassName = class_name.c_str();
-  atom_ = RegisterClassEx(&wc);
-  DCHECK(atom_);
-
-  message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0,
-                                 HWND_MESSAGE, 0, instance, 0);
-  DCHECK(message_hwnd_);
-}
-
 void MessagePumpForUI::WaitForWork() {
   // Wait until a message is available, up to the time needed by the timer
   // manager to fire the next set of timers.
@@ -296,7 +270,7 @@ void MessagePumpForUI::HandleWorkMessage() {
 }
 
 void MessagePumpForUI::HandleTimerMessage() {
-  KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
+  KillTimer(message_window_.hwnd(), reinterpret_cast<UINT_PTR>(this));
 
   // If we are being called outside of the context of Run, then don't do
   // anything.  This could correspond to a MessageBox call or something of
@@ -341,8 +315,7 @@ void MessagePumpForUI::RescheduleTimer() {
 
     // Create a WM_TIMER event that will wake us up to check for any pending
     // timers (in case we are running within a nested, external sub-pump).
-    BOOL ret = SetTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this),
-                        delay_msec, nullptr);
+    BOOL ret = SetTimer(message_window_.hwnd(), NULL, delay_msec, nullptr);

[dcheng, 2016/11/08 23:20:39]

Should we be consistent about using nullptr?

[robliao, 2016/11/08 23:31:56]

The second argument to SetTimer is a UINT_PTR, which can't be nullptr.

     if (ret)
       return;
     // If we can't set timers, we are in big trouble... but cross our fingers
@@ -385,7 +358,7 @@ bool MessagePumpForUI::ProcessMessageHelper(const MSG& msg) {
   }
 
   // While running our main message pump, we discard kMsgHaveWork messages.
-  if (msg.message == kMsgHaveWork && msg.hwnd == message_hwnd_)
+  if (msg.message == kMsgHaveWork && msg.hwnd == message_window_.hwnd())
     return ProcessPumpReplacementMessage();
 
   if (CallMsgFilter(const_cast<MSG*>(&msg), kMessageFilterCode))
@@ -413,7 +386,7 @@ bool MessagePumpForUI::ProcessPumpReplacementMessage() {
 
   // Expect no message or a message different than kMsgHaveWork.
   DCHECK(!have_message || kMsgHaveWork != msg.message ||
-         msg.hwnd != message_hwnd_);
+         msg.hwnd != message_window_.hwnd());
 
   // Since we discarded a kMsgHaveWork message, we must update the flag.
   int old_work_state_ = InterlockedExchange(&work_state_, READY);
@@ -438,7 +411,7 @@ MessagePumpForGpu::MessagePumpForGpu() {
   event_.Set(CreateEvent(nullptr, FALSE, FALSE, nullptr));
 }
 
-MessagePumpForGpu::~MessagePumpForGpu() {}
+MessagePumpForGpu::~MessagePumpForGpu() = default;
 
 // static
 void MessagePumpForGpu::InitFactory() {