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

Issue 447853002: Battery Status API: implementation for Windows. (Closed)

Created:
6 years, 4 months ago by timvolodine
Modified:
6 years, 4 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, fmeawad, cpu_(ooo_6.6-7.5)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Battery Status API: implementation for Windows. Implementation of the Battery Status API for the Windows platform. Implementation uses a message window to receive battery notifications. On versions prior to Vista there is limited support as the RegisterPowerSettingNotification function is not available. BUG=122593, 404139 TEST=http://jsbin.com/battery-status-test (manual) Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289634 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290147

Patch Set 1 #

Total comments: 10

Patch Set 2 : fixed comments #

Patch Set 3 : added unit tests #

Total comments: 12

Patch Set 4 : comments #

Patch Set 5 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+316 lines, -0 lines) Patch
A content/browser/battery_status/battery_status_manager_win.h View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
A content/browser/battery_status/battery_status_manager_win.cc View 1 2 3 1 chunk +206 lines, -0 lines 0 comments Download
A content/browser/battery_status/battery_status_manager_win_unittest.cc View 1 2 1 chunk +80 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 32 (0 generated)
timvolodine
Hi guys, one more platform, this is the last one, please review so we can ...
6 years, 4 months ago (2014-08-06 18:51:07 UTC) #1
mlamouri (slow - plz ping)
lgtm but I would like someone who knows about Windows to have a look. Maybe ...
6 years, 4 months ago (2014-08-07 16:40:47 UTC) #2
timvolodine
Hi Scott, would you have any comments regarding windows specifics?
6 years, 4 months ago (2014-08-08 15:21:00 UTC) #3
scottmg
How does this relate to the work +fmeawad/+cpu were doing in https://code.google.com/p/chromium/issues/detail?id=153139 ? (i.e. should/does ...
6 years, 4 months ago (2014-08-08 21:32:19 UTC) #4
timvolodine
Regaring PowerMonitor -- it has a more limited scope, it watches for power state and ...
6 years, 4 months ago (2014-08-11 19:43:42 UTC) #5
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc#newcode143 content/browser/battery_status/battery_status_manager_win.cc:143: bool CreateMessageWindow() { do we need to create a ...
6 years, 4 months ago (2014-08-13 23:29:06 UTC) #6
scottmg
https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc#newcode115 content/browser/battery_status/battery_status_manager_win.cc:115: typedef HPOWERNOTIFY (WINAPI* RegisterPowerSettingNotificationFunc) per previous comment, you don't ...
6 years, 4 months ago (2014-08-13 23:39:52 UTC) #7
timvolodine
https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc#newcode115 content/browser/battery_status/battery_status_manager_win.cc:115: typedef HPOWERNOTIFY (WINAPI* RegisterPowerSettingNotificationFunc) On 2014/08/13 23:39:52, scottmg wrote: ...
6 years, 4 months ago (2014-08-14 15:34:34 UTC) #8
timvolodine
The CQ bit was checked by timvolodine@chromium.org
6 years, 4 months ago (2014-08-14 15:34:48 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/447853002/80001
6 years, 4 months ago (2014-08-14 15:35:42 UTC) #10
scottmg
On 2014/08/14 15:34:34, timvolodine wrote: > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc > File content/browser/battery_status/battery_status_manager_win.cc (right): > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc#newcode115 > ...
6 years, 4 months ago (2014-08-14 16:55:52 UTC) #11
commit-bot: I haz the power
Committed patchset #5 (80001) as 289634
6 years, 4 months ago (2014-08-14 17:30:15 UTC) #12
timvolodine
> https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_status/battery_status_manager_win.cc#newcode143 > > content/browser/battery_status/battery_status_manager_win.cc:143: bool > > CreateMessageWindow() { > > On 2014/08/13 23:29:06, ...
6 years, 4 months ago (2014-08-14 17:42:27 UTC) #13
cpu_(ooo_6.6-7.5)
why this got committed without lgtm from scott ? You need lgtm from all the ...
6 years, 4 months ago (2014-08-14 23:04:26 UTC) #14
jschuh
Today's canary is bricked on Win XP due to failure to load RegisterPowerSettingNotification. I assume ...
6 years, 4 months ago (2014-08-15 15:30:47 UTC) #15
timvolodine
On 2014/08/14 23:04:26, cpu (slow to respond) wrote: > why this got committed without lgtm ...
6 years, 4 months ago (2014-08-15 15:31:40 UTC) #16
jschuh
One more chance. This broke WinXP, so we either need a fix ASAP, or I ...
6 years, 4 months ago (2014-08-15 15:45:47 UTC) #17
timvolodine
On 2014/08/15 15:45:47, Justin Schuh wrote: > One more chance. This broke WinXP, so we ...
6 years, 4 months ago (2014-08-15 15:47:22 UTC) #18
timvolodine
On 2014/08/15 15:47:22, timvolodine wrote: > On 2014/08/15 15:45:47, Justin Schuh wrote: > > One ...
6 years, 4 months ago (2014-08-15 15:48:21 UTC) #19
jschuh
On 2014/08/15 15:48:21, timvolodine wrote: > On 2014/08/15 15:47:22, timvolodine wrote: > > On 2014/08/15 ...
6 years, 4 months ago (2014-08-15 15:57:21 UTC) #20
timvolodine
On 2014/08/15 15:57:21, Justin Schuh wrote: > On 2014/08/15 15:48:21, timvolodine wrote: > > On ...
6 years, 4 months ago (2014-08-15 15:59:44 UTC) #21
scottmg
On 2014/08/15 15:57:21, Justin Schuh wrote: > On 2014/08/15 15:48:21, timvolodine wrote: > > On ...
6 years, 4 months ago (2014-08-15 15:59:53 UTC) #22
Reid Kleckner
On 2014/08/15 15:59:44, timvolodine wrote: > On 2014/08/15 15:57:21, Justin Schuh wrote: > > On ...
6 years, 4 months ago (2014-08-15 16:01:28 UTC) #23
scottmg
On 2014/08/15 15:59:53, scottmg wrote: > On 2014/08/15 15:57:21, Justin Schuh wrote: > > On ...
6 years, 4 months ago (2014-08-15 16:03:04 UTC) #24
jschuh
On 2014/08/15 15:59:53, scottmg wrote: > On 2014/08/15 15:57:21, Justin Schuh wrote: > > On ...
6 years, 4 months ago (2014-08-15 16:06:53 UTC) #25
timvolodine
A revert of this CL (patchset #5) has been created in https://codereview.chromium.org/481493002/ by timvolodine@chromium.org. The ...
6 years, 4 months ago (2014-08-15 16:53:24 UTC) #26
erikwright (departed)
A revert of this CL (patchset #5) has been created in https://codereview.chromium.org/466223006/ by erikwright@chromium.org. The ...
6 years, 4 months ago (2014-08-15 19:21:14 UTC) #27
erikwright (departed)
On 2014/08/15 19:21:14, erikwright wrote: > A revert of this CL (patchset #5) has been ...
6 years, 4 months ago (2014-08-15 19:22:22 UTC) #28
timvolodine
The CQ bit was checked by timvolodine@chromium.org
6 years, 4 months ago (2014-08-16 09:32:29 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/447853002/80001
6 years, 4 months ago (2014-08-16 09:33:01 UTC) #30
timvolodine
On 2014/08/16 09:33:01, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
6 years, 4 months ago (2014-08-16 09:41:13 UTC) #31
commit-bot: I haz the power
6 years, 4 months ago (2014-08-16 14:41:01 UTC) #32
Message was sent while issue was closed.
Committed patchset #5 (80001) as 290147

Powered by Google App Engine
This is Rietveld 408576698