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

Moved remoting::win::MessageWindow to base::win::MessageWindow so that it could be re-used outside of src/remoting.

BUG=241939
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207756

[alexeypa (please no reviews), 2013-06-11 18:20:23]

Hi Carlos,

This is another piece of https://chromiumcodereview.appspot.com/16020005/. This
CL moves MessageWindow to base::win. This CL addresses some of the concerns
raised in the other review.

> First off the message-only wrapper class is 99% the same motions
> you will have to do for any window, apart from like 4 values that
> you could provide in a constructor including HWND_MESSAGE.
> 
> Best here is to survey our window creation classes an unify this
> use into an existing one or if the existing ones are not up par
> then create one. On a different CL that is.

While this is true that the creation code for any kind of window is very
similar, I specifically didn't want to convert MessageWindow into a generic
window class. 

First, passing a registered window class to the constructor makes the calling
code more complex - it moves about half of the code out of MessageWindow class.
It could be a MessageWindowTraits object but, still, it is more complicated than
the current straightforward interface of MessageWindow.

Second, there is noticeable difference between modal and non-modal windows.
Having to support both would complicate the initialization in windows procedure.

Third, MessageWindow does not have anything to do with UI code most of the time.
It is typically used by some service code that needs to register to receive some
sort of notification. So I didn't want MessageWidnow to become something that
brings a "UI framework" dependency to other code.

WDYT?

[alexeypa (please no reviews), 2013-06-12 18:59:47]

ping

[alexeypa (please no reviews), 2013-06-14 15:14:04]

ping^2

[cpu_(ooo_6.6-7.5), 2013-06-14 18:17:27]

I've been thinking about this. I think we need to do a bit more re-organization
of code than this. You bring valid points so I am crafting an email to get all
parties on the same page.

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

can you change chromium/src/ui/base/clipboard/clipboard_win.cc
as well on this CL?

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

Looking good.

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc
File base/win/message_window.cc (right):

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc#ne...
base/win/message_window.cc:13: const wchar_t kClassNameFormat[] =
L"Chrome_MessageWindow_%p";
const string16

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc#ne...
base/win/message_window.cc:30: UnregisterClass(MAKEINTATOM(atom_),
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.

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc#ne...
base/win/message_window.cc:41: string16 class_name =
base::StringPrintf(kClassNameFormat, this);
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

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc#ne...
base/win/message_window.cc:101: }
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.

[cpu_(ooo_6.6-7.5), 2013-06-14 22:50:31]

ah one more, we should dcheck if destroywindow fails

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

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc
File base/win/message_window.cc (right):

https://codereview.chromium.org/16780006/diff/1/base/win/message_window.cc#ne...
base/win/message_window.cc:13: const wchar_t kClassNameFormat[] =
L"Chrome_MessageWindow_%p";
On 2013/06/14 22:49:32, cpu wrote:
> const string16

Wouldn't that introduce static intializer/finializer?

[alexeypa (please no reviews), 2013-06-17 15:39:06]

On 2013/06/14 22:22:33, cpu wrote:
> can you change chromium/src/ui/base/clipboard/clipboard_win.cc
> as well on this CL?

Here is how it looks: https://codereview.chromium.org/17285002

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

PTAL.

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
File base/win/message_window.cc (right):

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:13: const wchar_t kClassNameFormat[] =
L"Chrome_MessageWindow_%p";
On 2013/06/14 22:49:32, cpu wrote:
> const string16

Global constants are required to be POD types.

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:30: UnregisterClass(MAKEINTATOM(atom_),
On 2013/06/14 22:49:32, cpu wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:41: string16 class_name =
base::StringPrintf(kClassNameFormat, this);
On 2013/06/14 22:49:32, cpu wrote:
> 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

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.

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:101: }
On 2013/06/14 22:49:32, cpu wrote:
> 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.

Done.

[alexeypa (please no reviews), 2013-06-18 18:19:07]

Carlos, ping.

[cpu_(ooo_6.6-7.5), 2013-06-18 18:55:52]

I'll finish the review today. Its been a crazy week so far.

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

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
File base/win/message_window.cc (right):

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:13: const wchar_t kClassNameFormat[] =
L"Chrome_MessageWindow_%p";
On 2013/06/17 18:09:25, alexeypa wrote:
> On 2013/06/14 22:49:32, cpu wrote:
> > const string16
> 
> Global constants are required to be POD types.

yeah my bad.

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:30: UnregisterClass(MAKEINTATOM(atom_),
On 2013/06/17 18:09:25, alexeypa wrote:
> On 2013/06/14 22:49:32, cpu wrote:
> > 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.
> 
> 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. 
> 

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.

> 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.

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.

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:41: string16 class_name =
base::StringPrintf(kClassNameFormat, this);
On 2013/06/17 18:09:25, alexeypa wrote:
> On 2013/06/14 22:49:32, cpu wrote:
> > 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
> 
> 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 don't see how lazy class registration solves the problem. This is one process
finding another.

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

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

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
File base/win/message_window.cc (right):

https://chromiumcodereview.appspot.com/16780006/diff/1/base/win/message_windo...
base/win/message_window.cc:41: string16 class_name =
base::StringPrintf(kClassNameFormat, this);
On 2013/06/18 23:59:58, cpu wrote:
> I don't see how lazy class registration solves the problem. This is one
process
> finding another.

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.

[cpu_(ooo_6.6-7.5), 2013-06-19 00:15:02]

maybe there is another usage I am unaware, the usage I am aware of requires the
name of the window, which is the name of the profile. I guess we can throw in
the class atom in there but I don't think that would give us anything.

Anyhow, lgtm

[alexeypa (please no reviews), 2013-06-19 15:37:56]

On 2013/06/19 00:15:02, cpu wrote:
> maybe there is another usage I am unaware, the usage I am aware of requires
the
> name of the window, which is the name of the profile. I guess we can throw in
> the class atom in there but I don't think that would give us anything.

In chromium/src/chrome/browser/process_singleton_win.cc we pass the window class
name (which should be well known) and window name (profile path) to
FindWindowEx(HWND_MESSAGE, ...) to see if another chrome process created the
window already.

With MessageWindow the only part missing is a well-known class name. Right now
it is generated. Lazy instance allows to fix this - there will be a single
window class with a well known name. We will still need to pass the profile
directory to FindWindowEx just like it is done now.

[alexeypa (please no reviews), 2013-06-19 15:41:23]

OK, now I need an OWNER to rubber-stamp changes in src/base and src/remoting.
These are the same changes is in
https://chromiumcodereview.appspot.com/15261005/.

darin@ - PTAL at base/base.gyp and base/base.gypi
sergey@ - src/remoting.

[Sergey Ulanov, 2013-06-19 18:25:30]

lgtm

[cpu_(ooo_6.6-7.5), 2013-06-19 23:24:36]

I think is safe to TBR the gyp changes.

[commit-bot: I haz the power, 2013-06-20 21:31:35]

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

[darin (slow to review), 2013-06-20 21:33:57]

GYP changes LGTM

[commit-bot: I haz the power, 2013-06-20 22:18:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-20 22:30:53]

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

[commit-bot: I haz the power, 2013-06-20 23:37:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 00:20:38]

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

[commit-bot: I haz the power, 2013-06-21 08:49:25]

Change committed as 207756