Move power event handling logic from ui/ to base/ on Windows

Move power event handling logic from ui/ layer to base/ on Windows

This fix is to create a new window for receiving and handling power broadcast message
in base/ instead of in each HWNDMessageHandler instance. It addressed the issue of layer
violation in power message handling and make base/ self-contained.

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

[Hongbo Min, 2013-01-10 14:31:02]

ben@, could you please have a review.

willchan@, vandebo@, the CL at https://codereview.chromium.org/10959020/ may be
changed if this is fine.

[Hongbo Min, 2013-01-11 12:08:03]

ping ben@... Thanks.

[Ben Goodger (Google), 2013-01-11 23:24:03]

lgtm

[commit-bot: I haz the power, 2013-01-12 10:59:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/1

[commit-bot: I haz the power, 2013-01-12 12:40:27]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-13 03:49:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/1

[jam, 2013-01-13 05:03:43]

It seems that putting this in either ui or content is a layering violation. an
api in base should work on its own, without depending on a module at a higher
layer to forward messages to it.

if content can create its own system window which is separate from the browser,
why can't base create one as well?

[Hongbo Min, 2013-01-13 06:53:01]

On 2013/01/13 05:03:43, John Abd-El-Malek wrote:
> It seems that putting this in either ui or content is a layering violation. an
> api in base should work on its own, without depending on a module at a higher
> layer to forward messages to it.
> 
> if content can create its own system window which is separate from the
browser,
> why can't base create one as well?

I like jam's suggestion. If we can also create a message window in base for
power event handling, we could avoid to expose SystemMonitor::ProcessPowerEvent
method as public, and the client code of SystemMonitor doesn't need to know the
logic of how to process the power event.

[vandebo (ex-Chrome), 2013-01-14 16:17:06]

On 2013/01/13 06:53:01, Hongbo Min wrote:
> On 2013/01/13 05:03:43, John Abd-El-Malek wrote:
> > It seems that putting this in either ui or content is a layering violation.
an
> > api in base should work on its own, without depending on a module at a
higher
> > layer to forward messages to it.
> > 
> > if content can create its own system window which is separate from the
> browser,
> > why can't base create one as well?
> 
> I like jam's suggestion. If we can also create a message window in base for
> power event handling, we could avoid to expose
SystemMonitor::ProcessPowerEvent
> method as public, and the client code of SystemMonitor doesn't need to know
the
> logic of how to process the power event.

We're going in circles here people.  I suggested something like this what seems
like 6 months ago and it was shot down by Ben and Carlos.

[jam, 2013-01-14 16:39:36]

On 2013/01/14 16:17:06, vandebo wrote:
> On 2013/01/13 06:53:01, Hongbo Min wrote:
> > On 2013/01/13 05:03:43, John Abd-El-Malek wrote:
> > > It seems that putting this in either ui or content is a layering
violation.
> an
> > > api in base should work on its own, without depending on a module at a
> higher
> > > layer to forward messages to it.
> > > 
> > > if content can create its own system window which is separate from the
> > browser,
> > > why can't base create one as well?
> > 
> > I like jam's suggestion. If we can also create a message window in base for
> > power event handling, we could avoid to expose
> SystemMonitor::ProcessPowerEvent
> > method as public, and the client code of SystemMonitor doesn't need to know
> the
> > logic of how to process the power event.
> 
> We're going in circles here people.  I suggested something like this what
seems
> like 6 months ago and it was shot down by Ben and Carlos.

1) currently content creates its own top level window for this stuff. so having
one created in base/ as well didn't seem worse off than where we are today. cpu
is more knowledgeable about the win8 repercussions though, so I'll defer to him

2) in the bug I outlined one possible approach of having base normally create
its own window (i.e. like content does today), but having a way for a higher
level module (like chrome) to change this.

so it's unclear to me why this patch looks the way it does, since if 1) was shot
down by cpu/ben, this is still doing that but merely moves a layering violation
from one module to another.

[Hongbo Min, 2013-01-15 08:39:06]

adding cpu@...jam would like to defer to you on this issue. Thanks.

[cpu_(ooo_6.6-7.5), 2013-01-15 23:57:08]

Yes that was the problem win8 metro mode is not really friendly with windows
that are top level.

Initally we explored other things but none worked, so we are really left with a
window, if such needs to be created it really needs to be above at a place that
we can tweak it if necessary.

I think this change is an improvement, certainly I would love for this to be
self contained but windows does not seem to give us many choices. so lgtm.

[vandebo (ex-Chrome), 2013-01-16 00:01:08]

On 2013/01/15 23:57:08, cpu wrote:
> Yes that was the problem win8 metro mode is not really friendly with windows
> that are top level.
> 
> Initally we explored other things but none worked, so we are really left with
a
> window, if such needs to be created it really needs to be above at a place
that
> we can tweak it if necessary.
> 
> I think this change is an improvement, certainly I would love for this to be
> self contained but windows does not seem to give us many choices. so lgtm.

Carlos, can you say more about which approach you prefer?  Do you think this CL
with the layering violation is the best so far, or do you think a message window
in base with a mechanism to override/disable it would be better?

[Hongbo Min, 2013-01-16 09:37:48]

cpu@, and I will merge your detailed explanation about why we have to do it in
content/ instead of in base/ if thinking of win8 metro mode into the CL commit
log.

[Hongbo Min, 2013-01-19 10:08:46]

jam@, do you still have concerns on this CL? If not, I will commit CL. Thanks.

[jam, 2013-01-20 21:25:08]

On 2013/01/15 23:57:08, cpu wrote:
> Yes that was the problem win8 metro mode is not really friendly with windows
> that are top level.
> 
> Initally we explored other things but none worked, so we are really left with
a
> window, if such needs to be created it really needs to be above at a place
that
> we can tweak it if necessary.
> 
> I think this change is an improvement, certainly I would love for this to be
> self contained but windows does not seem to give us many choices. so lgtm.

Carlos: I don't see how this is an improvement? The top level window that's in
content is separate from the top level window for the chrome browser in metro.
if it is ok to have multiple top level windows (which exists with this patch),
then why wouldn't a third one in base be ok just like this one in content?

[cpu_(ooo_6.6-7.5), 2013-01-20 22:48:29]

Sure, I can elaborate. There are several things here. Grab a coffee and get a
good chair:

First off, if you create a top level window, in win8 metro we have to make sure
it has the right owner/parent, it needs to be the 'core window' [1] or chrome
itself (which is parented to the core window), it cannot be the desktop (aka
null).

This logic is enforced in src\chrome + src\ui because only chrome knows about
this window, note that this applies for the non-ash chrome on win8 in which all
our processes are in metro-mode [2].

This is a general rule that we don't have the manpower to check for every
possible use of a window [3]. I've fix numerous bugs and the only type we should
allow to be created unchecked are message-only windows [4].

Secondly, content_shell is not going to be made to run in metro mode so that is
a bit of an issue that we never wired src\content to expose the interfaces that
could be appropriate. Everything that knows about this stuff is in
metro_driver.dll, in src\ui and in src\chrome. There is a bit of that in
base\win\metro.h | .cc but none of that for tweaking the window.

I guess it could have been made better, but we know the current win8 metro
chrome was going to be a short lived hack.

Secondly I really don't like \base\ creating random ad-hoc windows, note for
example we also need to handle WM_DEVICECHANGE and surely other things will
appear [5]. All the sudden we have 3 different classes on base creating their
own pet window which of course needs to have a thread to pump messages so now we
have another 3 strange windows and 3 strange threads. Horrible.

What I've advocated in the past is that the application's window should handle
the notifications which can then pass to a \base class that can translate them
into generic power events (plus query functions to determine current power
state).

In other words the \base power class would not be self sufficient, it would rely
on upper layers providing the window and the thread. Is this a layer violation?
well sort of. But I've been told that 'windowless' power change notifications
are not possible [6].

Note that the second reason is more important in my mind that the first
(win8+metro) reason, I organized the material to end in the important one.

Ok that is all I have to say about it in general, I am open to any arrangement
that makes sense, hopefully this hour was well spent writing up the background
information. Btw, ATL handles this nicely via mixins [7] but the hell we are not
going back to that. Very painful to wean off from it.

Now back to this particular change the bad news and something I just assumed
without checking is that the window is not created using the src\ui helpers and
it is created in line 92 of system_message_window_win.cc so it is lacking the
tweaks that are needed in metro mode, so if this change sticks you are going to
have to wire that, see [2] for that.


[1] the core window is the HWND that sits behind this class:
http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.core.corewindow

[2]
https://code.google.com/p/chromium/source/search?q=GetWindowToParentTo&origq=...

[3] In particular what if it is top-level but invisible? no idea. But in general
if you are in metro mode and you create a desktop window, it is a 50/50 chance
that you are going to deadlock (as in you stop receiving input in metro mode).

[4] Search msdn for HWND_MESSAGE windows. We use them in our message loops.

[5] In fact I am a bit concerned that theme and settings change notifications
are not handled there.

[6] I think vandebo@ tried? memory fails me.

[7] There was a much better writeup abut mixins and ATL but seems gone from the
web, I found this http://itoshiki.wikispaces.com/ATL-MixIn

[jam, 2013-01-20 23:18:36]

On Sun, Jan 20, 2013 at 2:48 PM, <cpu@chromium.org> wrote:

> Sure, I can elaborate. There are several things here. Grab a coffee and
> get a
> good chair:
>
> First off, if you create a top level window, in win8 metro we have to make
> sure
> it has the right owner/parent, it needs to be the 'core window' [1] or
> chrome
> itself (which is parented to the core window), it cannot be the desktop
> (aka
> null).
>
> This logic is enforced in src\chrome + src\ui because only chrome knows
> about
> this window, note that this applies for the non-ash chrome on win8 in
> which all
> our processes are in metro-mode [2].
>
> This is a general rule that we don't have the manpower to check for every
> possible use of a window [3]. I've fix numerous bugs and the only type we
> should
> allow to be created unchecked are message-only windows [4].
>
> Secondly, content_shell is not going to be made to run in metro mode so
> that is
> a bit of an issue that we never wired src\content to expose the interfaces
> that
> could be appropriate.


I'm not sure I follow what content_shell has to do with
this? system_message_window_win.cc is used for any browser that's built on
content, i.e. for chrome as well as content_shell


> Everything that knows about this stuff is in
> metro_driver.dll, in src\ui and in src\chrome. There is a bit of that in
> base\win\metro.h | .cc but none of that for tweaking the window.
>
> I guess it could have been made better, but we know the current win8 metro
> chrome was going to be a short lived hack.
>
> Secondly I really don't like \base\ creating random ad-hoc windows, note
> for
> example we also need to handle WM_DEVICECHANGE and surely other things will
> appear [5]. All the sudden we have 3 different classes on base creating
> their
> own pet window which of course needs to have a thread to pump messages so
> now we
> have another 3 strange windows and 3 strange threads. Horrible.
>
> What I've advocated in the past is that the application's window should
> handle
> the notifications which can then pass to a \base class that can translate
> them
> into generic power events (plus query functions to determine current power
> state).
>
> In other words the \base power class would not be self sufficient, it
> would rely
> on upper layers providing the window and the thread. Is this a layer
> violation?
> well sort of. But I've been told that 'windowless' power change
> notifications
> are not possible [6].
>
> Note that the second reason is more important in my mind that the first
> (win8+metro) reason, I organized the material to end in the important one.
>
> Ok that is all I have to say about it in general, I am open to any
> arrangement
> that makes sense, hopefully this hour was well spent writing up the
> background
> information. Btw, ATL handles this nicely via mixins [7] but the hell we
> are not
> going back to that. Very painful to wean off from it.
>
> Now back to this particular change the bad news and something I just
> assumed
> without checking is that the window is not created using the src\ui
> helpers and
> it is created in line 92 of system_message_window_win.cc so it is lacking
> the
> tweaks that are needed in metro mode, so if this change sticks you are
> going to
> have to wire that, see [2] for that.
>

seems like that needs to be done either way, for whatever else is using
system_message_window_win.cc?

can base create its own window for this sort of stuff, just like content,
and parent it correctly using GetWindowToParentTo? (I guess that function
would then need to move to base/win/metro?). That would make base
self-sufficient; which seems like something we should aim for.


>
> [1] the core window is the HWND that sits behind this class:
> http://msdn.microsoft.com/en-**us/library/windows/apps/**
>
windows.ui.core.corewindow<http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.core.corewindow>
>
> [2]
> https://code.google.com/p/**chromium/source/search?q=**
>
GetWindowToParentTo&origq=**GetWindowToParentTo&btnG=**Search+Trunk<https://code.google.com/p/chromium/source/search?q=GetWindowToParentTo&origq=GetWindowToParentTo&btnG=Search+Trunk>
>
> [3] In particular what if it is top-level but invisible? no idea. But in
> general
> if you are in metro mode and you create a desktop window, it is a 50/50
> chance
> that you are going to deadlock (as in you stop receiving input in metro
> mode).
>
> [4] Search msdn for HWND_MESSAGE windows. We use them in our message loops.
>
> [5] In fact I am a bit concerned that theme and settings change
> notifications
> are not handled there.
>
> [6] I think vandebo@ tried? memory fails me.
>
> [7] There was a much better writeup abut mixins and ATL but seems gone
> from the
> web, I found this
http://itoshiki.wikispaces.**com/ATL-MixIn<http://itoshiki.wikispaces.com/ATL...
>
>
https://codereview.chromium.**org/11821050/<https://codereview.chromium.org/1...
>

[cpu_(ooo_6.6-7.5), 2013-01-22 23:44:01]

On 2013/01/20 23:18:36, John Abd-El-Malek wrote:
> On Sun, Jan 20, 2013 at 2:48 PM, <mailto:cpu@chromium.org> wrote:

> 
> I'm not sure I follow what content_shell has to do with
> this? system_message_window_win.cc is used for any browser that's built on
> content, i.e. for chrome as well as content_shell
> 

Yeah, I was talking about the rest of content shell. All the thing we did to
make chrome work on win8, some of them could have been don'e at the content
layer.


> 
> > Everything that knows about this stuff is in
> > metro_driver.dll, in src\ui and in src\chrome. There is a bit of that in
> > base\win\metro.h | .cc but none of that for tweaking the window.
> >
> > I guess it could have been made better, but we know the current win8 metro
> > chrome was going to be a short lived hack.
> >
> > Secondly I really don't like \base\ creating random ad-hoc windows, note
> > for
> > example we also need to handle WM_DEVICECHANGE and surely other things will
> > appear [5]. All the sudden we have 3 different classes on base creating
> > their
> > own pet window which of course needs to have a thread to pump messages so
> > now we
> > have another 3 strange windows and 3 strange threads. Horrible.
> >
> > What I've advocated in the past is that the application's window should
> > handle
> > the notifications which can then pass to a \base class that can translate
> > them
> > into generic power events (plus query functions to determine current power
> > state).
> >
> > In other words the \base power class would not be self sufficient, it
> > would rely
> > on upper layers providing the window and the thread. Is this a layer
> > violation?
> > well sort of. But I've been told that 'windowless' power change
> > notifications
> > are not possible [6].
> >
> > Note that the second reason is more important in my mind that the first
> > (win8+metro) reason, I organized the material to end in the important one.
> >
> > Ok that is all I have to say about it in general, I am open to any
> > arrangement
> > that makes sense, hopefully this hour was well spent writing up the
> > background
> > information. Btw, ATL handles this nicely via mixins [7] but the hell we
> > are not
> > going back to that. Very painful to wean off from it.
> >
> > Now back to this particular change the bad news and something I just
> > assumed
> > without checking is that the window is not created using the src\ui
> > helpers and
> > it is created in line 92 of system_message_window_win.cc so it is lacking
> > the
> > tweaks that are needed in metro mode, so if this change sticks you are
> > going to
> > have to wire that, see [2] for that.
> >
> 
> seems like that needs to be done either way, for whatever else is using
> system_message_window_win.cc?
> 
> can base create its own window for this sort of stuff, just like content,

Like I said, then you end up with a bunch of them. Otherwise you need to move
system_message_window_win.cc to base, or something similar.

> and parent it correctly using GetWindowToParentTo? (I guess that functio


> would then need to move to base/win/metro?). That would make base
> self-sufficient; which seems like something we should aim for.
> 

Sure, the remaining question is to figure out the right thread that
is going to pump these window or windows. I am assuming you don't advocate
base creating a thread for this use?



> 
> >
> > [1] the core window is the HWND that sits behind this class:
> > http://msdn.microsoft.com/en-**us/library/windows/apps/**
> >
>
windows.ui.core.corewindow<http://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.core.corewindow>
> >
> > [2]
> > https://code.google.com/p/**chromium/source/search?q=**
> >
>
GetWindowToParentTo&origq=**GetWindowToParentTo&btnG=**Search+Trunk<https://code.google.com/p/chromium/source/search?q=GetWindowToParentTo&origq=GetWindowToParentTo&btnG=Search+Trunk>
> >
> > [3] In particular what if it is top-level but invisible? no idea. But in
> > general
> > if you are in metro mode and you create a desktop window, it is a 50/50
> > chance
> > that you are going to deadlock (as in you stop receiving input in metro
> > mode).
> >
> > [4] Search msdn for HWND_MESSAGE windows. We use them in our message loops.
> >
> > [5] In fact I am a bit concerned that theme and settings change
> > notifications
> > are not handled there.
> >
> > [6] I think vandebo@ tried? memory fails me.
> >
> > [7] There was a much better writeup abut mixins and ATL but seems gone
> > from the
> > web, I found this
>
http://itoshiki.wikispaces.**com/ATL-MixIn%3Chttp://itoshiki.wikispaces.com/A...>
> >
> >
>
https://codereview.chromium.**org/11821050/%3Chttps://codereview.chromium.org...>
> >

[vandebo (ex-Chrome), 2013-01-24 22:27:03]

Ping.  Any consensus here?

[jam, 2013-01-25 02:09:52]

(i had an offline conversation with cpu earlier)
the summary from my pov is as before. the window in content is breaking the
issue with metro that Carlos described, so we need to fix that. also, i would
really like it if base was self sufficient. if we can move the
GetWindowToParentTo function to ui\base\metro so that the window there uses it,
that'd be my vote.

but as is, the current patch just makes base indirectly depend on content
instead of ui, which seems like something we should avoid

[vandebo (ex-Chrome), 2013-01-25 02:21:54]

On 2013/01/25 02:09:52, John Abd-El-Malek wrote:
> (i had an offline conversation with cpu earlier)
> the summary from my pov is as before. the window in content is breaking the
> issue with metro that Carlos described, so we need to fix that. also, i would
> really like it if base was self sufficient. if we can move the
> GetWindowToParentTo function to ui\base\metro so that the window there uses
it,
> that'd be my vote.
> 
> but as is, the current patch just makes base indirectly depend on content
> instead of ui, which seems like something we should avoid

From a quick look at the implementation of GetWindowToParentTo, looks like it
could move to base (but I'm no expert in this area), so that sounds like a plan.

[cpu_(ooo_6.6-7.5), 2013-01-25 04:15:25]

John and I are trying to hash this out. Stay tuned and sorry for the delay,
there have been offsites and vacations for us.

[cpu_(ooo_6.6-7.5), 2013-02-05 00:21:14]

Ok, we had a long conversation with John and we sort of have a rough idea here
on how to proceed.

I need to do a couple more tests to be able to tell you something that we are
not going to recant. Possibly tomorrow I can give you results.

I also need to check with another base owner because if things go johns' way
then we will be putting the whole power management in base.

[tfarina, 2013-02-05 00:25:37]

On Mon, Feb 4, 2013 at 10:21 PM,  <cpu@chromium.org> wrote:
> I also need to check with another base owner because if things go johns' way
> then we will be putting the whole power management in base.
>
base/ seems to be a place for low-level APIs (thread, mutex, lock,
message loop, file system, etc). Why it has to be in base? Can't it
live in its own toplevel directory? What is requiring it to live
there?

> https://codereview.chromium.org/11821050/



-- 
Thiago

[cpu_(ooo_6.6-7.5), 2013-02-06 03:14:52]

tfarina: What do you mean, power management notifications? I don't see that in
its own top level directory.

Others: I finished the investigation I was hoping to avoid last year. It is ok
that have a top level window in metro mode with the regular NULL parent as long
is not visible, size 0,0 and not activatable. That means that we don't have to
depend on the tweaks that only chrome needs to do, such as parenting to the
right corewindow.

This might break in win9 but to that I say, whatever.

That means that is possible to have an implementation that is fully contained in
base, we should move in that direction as opposed to have a split implementation
in the app and part in base.

I personally can go either way but jam@ has a strong opinion about this and
since it is now possible I this is how we should proceed.

To my other concern of having a single window for all notification I think we
can do this when the time comes that other notifications migrate to base.

[vandebo (ex-Chrome), 2013-02-08 19:41:59]

hongbo: I think there's enough info to move forward here, right?

[Hongbo Min, 2013-02-16 14:04:37]

On 2013/02/08 19:41:59, vandebo wrote:
> hongbo: I think there's enough info to move forward here, right?

I think so. I have updated the patch which makes system monitor self contained
by creating a message only window for power event handling. Am I correct in
understanding your comments?

[Hongbo Min, 2013-02-20 05:32:31]

ping vandebo@

[vandebo (ex-Chrome), 2013-02-20 22:30:40]

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor.h:154: PowerMessageWindow(SystemMonitor*
monitor);
explicit

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:45: void
SystemMonitor::PowerMessageWindow::InitMessageHWND() {
I think this can all go in the constructor.  It is in SystemMessageWindowClass. 
It's not in RemovableDeviceNotificationsWindowWin because in that case we need
the file thread to be running in order to examine the devices.

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:58: 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0,
instance_, 0);
Are you sure you want HWND_MESSAGE as the parent?  The other places that do this
pass 0 as the parent.

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:91:
monitor_->ProcessPowerMessage(power_event);
Can we just call SystemMonitor::Get() here, that way the PowerMessageWindow
doesn't need to take the SystemMonitor in the constructor. That further means
that it could be a direct member of SystemMonitor and you wouldn't need
PlatformInit/Destory.

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:105: return TRUE;
Should this also return ::DefWindowProc ?

[willchan no longer on Chromium, 2013-02-20 22:34:19]

I'm less well versed in the Windows specific issues here...I'm going to punt to
Brett.

[brettw, 2013-02-20 23:40:41]

I didn't yet think about the big picture and whether we need this. But looking
very briefly, it looks like we don't need the new class definition in the global
header and can have it be in an anon namespace in the _win.cc file, which will
make things cleaner.

[Hongbo Min, 2013-02-21 07:39:08]

ping jam@

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor.h:154: PowerMessageWindow(SystemMonitor*
monitor);
On 2013/02/20 22:30:40, vandebo wrote:
> explicit

No monitor parameter passed to ctor now.

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:45: void
SystemMonitor::PowerMessageWindow::InitMessageHWND() {
On 2013/02/20 22:30:40, vandebo wrote:
> I think this can all go in the constructor.  It is in
SystemMessageWindowClass. 
> It's not in RemovableDeviceNotificationsWindowWin because in that case we need
> the file thread to be running in order to examine the devices.

Make more sense. Done!

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:58: 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0,
instance_, 0);
On 2013/02/20 22:30:40, vandebo wrote:
> Are you sure you want HWND_MESSAGE as the parent?  The other places that do
this
> pass 0 as the parent.

I just saw MessagePumpForUI in message_pump_win.cc using HWND_MESSAGE to create
a message-only window.

After a double-check on CreateWindow function, I think it is not correct using
HWND_MESSAGE here. According to
http://msdn.microsoft.com/en-us/library/windows/desktop/ms632599(v=vs.85).asp...,
the message-only window has some special differences with a general window, for
example, it cannot be enumerated, and does not receive broadcast messages, and
simply dispatches messages.

So use 0 as parent instead.

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:91:
monitor_->ProcessPowerMessage(power_event);
On 2013/02/20 22:30:40, vandebo wrote:
> Can we just call SystemMonitor::Get() here, that way the PowerMessageWindow
> doesn't need to take the SystemMonitor in the constructor. That further means
> that it could be a direct member of SystemMonitor and you wouldn't need
> PlatformInit/Destory.

Done!

https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:105: return TRUE;
On 2013/02/20 22:30:40, vandebo wrote:
> Should this also return ::DefWindowProc ?

Good catch! Done!

[vandebo (ex-Chrome), 2013-02-21 19:47:17]

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:144: #if defined(OS_MACOSX) ||
defined(OS_WIN)
Remove change - you don't have PlatformInit/Desotry on Windows now.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:152: class PowerMessageWindow {
Brett's point was that this can be an anonymous class in the .cpp file.  You
just have to declare the class and use a scoped_ptr to it here.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:154: friend class SystemMonitor;
Looks like won't need this friend declaration after you make it anonymous
because you can make the ctor and dtor public.

[cpu_(ooo_6.6-7.5), 2013-02-21 20:44:38]

looking good,

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_,
0);
the createwindow call is unnecessarily terse, use NULL for the name using 0 for
the style is WS_OVERLAPPED which implies title and border which is weird, how
about
WS_POPUP? does that still work?

In fact try WS_POPUP and WS_EX_NOACTIVATE for the 'ex style'
see if that works.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:102: GetWindowLongPtr(hwnd,
GWLP_USERDATA));
hmmm danger danger

You can (and will) receive messages before line 43 is executed, you need to pass
those to defwindowproc

[vandebo (ex-Chrome), 2013-02-21 20:54:44]

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:102: GetWindowLongPtr(hwnd,
GWLP_USERDATA));
On 2013/02/21 20:44:38, cpu wrote:
> hmmm danger danger
> 
> You can (and will) receive messages before line 43 is executed, you need to
pass
> those to defwindowproc

Will message_hwnd be Null in that case?  If so, line 103 should catch that case,
right?

[Hongbo Min, 2013-02-22 06:55:43]

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:144: #if defined(OS_MACOSX) ||
defined(OS_WIN)
On 2013/02/21 19:47:17, vandebo wrote:
> Remove change - you don't have PlatformInit/Desotry on Windows now.

Done.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:152: class PowerMessageWindow {
On 2013/02/21 19:47:17, vandebo wrote:
> Brett's point was that this can be an anonymous class in the .cpp file.  You
> just have to declare the class and use a scoped_ptr to it here.

The problem is, if we just declare the class here and use a scoped_ptr to hold
it, the class MUST be defined in the system_monitor.cc using OS_WIN macro.
Otherwise, if it is defined in system_monitor_win.cc, we will get a compile
error saying 'use of undefined type' when compiling system_monitor.cc. The
forwarding class declaration should not cross 2 translation units, right?

So is it worthy to define the PowerMessageWindow in system_monitor.cc using
OS_WIN macro although a platform specific file system_monitor_win.cc is there?
See the PatchSet 5.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:154: friend class SystemMonitor;
On 2013/02/21 19:47:17, vandebo wrote:
> Looks like won't need this friend declaration after you make it anonymous
> because you can make the ctor and dtor public.

Actually, no need to make ctor and dtor as private since the inner class is also
declared as a private in SystemMonitor.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_,
0);
On 2013/02/21 20:44:38, cpu wrote:
> the createwindow call is unnecessarily terse, use NULL for the name using 0
for
> the style is WS_OVERLAPPED which implies title and border which is weird, how
> about
> WS_POPUP? does that still work?
> 
> In fact try WS_POPUP and WS_EX_NOACTIVATE for the 'ex style'
> see if that works.

I am a bit confused by your concerns about the title and border. Since the size
of the window is not (0, 0), which should be invisible. Why do we care about its
appearance?

And the code I referenced is from content/browser/system_message_window_win.cc

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:102: GetWindowLongPtr(hwnd,
GWLP_USERDATA));
On 2013/02/21 20:54:45, vandebo wrote:
> On 2013/02/21 20:44:38, cpu wrote:
> > hmmm danger danger
> > 
> > You can (and will) receive messages before line 43 is executed, you need to
> pass
> > those to defwindowproc
> 
> Will message_hwnd be Null in that case?  If so, line 103 should catch that
case,
> right?

I agree with Steve.

[vandebo (ex-Chrome), 2013-02-23 01:14:53]

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:152: class PowerMessageWindow {
On 2013/02/22 06:55:43, Hongbo Min wrote:
> On 2013/02/21 19:47:17, vandebo wrote:
> > Brett's point was that this can be an anonymous class in the .cpp file.  You
> > just have to declare the class and use a scoped_ptr to it here.
> 
> The problem is, if we just declare the class here and use a scoped_ptr to hold
> it, the class MUST be defined in the system_monitor.cc using OS_WIN macro.
> Otherwise, if it is defined in system_monitor_win.cc, we will get a compile
> error saying 'use of undefined type' when compiling system_monitor.cc. The
> forwarding class declaration should not cross 2 translation units, right?
> 
> So is it worthy to define the PowerMessageWindow in system_monitor.cc using
> OS_WIN macro although a platform specific file system_monitor_win.cc is there?
> See the PatchSet 5.
> 

Good point.  Put it in system_monitor_win.cc and use PlatformInit to create it.

https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/syste...
base/system_monitor/system_monitor.h:152: class PowerMessageWindow;
style guide says this should go on line 144.

https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/syste...
base/system_monitor/system_monitor.h:153: scoped_ptr<PowerMessageWindow>
power_message_window_;
style guide says this should go on line 185.

[Hongbo Min, 2013-02-23 03:42:01]

https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/syste...
base/system_monitor/system_monitor.h:152: class PowerMessageWindow;
On 2013/02/23 01:14:53, vandebo wrote:
> style guide says this should go on line 144.

Done.

https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/syste...
base/system_monitor/system_monitor.h:153: scoped_ptr<PowerMessageWindow>
power_message_window_;
On 2013/02/23 01:14:53, vandebo wrote:
> style guide says this should go on line 185.

Done.

https://codereview.chromium.org/11821050/diff/46003/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/46003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:147: class PowerMessageWindow {
Let me explain the reason why we need to define the class in the header file.

If we use forwarding declaration in combination with scoped_ptr, and define the
class in system_monitor_win.cc file, we will get compiling error saying 'use
undefined type" when compiling system_monitor.cc file. The scoped_ptr usage
can't span over 2 translation units (system_monitor.cc and
system_monitor_win.cc). 

It can be solved by defining the class in system_monitor.cc file, however, it is
still not a good idea to define a platform-specific class in the
system_monitor.cc, not in the system_monitor_win.cc.

[cpu_(ooo_6.6-7.5), 2013-02-23 18:00:55]

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_,
0);
On 2013/02/22 06:55:43, Hongbo Min wrote:
> On 2013/02/21 20:44:38, cpu wrote:
> > the createwindow call is unnecessarily terse, use NULL for the name using 0
> for
> > the style is WS_OVERLAPPED which implies title and border which is weird,
how
> > about
> > WS_POPUP? does that still work?
> > 
> > In fact try WS_POPUP and WS_EX_NOACTIVATE for the 'ex style'
> > see if that works.
> 
> I am a bit confused by your concerns about the title and border. Since the
size
> of the window is not (0, 0), which should be invisible. Why do we care about
its
> appearance?

Because the right flags signal your intention. And we don't use 0 for NULL.

On top of that you get all the nonclient area messages, since you said you have
a full featured window.

> 
> And the code I referenced is from content/browser/system_message_window_win.cc
> 

Yeah, still wrong. No need to cargo cult that into other code.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:102: GetWindowLongPtr(hwnd,
GWLP_USERDATA));
On 2013/02/22 06:55:43, Hongbo Min wrote:
> On 2013/02/21 20:54:45, vandebo wrote:
> > On 2013/02/21 20:44:38, cpu wrote:
> > > hmmm danger danger
> > > 


I agree too. I must have misread it.
> > > You can (and will) receive messages before line 43 is executed, you need
to
> > pass
> > > those to defwindowproc
> > 
> > Will message_hwnd be Null in that case?  If so, line 103 should catch that
> case,
> > right?
> 
> I agree with Steve.

[Hongbo Min, 2013-02-24 07:27:22]

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_,
0);
cpu@, I did an experiment to verify the concerns you mentioned by writing a
small program using CreateWindow with different styles. Let me summarize the
findings to you.

1) The title and border won't be showed if ShowWindow is not called, regardless
of the window size and styles.

2) If the window size is (0, 0) and ShowWindow gets called, the title and border
become weird, as you said.

3) By use of WM_EX_NOACTIVATE ex style, the window won't be showed on the task
bar even if ShowWindow gets called.

4) By use of WM_POPUP, no title and border is showed if ShowWindow gets called.

5) All the nonclient area messages are received even if using WM_EX_NOACTIVATE
and WM_POPUP. I create two windows for testing, one is using WS_OVERLAPPEDWINDOW
(called A), the one is using WM_EX_NOACTIVATE and WM_POPUP (called B), according
to the output, the latter one could receive all nonclient area messages from the
former one, for example, if I resize the A window, B window can still receive
the nonclient area messages arose from resizing.

So if not calling ShowWindow on the hwnd, there is no obvious benefit from using
WM_EX_NOACTIVATE + WS_POPUP although it is a more cleaner and stricter way.

[vandebo (ex-Chrome), 2013-02-25 17:46:54]

Moved brett and jam to cc.

Looks like you need final approval from Ben, cpu, and Will.

Aside from cpu's comment about window arguments, LGTM.

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
File base/system_monitor/system_monitor_win.cc (right):

https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/syste...
base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_,
0);
On 2013/02/24 07:27:22, Hongbo Min wrote:
> cpu@, I did an experiment to verify the concerns you mentioned by writing a
> small program using CreateWindow with different styles. Let me summarize the
> findings to you.
> 
> 1) The title and border won't be showed if ShowWindow is not called,
regardless
> of the window size and styles.
> 
> 2) If the window size is (0, 0) and ShowWindow gets called, the title and
border
> become weird, as you said.
> 
> 3) By use of WM_EX_NOACTIVATE ex style, the window won't be showed on the task
> bar even if ShowWindow gets called.
> 
> 4) By use of WM_POPUP, no title and border is showed if ShowWindow gets
called.
> 
> 5) All the nonclient area messages are received even if using WM_EX_NOACTIVATE
> and WM_POPUP. I create two windows for testing, one is using
WS_OVERLAPPEDWINDOW
> (called A), the one is using WM_EX_NOACTIVATE and WM_POPUP (called B),
according
> to the output, the latter one could receive all nonclient area messages from
the
> former one, for example, if I resize the A window, B window can still receive
> the nonclient area messages arose from resizing.
> 
> So if not calling ShowWindow on the hwnd, there is no obvious benefit from
using
> WM_EX_NOACTIVATE + WS_POPUP although it is a more cleaner and stricter way.

I think Carlos would like you to do the clearer thing.

https://codereview.chromium.org/11821050/diff/46003/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/11821050/diff/46003/base/system_monitor/syste...
base/system_monitor/system_monitor.h:147: class PowerMessageWindow {
On 2013/02/23 03:42:01, Hongbo Min wrote:
> Let me explain the reason why we need to define the class in the header file.
> 
> If we use forwarding declaration in combination with scoped_ptr, and define
the
> class in system_monitor_win.cc file, we will get compiling error saying 'use
> undefined type" when compiling system_monitor.cc file. The scoped_ptr usage
> can't span over 2 translation units (system_monitor.cc and
> system_monitor_win.cc). 
> 
> It can be solved by defining the class in system_monitor.cc file, however, it
is
> still not a good idea to define a platform-specific class in the
> system_monitor.cc, not in the system_monitor_win.cc.
> 

I see.  This can be solved by making SystemMonitor a base class and putting the
implementation in platform specific classes that inherit from SystemMonitor. 
But that can be done later.

[Hongbo Min, 2013-02-26 07:39:05]

On 2013/02/25 17:46:54, vandebo wrote:
> I think Carlos would like you to do the clearer thing.

Done!

> I see.  This can be solved by making SystemMonitor a base class and putting
the
> implementation in platform specific classes that inherit from SystemMonitor. 
> But that can be done later.

Agree.

[Hongbo Min, 2013-02-26 07:56:43]

beng@, cpu@, willchan@, could you please have a review this CL again? Thanks.

[willchan no longer on Chromium, 2013-02-26 15:50:35]

On 2013/02/26 07:56:43, Hongbo Min wrote:
> beng@, cpu@, willchan@, could you please have a review this CL again? Thanks.

I punted to Brett earlier since this at first glance appeared more Windows
specific than I was comfortable with. Let me know if that's not true and I'll be
happy to look. Otherwise, I'm going to continue to defer to Brett here.

[cpu_(ooo_6.6-7.5), 2013-02-27 00:43:47]

lgtm

[Hongbo Min, 2013-02-27 01:19:19]

On 2013/02/26 15:50:35, willchan wrote:
> On 2013/02/26 07:56:43, Hongbo Min wrote:
> > beng@, cpu@, willchan@, could you please have a review this CL again?
Thanks.
> 
> I punted to Brett earlier since this at first glance appeared more Windows
> specific than I was comfortable with. Let me know if that's not true and I'll
be
> happy to look. Otherwise, I'm going to continue to defer to Brett here.

Brett, your thought with this CL now?

[Ben Goodger (Google), 2013-02-27 18:28:05]

lgtm for views bits

[Hongbo Min, 2013-02-28 14:04:01]

ping brettw@

[brettw, 2013-02-28 23:20:58]

lgtm

[commit-bot: I haz the power, 2013-02-28 23:27:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/55009

[commit-bot: I haz the power, 2013-02-28 23:45:37]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
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-03-01 01:46:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/55009

[commit-bot: I haz the power, 2013-03-01 03:20:33]

Change committed as 185433