Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(77)

Issue 11821050: Move power event handling logic from ui/ to base/ on Windows (Closed)

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.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+102 lines, -32 lines) Patch
M base/system_monitor/system_monitor.h View 1 2 3 4 5 3 chunks +27 lines, -7 lines 0 comments Download
M base/system_monitor/system_monitor_win.cc View 1 2 3 4 5 6 2 chunks +75 lines, -14 lines 0 comments Download
M ui/views/win/hwnd_message_handler.h View 1 2 3 2 chunks +0 lines, -2 lines 0 comments Download
M ui/views/win/hwnd_message_handler.cc View 1 2 3 2 chunks +0 lines, -9 lines 0 comments Download

Messages

Total messages: 54 (0 generated)
Hongbo Min
ben@, could you please have a review. willchan@, vandebo@, the CL at https://codereview.chromium.org/10959020/ may be ...
7 years, 11 months ago (2013-01-10 14:31:02 UTC) #1
Hongbo Min
ping ben@... Thanks.
7 years, 11 months ago (2013-01-11 12:08:03 UTC) #2
Ben Goodger (Google)
lgtm
7 years, 11 months ago (2013-01-11 23:24:03 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/1
7 years, 11 months ago (2013-01-12 10:59:29 UTC) #4
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) browser_tests
7 years, 11 months ago (2013-01-12 12:40:27 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/1
7 years, 11 months ago (2013-01-13 03:49:35 UTC) #6
jam
It seems that putting this in either ui or content is a layering violation. an ...
7 years, 11 months ago (2013-01-13 05:03:43 UTC) #7
Hongbo Min
On 2013/01/13 05:03:43, John Abd-El-Malek wrote: > It seems that putting this in either ui ...
7 years, 11 months ago (2013-01-13 06:53:01 UTC) #8
vandebo (ex-Chrome)
On 2013/01/13 06:53:01, Hongbo Min wrote: > On 2013/01/13 05:03:43, John Abd-El-Malek wrote: > > ...
7 years, 11 months ago (2013-01-14 16:17:06 UTC) #9
jam
On 2013/01/14 16:17:06, vandebo wrote: > On 2013/01/13 06:53:01, Hongbo Min wrote: > > On ...
7 years, 11 months ago (2013-01-14 16:39:36 UTC) #10
Hongbo Min
adding cpu@...jam would like to defer to you on this issue. Thanks.
7 years, 11 months ago (2013-01-15 08:39:06 UTC) #11
cpu_(ooo_6.6-7.5)
Yes that was the problem win8 metro mode is not really friendly with windows that ...
7 years, 11 months ago (2013-01-15 23:57:08 UTC) #12
vandebo (ex-Chrome)
On 2013/01/15 23:57:08, cpu wrote: > Yes that was the problem win8 metro mode is ...
7 years, 11 months ago (2013-01-16 00:01:08 UTC) #13
Hongbo Min
cpu@, and I will merge your detailed explanation about why we have to do it ...
7 years, 11 months ago (2013-01-16 09:37:48 UTC) #14
Hongbo Min
jam@, do you still have concerns on this CL? If not, I will commit CL. ...
7 years, 11 months ago (2013-01-19 10:08:46 UTC) #15
jam
On 2013/01/15 23:57:08, cpu wrote: > Yes that was the problem win8 metro mode is ...
7 years, 11 months ago (2013-01-20 21:25:08 UTC) #16
cpu_(ooo_6.6-7.5)
Sure, I can elaborate. There are several things here. Grab a coffee and get a ...
7 years, 11 months ago (2013-01-20 22:48:29 UTC) #17
jam
On Sun, Jan 20, 2013 at 2:48 PM, <cpu@chromium.org> wrote: > Sure, I can elaborate. ...
7 years, 11 months ago (2013-01-20 23:18:36 UTC) #18
cpu_(ooo_6.6-7.5)
On 2013/01/20 23:18:36, John Abd-El-Malek wrote: > On Sun, Jan 20, 2013 at 2:48 PM, ...
7 years, 11 months ago (2013-01-22 23:44:01 UTC) #19
vandebo (ex-Chrome)
Ping. Any consensus here?
7 years, 11 months ago (2013-01-24 22:27:03 UTC) #20
jam
(i had an offline conversation with cpu earlier) the summary from my pov is as ...
7 years, 11 months ago (2013-01-25 02:09:52 UTC) #21
vandebo (ex-Chrome)
On 2013/01/25 02:09:52, John Abd-El-Malek wrote: > (i had an offline conversation with cpu earlier) ...
7 years, 11 months ago (2013-01-25 02:21:54 UTC) #22
cpu_(ooo_6.6-7.5)
John and I are trying to hash this out. Stay tuned and sorry for the ...
7 years, 11 months ago (2013-01-25 04:15:25 UTC) #23
cpu_(ooo_6.6-7.5)
Ok, we had a long conversation with John and we sort of have a rough ...
7 years, 10 months ago (2013-02-05 00:21:14 UTC) #24
tfarina
On Mon, Feb 4, 2013 at 10:21 PM, <cpu@chromium.org> wrote: > I also need to ...
7 years, 10 months ago (2013-02-05 00:25:37 UTC) #25
cpu_(ooo_6.6-7.5)
tfarina: What do you mean, power management notifications? I don't see that in its own ...
7 years, 10 months ago (2013-02-06 03:14:52 UTC) #26
vandebo (ex-Chrome)
hongbo: I think there's enough info to move forward here, right?
7 years, 10 months ago (2013-02-08 19:41:59 UTC) #27
Hongbo Min
On 2013/02/08 19:41:59, vandebo wrote: > hongbo: I think there's enough info to move forward ...
7 years, 10 months ago (2013-02-16 14:04:37 UTC) #28
Hongbo Min
ping vandebo@
7 years, 10 months ago (2013-02-20 05:32:31 UTC) #29
vandebo (ex-Chrome)
https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/system_monitor.h File base/system_monitor/system_monitor.h (right): https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/system_monitor.h#newcode154 base/system_monitor/system_monitor.h:154: PowerMessageWindow(SystemMonitor* monitor); explicit https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/system_monitor_win.cc File base/system_monitor/system_monitor_win.cc (right): https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/system_monitor_win.cc#newcode45 base/system_monitor/system_monitor_win.cc:45: ...
7 years, 10 months ago (2013-02-20 22:30:40 UTC) #30
willchan no longer on Chromium
I'm less well versed in the Windows specific issues here...I'm going to punt to Brett.
7 years, 10 months ago (2013-02-20 22:34:19 UTC) #31
brettw
I didn't yet think about the big picture and whether we need this. But looking ...
7 years, 10 months ago (2013-02-20 23:40:41 UTC) #32
Hongbo Min
ping jam@ https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/system_monitor.h File base/system_monitor/system_monitor.h (right): https://codereview.chromium.org/11821050/diff/38001/base/system_monitor/system_monitor.h#newcode154 base/system_monitor/system_monitor.h:154: PowerMessageWindow(SystemMonitor* monitor); On 2013/02/20 22:30:40, vandebo wrote: ...
7 years, 10 months ago (2013-02-21 07:39:08 UTC) #33
vandebo (ex-Chrome)
https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor.h File base/system_monitor/system_monitor.h (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor.h#newcode144 base/system_monitor/system_monitor.h:144: #if defined(OS_MACOSX) || defined(OS_WIN) Remove change - you don't ...
7 years, 10 months ago (2013-02-21 19:47:17 UTC) #34
cpu_(ooo_6.6-7.5)
looking good, https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc File base/system_monitor/system_monitor_win.cc (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc#newcode42 base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, ...
7 years, 10 months ago (2013-02-21 20:44:38 UTC) #35
vandebo (ex-Chrome)
https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc File base/system_monitor/system_monitor_win.cc (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc#newcode102 base/system_monitor/system_monitor_win.cc:102: GetWindowLongPtr(hwnd, GWLP_USERDATA)); On 2013/02/21 20:44:38, cpu wrote: > hmmm ...
7 years, 10 months ago (2013-02-21 20:54:44 UTC) #36
Hongbo Min
https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor.h File base/system_monitor/system_monitor.h (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor.h#newcode144 base/system_monitor/system_monitor.h:144: #if defined(OS_MACOSX) || defined(OS_WIN) On 2013/02/21 19:47:17, vandebo wrote: ...
7 years, 10 months ago (2013-02-22 06:55:43 UTC) #37
vandebo (ex-Chrome)
https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor.h File base/system_monitor/system_monitor.h (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor.h#newcode152 base/system_monitor/system_monitor.h:152: class PowerMessageWindow { On 2013/02/22 06:55:43, Hongbo Min wrote: ...
7 years, 10 months ago (2013-02-23 01:14:53 UTC) #38
Hongbo Min
https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/system_monitor.h File base/system_monitor/system_monitor.h (right): https://codereview.chromium.org/11821050/diff/45004/base/system_monitor/system_monitor.h#newcode152 base/system_monitor/system_monitor.h:152: class PowerMessageWindow; On 2013/02/23 01:14:53, vandebo wrote: > style ...
7 years, 10 months ago (2013-02-23 03:42:01 UTC) #39
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc File base/system_monitor/system_monitor_win.cc (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc#newcode42 base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_, ...
7 years, 10 months ago (2013-02-23 18:00:55 UTC) #40
Hongbo Min
https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc File base/system_monitor/system_monitor_win.cc (right): https://codereview.chromium.org/11821050/diff/41003/base/system_monitor/system_monitor_win.cc#newcode42 base/system_monitor/system_monitor_win.cc:42: 0, 0, 0, 0, 0, 0, 0, 0, instance_, ...
7 years, 10 months ago (2013-02-24 07:27:22 UTC) #41
vandebo (ex-Chrome)
Moved brett and jam to cc. Looks like you need final approval from Ben, cpu, ...
7 years, 10 months ago (2013-02-25 17:46:54 UTC) #42
Hongbo Min
On 2013/02/25 17:46:54, vandebo wrote: > I think Carlos would like you to do the ...
7 years, 10 months ago (2013-02-26 07:39:05 UTC) #43
Hongbo Min
beng@, cpu@, willchan@, could you please have a review this CL again? Thanks.
7 years, 10 months ago (2013-02-26 07:56:43 UTC) #44
willchan no longer on Chromium
On 2013/02/26 07:56:43, Hongbo Min wrote: > beng@, cpu@, willchan@, could you please have a ...
7 years, 10 months ago (2013-02-26 15:50:35 UTC) #45
cpu_(ooo_6.6-7.5)
lgtm
7 years, 10 months ago (2013-02-27 00:43:47 UTC) #46
Hongbo Min
On 2013/02/26 15:50:35, willchan wrote: > On 2013/02/26 07:56:43, Hongbo Min wrote: > > beng@, ...
7 years, 10 months ago (2013-02-27 01:19:19 UTC) #47
Ben Goodger (Google)
lgtm for views bits
7 years, 9 months ago (2013-02-27 18:28:05 UTC) #48
Hongbo Min
ping brettw@
7 years, 9 months ago (2013-02-28 14:04:01 UTC) #49
brettw
lgtm
7 years, 9 months ago (2013-02-28 23:20:58 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/55009
7 years, 9 months ago (2013-02-28 23:27:06 UTC) #51
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 9 months ago (2013-02-28 23:45:37 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11821050/55009
7 years, 9 months ago (2013-03-01 01:46:43 UTC) #53
commit-bot: I haz the power
7 years, 9 months ago (2013-03-01 03:20:33 UTC) #54
Message was sent while issue was closed.
Change committed as 185433

Powered by Google App Engine
This is Rietveld 408576698