Split the singleton hwnd from screen_compatible_dc_win.cc.

Split the singleton hwnd from screen_compatible_dc_win.cc.

This allows different clients to register for global WM_ messages.

Specifically, I plan to use this to listen for changes to the "use ClearType" system setting, which will be a separate CL.

BUG=105550, 113184
TEST=none

[tfarina, 2012-02-08 20:34:55]

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.cc
File ui/base/win/singleton_hwnd.cc (right):

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.c...
ui/base/win/singleton_hwnd.cc:7: #include <vector>
nit: I'd remove this include since you already need/have it on the header file.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.c...
ui/base/win/singleton_hwnd.cc:12: #include "base/memory/singleton.h"
also in the header file.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:6: #define UI_GFX_SINGLETON_HWND_H_
nit: UI_BASE_WIN_SINGLETON_HWND_H_

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:10: #include <vector>
nit: I'd put a blank line between windows.h and vector to separate between C
includes and C++ includes.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:13: #include "base/basictypes.h"
sort.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:16: namespace gfx {
nit: namespace ui instead?

[asvitkine_google, 2012-02-08 21:04:57]

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.cc
File ui/base/win/singleton_hwnd.cc (right):

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.c...
ui/base/win/singleton_hwnd.cc:7: #include <vector>
On 2012/02/08 20:34:55, tfarina wrote:
> nit: I'd remove this include since you already need/have it on the header
file.

Done.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.c...
ui/base/win/singleton_hwnd.cc:12: #include "base/memory/singleton.h"
On 2012/02/08 20:34:55, tfarina wrote:
> also in the header file.

Done.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:6: #define UI_GFX_SINGLETON_HWND_H_
On 2012/02/08 20:34:55, tfarina wrote:
> nit: UI_BASE_WIN_SINGLETON_HWND_H_

Done.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:10: #include <vector>
On 2012/02/08 20:34:55, tfarina wrote:
> nit: I'd put a blank line between windows.h and vector to separate between C
> includes and C++ includes.

I've seen a different review go the other way - last I checked, the style guide
didn't mention anything either way - only about the order.

Scott, what do you think?

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:13: #include "base/basictypes.h"
On 2012/02/08 20:34:55, tfarina wrote:
> sort.

Done.

http://codereview.chromium.org/9359019/diff/2002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:16: namespace gfx {
On 2012/02/08 20:34:55, tfarina wrote:
> nit: namespace ui instead?

Done.

[Alexei Svitkine (slow), 2012-02-08 22:13:29]

From the comments on http://codereview.chromium.org/9323011/, it looks like
screen_compatible_dc_win.cc might not make sense anymore...

However, I still need the hwnd singleton for ClearType via the WM_SETTINGSCHANGE
message. I've verified that WM_SETTINGSCHANGE does actually get sent, unlike
WM_DISPLAYCHANGE.

So I think this split still makes sense - since I'll need to use the singleton
hwnd for WM_SETTINGSCHANGE...

[sky, 2012-02-08 23:34:04]

LGTM

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:39: SingletonHwnd();
this should be first.

[sky, 2012-02-08 23:34:28]

I'm adding Carlos to the review list, wait for him to sign off before landing.

[tfarina, 2012-02-08 23:35:50]

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:5: #ifndef UI_BASE_SINGLETON_HWND_H_
still wrong :)
UI_BASE_SINGLETON_HWND_H_ -> UI_BASE_WIN_SINGLETON_HWND_H_

[Alexei Svitkine (slow), 2012-02-09 15:11:43]

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:5: #ifndef UI_BASE_SINGLETON_HWND_H_
On 2012/02/08 23:35:50, tfarina wrote:
> still wrong :)
> UI_BASE_SINGLETON_HWND_H_ -> UI_BASE_WIN_SINGLETON_HWND_H_

Oops, you're right. Fixed.

http://codereview.chromium.org/9359019/diff/6002/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:39: SingletonHwnd();
On 2012/02/08 23:34:04, sky wrote:
> this should be first.

Done.

[cpu_(ooo_6.6-7.5), 2012-02-09 21:58:37]

I propose that you consider using chrome notifications. Chrome  I believe
already cares about the main XXXX_change messages.

http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd.cc
File ui/base/win/singleton_hwnd.cc (right):

http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:16: const wchar_t kWindowClassName[] =
L"SingletonHwnd";
too short. How about Chrome.SigletonHwnd or Chrome_SingletonHwnd ?

http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:24: return ::DefWindowProc(hwnd, message, wparam,
lparam);
hmm, kind of weird that you can get to act on the message but not set the return
value. A lot of useful actions require you to return a different value from what
DefWindowProc requires.

http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:41: ATOM clazz = ::RegisterClassEx(&wc);
I can smell the copy and paste :) please use var names in english, like
window_class or win_class or whatever.

http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:45: hinst, 0);
wait, this is also a message only window.. ??  you don't get broadcast messages.

[Alexei Svitkine (slow), 2012-02-13 16:42:18]

Since I'll be killing the screen_compatible_dc_win.cc part of this, I think I'll
just close this CL and add the singleton_hwnd.cc file to
http://codereview.chromium.org/9370026/.

I'll do another CL to kill screen_compatible_dc_win.cc and just cache the DC in
RenderTextWin per discussion on http://codereview.chromium.org/9323011/.

[Alexei Svitkine (slow), 2012-02-13 23:13:55]

I've moved the addition of singleton_hwnd.cc to
http://codereview.chromium.org/9370026/ with some comments addressed, but
replying here..

On 2012/02/09 21:58:37, cpu wrote:
> I propose that you consider using chrome notifications. Chrome  I believe
> already cares about the main XXXX_change messages.

Do you mean the mechanism that NativeWidgetWin and friends use, i.e. this?

http://code.google.com/codesearch#search/&exact_package=chromium&q=WM_SETTING...

I didn't go with that because it's provided by ATL and last I heard, we're
trying to cut out dependencies on ATL rather than introduce new ones...

>
http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd.cc
> File ui/base/win/singleton_hwnd.cc (right):
> 
>
http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
> ui/base/win/singleton_hwnd.cc:16: const wchar_t kWindowClassName[] =
> L"SingletonHwnd";
> too short. How about Chrome.SigletonHwnd or Chrome_SingletonHwnd ?

Done.


> 
>
http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
> ui/base/win/singleton_hwnd.cc:24: return ::DefWindowProc(hwnd, message,
wparam,
> lparam);
> hmm, kind of weird that you can get to act on the message but not set the
return
> value. A lot of useful actions require you to return a different value from
what
> DefWindowProc requires.

The ones that this change is interested in do not need them - if they're later
needed we could expand this helper to handle that..

> 
>
http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
> ui/base/win/singleton_hwnd.cc:41: ATOM clazz = ::RegisterClassEx(&wc);
> I can smell the copy and paste :) please use var names in english, like
> window_class or win_class or whatever.

Done.

>
http://codereview.chromium.org/9359019/diff/13002/ui/base/win/singleton_hwnd....
> ui/base/win/singleton_hwnd.cc:45: hinst, 0);
> wait, this is also a message only window.. ??  you don't get broadcast
messages.

For the new use case I need, in http://codereview.chromium.org/9323011/, this is
sufficient. I've added a comment to the top of the class to mention that it's a
message only window.