Added base::win::InitializeWindowClass() wrapper to make sure that window classes are properly asso…

Added base::win::InitializeWindowClass() wrapper to make sure that window classes are properly associated with the modules containing their window procedures.

TEST=win,win_rel


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136116

[alexeypa (please no reviews), 2012-05-03 15:56:36]

Please take a look:

sky: chrome/browser/ui, content/browser, ui, ui/views
dcheng: ui/base/clipboard
sergeyu: remoting
brettw: base, content

This CL fixes several instances of improper usage of RegisterClass(Ex)()
function (this is Windows-only). The module handle passed to this function
should match the module containing windows procedure and not the main .EXE
module. Otherwise bad things like http://crbug.com/124091 can happen. 

Additional background on the issue:

- http://blogs.msdn.com/b/oldnewthing/archive/2005/04/18/409205.aspx
-
http://groups.google.com/a/chromium.org/group/chromium-dev/msg/aa9893b52e2344...

[sky, 2012-05-03 18:06:02]

Carlos should review this.

[Sergey Ulanov, 2012-05-03 18:07:14]

https://chromiumcodereview.appspot.com/10315012/diff/1/base/win/wrapped_windo...
File base/win/wrapped_window_proc.h (right):

https://chromiumcodereview.appspot.com/10315012/diff/1/base/win/wrapped_windo...
base/win/wrapped_window_proc.h:42: WNDCLASSEX* class_out,
Since this is an output parameter, shouldn't it be the last in the list?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[alexeypa (please no reviews), 2012-05-03 18:10:54]

https://chromiumcodereview.appspot.com/10315012/diff/1/base/win/wrapped_windo...
File base/win/wrapped_window_proc.h (right):

https://chromiumcodereview.appspot.com/10315012/diff/1/base/win/wrapped_windo...
base/win/wrapped_window_proc.h:42: WNDCLASSEX* class_out,
On 2012/05/03 18:07:14, sergeyu wrote:
> Since this is an output parameter, shouldn't it be the last in the list?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Yes, indeed. I'll fix that.

[alexeypa (please no reviews), 2012-05-04 18:10:50]

Ping?

[brettw, 2012-05-04 18:19:55]

base owners lgtm but I didn't really check the code.

[dcheng, 2012-05-04 19:40:31]

http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
File base/win/wrapped_window_proc.cc (right):

http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
base/win/wrapped_window_proc.cc:45: class_out->cbSize = sizeof(WNDCLASSEX);
Should we add a compile assert here so we don't forget to initialize members
that get added?

http://codereview.chromium.org/10315012/diff/8001/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/10315012/diff/8001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_win.cc:191: WNDCLASSEXW window_class;
Why change this from WNDCLASSEX to WNDCLASSEXW?

[alexeypa (please no reviews), 2012-05-04 19:54:23]

FYI.

http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
File base/win/wrapped_window_proc.cc (right):

http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
base/win/wrapped_window_proc.cc:45: class_out->cbSize = sizeof(WNDCLASSEX);
On 2012/05/04 19:40:31, dcheng wrote:
> Should we add a compile assert here so we don't forget to initialize members
> that get added?

WNDCLASSEX is a public API. It is set in stone and never changes.

It is possible however that a new structure extending WNDCLASSEX will be
introduced in the future (and say cbSize will be tested to see which structure
is passes to Win32 APIs). In such a case this function will support only the old
WNDCLASSEX structure, so it will be up to the extender of INitializeWindowClass
to take care of choosing between the old and new WNDCLASSEX.

http://codereview.chromium.org/10315012/diff/8001/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/10315012/diff/8001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_win.cc:191: WNDCLASSEXW window_class;
On 2012/05/04 19:40:31, dcheng wrote:
> Why change this from WNDCLASSEX to WNDCLASSEXW?

Unintentional. Reverted.

[brettw, 2012-05-04 20:00:55]

http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
File base/win/wrapped_window_proc.cc (right):

http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
base/win/wrapped_window_proc.cc:45: class_out->cbSize = sizeof(WNDCLASSEX);
I agree with no assert here.

[dcheng, 2012-05-04 20:02:25]

On 2012/05/04 20:00:55, brettw wrote:
>
http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
> File base/win/wrapped_window_proc.cc (right):
> 
>
http://codereview.chromium.org/10315012/diff/8001/base/win/wrapped_window_pro...
> base/win/wrapped_window_proc.cc:45: class_out->cbSize = sizeof(WNDCLASSEX);
> I agree with no assert here.

ui/base/clipboard LGTM.

[alexeypa (please no reviews), 2012-05-07 20:35:56]

Ping. I haven't yet got LGTMs for:

cpu or sky: chrome/browser/ui, content/browser, ui, ui/views
sergeyu: remoting

[sky, 2012-05-07 21:50:00]

cpu is the one that should review.

[cpu_(ooo_6.6-7.5), 2012-05-09 01:52:31]

I wrote a comment and then lgtm this a couple of days ago. I don't know what
happened...

Anyhow, lgtm and if I remember what my comment was I'll add it post check in.

[cpu_(ooo_6.6-7.5), 2012-05-09 01:56:08]

Ok, I recall now. My comment was: I hope we don't register any atl class via
this method. I looked and it looks like we don't.

The issue with ATL classes is that the wndproc is allocated on the heap so I am
not sure what the 'get-module-from-address' will do in this case.

[sky, 2012-05-09 04:09:24]

LGTM

https://chromiumcodereview.appspot.com/10315012/diff/16001/chrome/browser/ui/...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://chromiumcodereview.appspot.com/10315012/diff/16001/chrome/browser/ui/...
chrome/browser/ui/views/status_icons/status_tray_win.cc:29: DCHECK(atom_);
Make this a CHECK too.

https://chromiumcodereview.appspot.com/10315012/diff/16001/ui/views/controls/...
File ui/views/controls/menu/native_menu_win.cc (right):

https://chromiumcodereview.appspot.com/10315012/diff/16001/ui/views/controls/...
ui/views/controls/menu/native_menu_win.cc:113: DCHECK(clazz);
Make this a CHECK, otherwise we're going to fail.

[alexeypa (please no reviews), 2012-05-09 16:56:52]

FYI

https://chromiumcodereview.appspot.com/10315012/diff/16001/chrome/browser/ui/...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://chromiumcodereview.appspot.com/10315012/diff/16001/chrome/browser/ui/...
chrome/browser/ui/views/status_icons/status_tray_win.cc:29: DCHECK(atom_);
On 2012/05/09 04:09:25, sky wrote:
> Make this a CHECK too.

Done.

https://chromiumcodereview.appspot.com/10315012/diff/16001/ui/views/controls/...
File ui/views/controls/menu/native_menu_win.cc (right):

https://chromiumcodereview.appspot.com/10315012/diff/16001/ui/views/controls/...
ui/views/controls/menu/native_menu_win.cc:113: DCHECK(clazz);
On 2012/05/09 04:09:25, sky wrote:
> Make this a CHECK, otherwise we're going to fail.

Done.

[commit-bot: I haz the power, 2012-05-09 16:57:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10315012/17017

[commit-bot: I haz the power, 2012-05-09 20:38:21]

Change committed as 136116