|
|
Created:
7 years, 11 months ago by Hongbo Min Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, tfarina, jam, willchan no longer on Chromium Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMove 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
Patch Set 1 #Patch Set 2 : add message only window into base/ #
Total comments: 10
Patch Set 3 : update per vandebo's comments #
Total comments: 16
Patch Set 4 : minor changes without scoped_ptr #Patch Set 5 : use scoped_ptr #
Total comments: 4
Patch Set 6 : rollback to the version of no scoped_ptr usage #
Total comments: 2
Patch Set 7 : use WS_EX_NOACTIVATE and WS_POPUP instead for window creation #
Messages
Total messages: 54 (0 generated)
ben@, could you please have a review. willchan@, vandebo@, the CL at https://codereview.chromium.org/10959020/ may be changed if this is fine.
ping ben@... Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/1
Retried try job too often on mac_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/1
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?
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.
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.
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.
adding cpu@...jam would like to defer to you on this issue. Thanks.
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.
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?
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.
jam@, do you still have concerns on this CL? If not, I will commit CL. Thanks.
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?
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
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... >
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...> > >
Ping. Any consensus here?
(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
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.
John and I are trying to hash this out. Stay tuned and sorry for the delay, there have been offsites and vacations for us.
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.
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
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.
hongbo: I think there's enough info to move forward here, right?
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?
ping vandebo@
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 ?
I'm less well versed in the Windows specific issues here...I'm going to punt to Brett.
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.
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!
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.
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
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?
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.
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.
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.
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.
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.
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.
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.
beng@, cpu@, willchan@, could you please have a review this CL again? Thanks.
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.
lgtm
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?
lgtm for views bits
ping brettw@
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/55009
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/55009
Message was sent while issue was closed.
Change committed as 185433 |