Moved remoting::win::MessageWindow to base::win::MessageWindow so that it could be re-used outside …

base/win/message_window.cc

 // Copyright 2013 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 "remoting/host/win/message_window.h"
+#include "base/win/message_window.h"
 
 #include "base/logging.h"
 #include "base/process_util.h"
+#include "base/string16.h"
 #include "base/strings/stringprintf.h"
-#include "base/strings/utf_string_conversions.h"
 #include "base/win/wrapped_window_proc.h"
 
-const char kClassNameFormat[] = "Chromoting_MessageWindow_%p";
+const wchar_t kClassNameFormat[] = L"Chrome_MessageWindow_%p";

[cpu_(ooo_6.6-7.5), 2013/06/14 22:49:32]

const string16

[Sergey Ulanov, 2013/06/14 22:53:06]

Wouldn't that introduce static intializer/finializer?

[alexeypa (please no reviews), 2013/06/17 18:09:25]

Global constants are required to be POD types.

[cpu_(ooo_6.6-7.5), 2013/06/18 23:59:58]

yeah my bad.

 
-namespace remoting {
+namespace base {
 namespace win {
 
 MessageWindow::MessageWindow()
     : atom_(0),
-      instance_(NULL),
-      window_(NULL) {
-  class_name_ = base::StringPrintf(kClassNameFormat, this);
-  instance_ = base::GetModuleFromAddress(static_cast<WNDPROC>(
-      &base::win::WrappedWindowProc<WindowProc>));
-}
-
-MessageWindow::MessageWindow(const std::string& class_name, HINSTANCE instance)
-    : atom_(0),
-      class_name_(class_name),
-      instance_(instance),
       window_(NULL) {
 }
 
 MessageWindow::~MessageWindow() {
   DCHECK(CalledOnValidThread());
 
-  if (window_ != NULL) {
+  if (window_ != NULL)
     DestroyWindow(window_);
-    window_ = NULL;
-  }
 
   if (atom_ != 0) {
-    UnregisterClass(MAKEINTATOM(atom_), instance_);
-    atom_ = 0;
+    UnregisterClass(MAKEINTATOM(atom_),

[cpu_(ooo_6.6-7.5), 2013/06/14 22:49:32]

in many files we use :: for windows calls, so here it would be
::UnregisterClass, unfortunately this is not universally followed so we have
both styles. Just saying that here you have a choice and that might make the
reading easier for people.

[alexeypa (please no reviews), 2013/06/17 18:09:25]

I prefer to avoid using explicit scope.

This point was risen a bunch of times in other reviews. In all the cases I've
seen so far there was no good evidence that one way of writing this is more
readable than the other. It was all based on personal taste. I think that less
punctuation makes it less cluttered. Some folks think that an explicit scope
make it apparent that it is not a method of the same class. 

I'm also not sure whether there is a correlation between amount of Windows
experience a person has and leaning toward avoding the explicit scope.

[cpu_(ooo_6.6-7.5), 2013/06/18 23:59:58]

 It was all based on personal taste. I think that less
It actually caused bugs in some cases, I won't go into details but it requires
a mixing of atl and non atl code for example. Fortunately we manage to remove
most of the atl code.
haha. Sure. I think every 10 years writing windows code you need to add one set
of :: in front of your windows code.

Like I said it was choice. Its fine either way for this case.

+                    base::GetModuleFromAddress(&MessageWindow::WindowProc));
   }
 }
 
 bool MessageWindow::Create(Delegate* delegate) {
   DCHECK(CalledOnValidThread());
   DCHECK(!atom_);
   DCHECK(!window_);
 
   // Register a separate window class for each instance of |MessageWindow|.
-  string16 class_name = UTF8ToUTF16(class_name_);
+  string16 class_name = base::StringPrintf(kClassNameFormat, this);

[cpu_(ooo_6.6-7.5), 2013/06/14 22:49:32]

so one of the main uses is for example in our process singleton
chromium/src/chrome/browser/process_singleton_win.cc

that allows to find the window by name, it would be more reusable if we can
allow this and also have either a factory that finds the window via
FindWindowEx(HWND_MESSAGE, ...)

chromium/src/chrome/browser/chrome_process_finder_win.cc

[alexeypa (please no reviews), 2013/06/17 18:09:25]

I was going to update MessageWindow at some point such that it will register a
single window class via LazyInstance. I think it is better to do it as a
separate CL.

I added the window name parameter to MessageWindow::Create() method.

[cpu_(ooo_6.6-7.5), 2013/06/18 23:59:58]

I don't see how lazy class registration solves the problem. This is one process
finding another.

[alexeypa (please no reviews), 2013/06/19 00:04:57]

With lazy initialization there will be a single window class registered for all
message-only windows. So the name of the class could be a predefined constant.
Once it id done, the current logic will continue working. I'll just need to wrap
FindWindowEx into a nice wrapper to avoid defining the same class name is
several different places. 

+  HINSTANCE instance = base::GetModuleFromAddress(&MessageWindow::WindowProc);
+
   WNDCLASSEX window_class;
   window_class.cbSize = sizeof(window_class);
   window_class.style = 0;
   window_class.lpfnWndProc = &base::win::WrappedWindowProc<WindowProc>;
   window_class.cbClsExtra = 0;
   window_class.cbWndExtra = 0;
-  window_class.hInstance = instance_;
+  window_class.hInstance = instance;
   window_class.hIcon = NULL;
   window_class.hCursor = NULL;
   window_class.hbrBackground = NULL;
   window_class.lpszMenuName = NULL;
   window_class.lpszClassName = class_name.c_str();
   window_class.hIconSm = NULL;
   atom_ = RegisterClassEx(&window_class);
   if (atom_ == 0) {
     LOG_GETLASTERROR(ERROR)
-        << "Failed to register the window class '" << class_name_ << "'";
+        << "Failed to register the window class for a message-only window";
     return false;
   }
 
   window_ = CreateWindow(MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0,
-                         instance_, delegate);
+                         instance, delegate);
   if (!window_) {
     LOG_GETLASTERROR(ERROR) << "Failed to create a message-only window";
     return false;
   }
 
   return true;
 }
 
 // static
 LRESULT CALLBACK MessageWindow::WindowProc(HWND hwnd,
@@ skipping 15 matching lines @@
   } else {
     delegate = reinterpret_cast<Delegate*>(GetWindowLongPtr(hwnd,
                                                             GWLP_USERDATA));
   }
 
   // Handle the message.
   if (delegate) {
     LRESULT message_result;
     if (delegate->HandleMessage(hwnd, message, wparam, lparam, &message_result))
       return message_result;
   }

[cpu_(ooo_6.6-7.5), 2013/06/14 22:49:32]

An improvement here is to stop calling the delegate after WM_DESTROY is seen.

We had severe bugs where people assume that the last message is that one, which
is not true.

[alexeypa (please no reviews), 2013/06/17 18:09:25]

Done.

 
   return DefWindowProc(hwnd, message, wparam, lparam);
 }
 
 }  // namespace win
-}  // namespace remoting
+}  // namespace base