Switch MessagePumpWin to use base::win::MessageWindow

base/message_loop/message_pump_win.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 "base/message_loop/message_pump_win.h"
 
 #include <math.h>
 #include <stdint.h>
 
 #include <limits>
@@ skipping 13 matching lines @@
 enum MessageLoopProblems {
   MESSAGE_POST_ERROR,
   COMPLETION_POST_ERROR,
   SET_TIMER_ERROR,
   RECEIVED_WM_QUIT_ERROR,
   MESSAGE_LOOP_PROBLEM_MAX,
 };
 
 }  // 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;
 
 // The application-defined code passed to the hook procedure.
 static const int kMessageFilterCode = 0x5001;
 
 //-----------------------------------------------------------------------------
 // MessagePumpWin public:
 
-MessagePumpWin::MessagePumpWin() {
-}
+MessagePumpWin::MessagePumpWin() = default;
 
 void MessagePumpWin::Run(Delegate* delegate) {
   RunState s;
   s.delegate = delegate;
   s.should_quit = false;
   s.run_depth = state_ ? state_->run_depth + 1 : 1;
 
   // TODO(stanisc): crbug.com/596190: Remove this code once the bug is fixed.
   s.schedule_work_error_count = 0;
   s.last_schedule_work_error_time = Time();
@@ skipping 29 matching lines @@
   // "overflowingly" large, that means a delayed task was posted with a
   // super-long delay.
   return timeout < 0 ? 0 :
       (timeout > std::numeric_limits<int>::max() ?
        std::numeric_limits<int>::max() : static_cast<int>(timeout));
 }
 
 //-----------------------------------------------------------------------------
 // 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.
 
   // We have failed to insert a have-work message, so there is a chance that we
   // will starve tasks/timers while sitting in a nested message loop.  Nested
   // loops only look at Windows Message queues, and don't look at *our* task
   // queues, etc., so we might not get a time slice in such. :-(
   // We could abort here, but the fear is that this failure mode is plausibly
   // common (queue is full, of about 2000 messages), so we'll do a near-graceful
   // recovery.  Nested loops are pretty transient (we think), so this will
   // probably be recoverable.
 
   // Clarify that we didn't really insert.
   InterlockedExchange(&work_state_, READY);
   UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", MESSAGE_POST_ERROR,
                             MESSAGE_LOOP_PROBLEM_MAX);
   state_->schedule_work_error_count++;
   state_->last_schedule_work_error_time = Time::Now();
 }
 
 void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   delayed_work_time_ = delayed_work_time;
   RescheduleTimer();
 }
 
 //-----------------------------------------------------------------------------
 // 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() {
   // IF this was just a simple PeekMessage() loop (servicing all possible work
   // queues), then Windows would try to achieve the following order according
   // to MSDN documentation about PeekMessage with no filter):
   //    * Sent messages
   //    * Posted messages
   //    * Sent messages (again)
   //    * WM_PAINT messages
@@ skipping 20 matching lines @@
     if (state_->should_quit)
       break;
 
     more_work_is_plausible |=
         state_->delegate->DoDelayedWork(&delayed_work_time_);
     // If we did not process any delayed work, then we can assume that our
     // existing WM_TIMER if any will fire when delayed work should run.  We
     // 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;
 
     if (more_work_is_plausible)
       continue;
 
     more_work_is_plausible = state_->delegate->DoIdleWork();
     if (state_->should_quit)
       break;
 
     if (more_work_is_plausible)
       continue;
 
     WaitForWork();  // Wait (sleep) until we have work to do again.
   }
 }
 
-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.
   int delay;
   DWORD wait_flags = MWMO_INPUTAVAILABLE;
 
   while ((delay = GetCurrentDelay()) != 0) {
     if (delay < 0)  // Negative value means no timers waiting.
       delay = INFINITE;
 
@@ skipping 49 matching lines @@
 
   // Now give the delegate a chance to do some work.  It'll let us know if it
   // needs to do more work.
   if (state_->delegate->DoWork())
     ScheduleWork();
   state_->delegate->DoDelayedWork(&delayed_work_time_);
   RescheduleTimer();
 }
 
 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
   // that sort.
   if (!state_)
     return;
 
   state_->delegate->DoDelayedWork(&delayed_work_time_);
   RescheduleTimer();
 }
@@ skipping 24 matching lines @@
   int delay_msec = GetCurrentDelay();
   DCHECK_GE(delay_msec, 0);
   if (delay_msec == 0) {
     ScheduleWork();
   } else {
     if (delay_msec < USER_TIMER_MINIMUM)
       delay_msec = USER_TIMER_MINIMUM;
 
     // 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
     // for now.
     // TODO(jar): If we don't see this error, use a CHECK() here instead.
     UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", SET_TIMER_ERROR,
                               MESSAGE_LOOP_PROBLEM_MAX);
   }
 }
 
@@ skipping 22 matching lines @@
     UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem",
                               RECEIVED_WM_QUIT_ERROR, MESSAGE_LOOP_PROBLEM_MAX);
     // Repost the QUIT message so that it will be retrieved by the primary
     // GetMessage() loop.
     state_->should_quit = true;
     PostQuitMessage(static_cast<int>(msg.wParam));
     return false;
   }
 
   // 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))
     return true;
 
   TranslateMessage(&msg);
   DispatchMessage(&msg);
 
   return true;
 }
 
 bool MessagePumpForUI::ProcessPumpReplacementMessage() {
   // When we encounter a kMsgHaveWork message, this method is called to peek and
   // process a replacement message. The goal is to make the kMsgHaveWork as non-
   // intrusive as possible, even though a continuous stream of such messages are
   // posted. This method carefully peeks a message while there is no chance for
   // a kMsgHaveWork to be pending, then resets the |have_work_| flag (allowing a
   // replacement kMsgHaveWork to possibly be posted), and finally dispatches
   // that peeked replacement. Note that the re-post of kMsgHaveWork may be
   // asynchronous to this thread!!
 
   MSG msg;
   const bool have_message =
       PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE) != FALSE;
 
   // 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);
   DCHECK_EQ(HAVE_WORK, old_work_state_);
 
   // We don't need a special time slice if we didn't have_message to process.
   if (!have_message)
     return false;
 
   // Guarantee we'll get another time slice in the case where we go into native
   // windows code.   This ScheduleWork() may hurt performance a tiny bit when
   // tasks appear very infrequently, but when the event queue is busy, the
   // kMsgHaveWork events get (percentage wise) rarer and rarer.
   ScheduleWork();
   return ProcessMessageHelper(msg);
 }
 
 //-----------------------------------------------------------------------------
 // MessagePumpForGpu public:
 
 MessagePumpForGpu::MessagePumpForGpu() {
   event_.Set(CreateEvent(nullptr, FALSE, FALSE, nullptr));
 }
 
-MessagePumpForGpu::~MessagePumpForGpu() {}
+MessagePumpForGpu::~MessagePumpForGpu() = default;
 
 // static
 void MessagePumpForGpu::InitFactory() {
   bool init_result = MessageLoop::InitMessagePumpForUIFactory(
       &MessagePumpForGpu::CreateMessagePumpForGpu);
   DCHECK(init_result);
 }
 
 // static
 std::unique_ptr<MessagePump> MessagePumpForGpu::CreateMessagePumpForGpu() {
@@ skipping 290 matching lines @@
     if (!filter || it->handler == filter) {
       *item = *it;
       completed_io_.erase(it);
       return true;
     }
   }
   return false;
 }
 
 }  // namespace base