|
|
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. |
DescriptionBattery 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 #
Messages
Total messages: 32 (0 generated)
Hi guys, one more platform, this is the last one, please review so we can ship in M38! I'll probably add some unit tests similarly to how I did for Linux. thanks, Tim
lgtm but I would like someone who knows about Windows to have a look. Maybe scottmg@? https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:25: status.charging = !on_battery; // not offline Couldn't you do: status.charging = win_status.ACLineStatus != 0; Then use |!status.charging| instead of |on_battery| ? https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:41: status.chargingTime = std::numeric_limits<double>::infinity(); Can't you use status.BatteryFullLifeTime? https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:154: return; If that means that we don't get any event before Vista (ie. XP) unless the battery mode is toggled, that's a problem. I wouldn't block shipping this in M38 just for that but we need to fix that in a follow-up. Could you open a bug and mark it as blocking bug 122593. We should consider polling at a very high interval like 30s to 5 minutes.
Hi Scott, would you have any comments regarding windows specifics?
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 PowerMonitor already know the battery status?) https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:156: HMODULE user32 = GetModuleHandleA("user32.dll"); I believe user32 is delayload, so you can just runtime-guard this, but call the functions normally and avoid the extra members. https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:164: bool CreateMessageWindow() { can we use base::win::MessageWindow here?
Regaring PowerMonitor -- it has a more limited scope, it watches for power state and suspend/resume events. Battery Status API watches at a more detailed level, e.g. battery level changes. Also the latter API is on-demand while power monitor is always there. https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:25: status.charging = !on_battery; // not offline On 2014/08/07 16:40:47, Mounir Lamouri - OOO back 15th wrote: > Couldn't you do: > status.charging = win_status.ACLineStatus != 0; > > Then use |!status.charging| instead of |on_battery| ? Done. https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:41: status.chargingTime = std::numeric_limits<double>::infinity(); On 2014/08/07 16:40:47, Mounir Lamouri - OOO back 15th wrote: > Can't you use status.BatteryFullLifeTime? according to the documentation BatteryFullLifeTime is "The number of seconds of battery life when at full charge", so not usable for the purpose of chargingTime. https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:154: return; On 2014/08/07 16:40:47, Mounir Lamouri - OOO back 15th wrote: > If that means that we don't get any event before Vista (ie. XP) unless the > battery mode is toggled, that's a problem. I wouldn't block shipping this in M38 > just for that but we need to fix that in a follow-up. > > Could you open a bug and mark it as blocking bug 122593. > > We should consider polling at a very high interval like 30s to 5 minutes. Done. added a crbug. https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:156: HMODULE user32 = GetModuleHandleA("user32.dll"); On 2014/08/08 21:32:19, scottmg wrote: > I believe user32 is delayload, so you can just runtime-guard this, but call the > functions normally and avoid the extra members. Done. https://codereview.chromium.org/447853002/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_win.cc:164: bool CreateMessageWindow() { On 2014/08/08 21:32:19, scottmg wrote: > can we use base::win::MessageWindow here? yes. thanks for noticing this, done.
https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:143: bool CreateMessageWindow() { do we need to create a new window? I thought we already have a window singleton that you can subscribe for messages?
https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:115: typedef HPOWERNOTIFY (WINAPI* RegisterPowerSettingNotificationFunc) per previous comment, you don't need GetProcAddress, just call RegisterPowerSettingNotification(...). the runtime guard at 112 is enough. https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:118: RegisterPowerSettingNotificationFunc register_func_ = (don't need it, but no trailing _ on locals) https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:135: UnregisterPowerSettingNotificationFunc unregister_func_ = same here https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_win.h (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.h:4: #ifndef CHROME_BROWSER_BATTERY_STATUS_BATTERY_STATUS_MANAGER_WIN_H_ nit; add \n https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.h:21: const SYSTEM_POWER_STATUS& win_status); nit; 4s indent
https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_win.cc (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:115: typedef HPOWERNOTIFY (WINAPI* RegisterPowerSettingNotificationFunc) On 2014/08/13 23:39:52, scottmg wrote: > per previous comment, you don't need GetProcAddress, just call > RegisterPowerSettingNotification(...). the runtime guard at 112 is enough. great, it magically works :) thanks. Done. https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:118: RegisterPowerSettingNotificationFunc register_func_ = On 2014/08/13 23:39:52, scottmg wrote: > (don't need it, but no trailing _ on locals) Done. https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:135: UnregisterPowerSettingNotificationFunc unregister_func_ = On 2014/08/13 23:39:52, scottmg wrote: > same here Done. https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.cc:143: bool CreateMessageWindow() { On 2014/08/13 23:29:06, cpu (slow to respond) wrote: > do we need to create a new window? I thought we already have a window singleton > that you can subscribe for messages? not sure what window singleton you are referring to, but we may be able to reuse the power_monitor message window. I'll need to check this because last time I experimented with power_monitor it seemed that it can be not initialized yet at the start of battery status api.. added a TODO. https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_win.h (right): https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.h:4: #ifndef CHROME_BROWSER_BATTERY_STATUS_BATTERY_STATUS_MANAGER_WIN_H_ On 2014/08/13 23:39:52, scottmg wrote: > nit; add \n Done. https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_win.h:21: const SYSTEM_POWER_STATUS& win_status); On 2014/08/13 23:39:52, scottmg wrote: > nit; 4s indent Done.
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/447853002/80001
On 2014/08/14 15:34:34, timvolodine wrote: > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > File content/browser/battery_status/battery_status_manager_win.cc (right): > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_win.cc:115: typedef > HPOWERNOTIFY (WINAPI* RegisterPowerSettingNotificationFunc) > On 2014/08/13 23:39:52, scottmg wrote: > > per previous comment, you don't need GetProcAddress, just call > > RegisterPowerSettingNotification(...). the runtime guard at 112 is enough. > > great, it magically works :) thanks. > Done. > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_win.cc:118: > RegisterPowerSettingNotificationFunc register_func_ = > On 2014/08/13 23:39:52, scottmg wrote: > > (don't need it, but no trailing _ on locals) > > Done. > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_win.cc:135: > UnregisterPowerSettingNotificationFunc unregister_func_ = > On 2014/08/13 23:39:52, scottmg wrote: > > same here > > Done. > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_win.cc:143: bool > CreateMessageWindow() { > On 2014/08/13 23:29:06, cpu (slow to respond) wrote: > > do we need to create a new window? I thought we already have a window > singleton > > that you can subscribe for messages? > > not sure what window singleton you are referring to, but we may be able to reuse > the power_monitor message window. I'll need to check this because last time I > experimented with power_monitor it seemed that it can be not initialized yet at > the start of battery status api.. added a TODO. Presumably this one https://code.google.com/p/chromium/codesearch#search/&q=singleton%20hwnd&sq=p... which seems better. > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > File content/browser/battery_status/battery_status_manager_win.h (right): > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_win.h:4: #ifndef > CHROME_BROWSER_BATTERY_STATUS_BATTERY_STATUS_MANAGER_WIN_H_ > On 2014/08/13 23:39:52, scottmg wrote: > > nit; add \n > > Done. > > https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_win.h:21: const > SYSTEM_POWER_STATUS& win_status); > On 2014/08/13 23:39:52, scottmg wrote: > > nit; 4s indent > > Done.
Message was sent while issue was closed.
Committed patchset #5 (80001) as 289634
Message was sent while issue was closed.
> https://codereview.chromium.org/447853002/diff/40001/content/browser/battery_... > > content/browser/battery_status/battery_status_manager_win.cc:143: bool > > CreateMessageWindow() { > > On 2014/08/13 23:29:06, cpu (slow to respond) wrote: > > > do we need to create a new window? I thought we already have a window > > singleton > > > that you can subscribe for messages? > > > > not sure what window singleton you are referring to, but we may be able to > reuse > > the power_monitor message window. I'll need to check this because last time I > > experimented with power_monitor it seemed that it can be not initialized yet > at > > the start of battery status api.. added a TODO. > > Presumably this one > https://code.google.com/p/chromium/codesearch#search/&q=singleton%20hwnd&sq=p... > which seems better. > ah, I see. I'll address this in a follow-up patch. Thanks for the link.
Message was sent while issue was closed.
why this got committed without lgtm from scott ? You need lgtm from all the reviewers or they can say that they trust some other reviewer.
Message was sent while issue was closed.
Today's canary is bricked on Win XP due to failure to load RegisterPowerSettingNotification. I assume this is the culprit, but I'll give a chance to respond before I revert it.
Message was sent while issue was closed.
On 2014/08/14 23:04:26, cpu (slow to respond) wrote: > why this got committed without lgtm from scott ? > > You need lgtm from all the reviewers or they can say that they trust some other > reviewer. sorry, I thought I addressed all comments as the patch has been around for some time. I've created a patch using SingletonHwnd as suggested: https://codereview.chromium.org/480503002/. Please have a look.
Message was sent while issue was closed.
One more chance. This broke WinXP, so we either need a fix ASAP, or I will be forced to revert.
Message was sent while issue was closed.
On 2014/08/15 15:45:47, Justin Schuh wrote: > One more chance. This broke WinXP, so we either need a fix ASAP, or I will be > forced to revert. yes RegisterPowerSettingNotification is this patch. According to scottmg it was safe to simple have a VISTA runtime guard. scottmg: any ideas?
Message was sent while issue was closed.
On 2014/08/15 15:47:22, timvolodine wrote: > On 2014/08/15 15:45:47, Justin Schuh wrote: > > One more chance. This broke WinXP, so we either need a fix ASAP, or I will be > > forced to revert. > > yes RegisterPowerSettingNotification is this patch. According to scottmg it was > safe to simple have a VISTA runtime guard. > scottmg: any ideas? Justin doe you have a link with the breakage?
Message was sent while issue was closed.
On 2014/08/15 15:48:21, timvolodine wrote: > On 2014/08/15 15:47:22, timvolodine wrote: > > On 2014/08/15 15:45:47, Justin Schuh wrote: > > > One more chance. This broke WinXP, so we either need a fix ASAP, or I will > be > > > forced to revert. > > > > yes RegisterPowerSettingNotification is this patch. According to scottmg it > was > > safe to simple have a VISTA runtime guard. > > scottmg: any ideas? You can't rely on delay loads for user32. You have to manually LoadLibrary and GetProcAddress for RegisterPowerSettingNotification and UnregisterPowerSettingNotification. That's why it's dying in the loader (because the import is failing). > > Justin doe you have a link with the breakage? Nope, I think all the WinXP bots have been removed (so much for those 250mil users). I just ran into this because I was trying to test a CL on XP this morning and it won't start.
Message was sent while issue was closed.
On 2014/08/15 15:57:21, Justin Schuh wrote: > On 2014/08/15 15:48:21, timvolodine wrote: > > On 2014/08/15 15:47:22, timvolodine wrote: > > > On 2014/08/15 15:45:47, Justin Schuh wrote: > > > > One more chance. This broke WinXP, so we either need a fix ASAP, or I will > > be > > > > forced to revert. > > > > > > yes RegisterPowerSettingNotification is this patch. According to scottmg it > > was > > > safe to simple have a VISTA runtime guard. > > > scottmg: any ideas? > > You can't rely on delay loads for user32. You have to manually LoadLibrary and > GetProcAddress for RegisterPowerSettingNotification and > UnregisterPowerSettingNotification. That's why it's dying in the loader (because > the import is failing). > > > > > Justin doe you have a link with the breakage? > > Nope, I think all the WinXP bots have been removed (so much for those 250mil > users). I just ran into this because I was trying to test a CL on XP this > morning and it won't start. I see, so it's a runtime problem right? (not linkage related)
Message was sent while issue was closed.
On 2014/08/15 15:57:21, Justin Schuh wrote: > On 2014/08/15 15:48:21, timvolodine wrote: > > On 2014/08/15 15:47:22, timvolodine wrote: > > > On 2014/08/15 15:45:47, Justin Schuh wrote: > > > > One more chance. This broke WinXP, so we either need a fix ASAP, or I will > > be > > > > forced to revert. > > > > > > yes RegisterPowerSettingNotification is this patch. According to scottmg it > > was > > > safe to simple have a VISTA runtime guard. > > > scottmg: any ideas? > > You can't rely on delay loads for user32. You have to manually LoadLibrary and > GetProcAddress for RegisterPowerSettingNotification and > UnregisterPowerSettingNotification. That's why it's dying in the loader (because > the import is failing). Really? I had no idea. user32 is special? Well, I suck. Sorry about that Tim. You'll need to switch back to the previous ps I guess. > > > > > Justin doe you have a link with the breakage? > > Nope, I think all the WinXP bots have been removed (so much for those 250mil > users). I just ran into this because I was trying to test a CL on XP this > morning and it won't start.
Message was sent while issue was closed.
On 2014/08/15 15:59:44, timvolodine wrote: > On 2014/08/15 15:57:21, Justin Schuh wrote: > > On 2014/08/15 15:48:21, timvolodine wrote: > > > On 2014/08/15 15:47:22, timvolodine wrote: > > > > On 2014/08/15 15:45:47, Justin Schuh wrote: > > > > > One more chance. This broke WinXP, so we either need a fix ASAP, or I > will > > > be > > > > > forced to revert. > > > > > > > > yes RegisterPowerSettingNotification is this patch. According to scottmg > it > > > was > > > > safe to simple have a VISTA runtime guard. > > > > scottmg: any ideas? > > > > You can't rely on delay loads for user32. You have to manually LoadLibrary and > > GetProcAddress for RegisterPowerSettingNotification and > > UnregisterPowerSettingNotification. That's why it's dying in the loader > (because > > the import is failing). > > > > > > > > Justin doe you have a link with the breakage? > > > > Nope, I think all the WinXP bots have been removed (so much for those 250mil > > users). I just ran into this because I was trying to test a CL on XP this > > morning and it won't start. > > I see, so it's a runtime problem right? > (not linkage related) This is manifesting as a runtime failure for Clang-built binaries on Win7, and a link failure with Clang ASan on Win7. I don't know what's different about our configurations, but it would be great to get this fixed. I assume the fix is to use GetProcAddress and function pointers.
Message was sent while issue was closed.
On 2014/08/15 15:59:53, scottmg wrote: > On 2014/08/15 15:57:21, Justin Schuh wrote: > > On 2014/08/15 15:48:21, timvolodine wrote: > > > On 2014/08/15 15:47:22, timvolodine wrote: > > > > On 2014/08/15 15:45:47, Justin Schuh wrote: > > > > > One more chance. This broke WinXP, so we either need a fix ASAP, or I > will > > > be > > > > > forced to revert. > > > > > > > > yes RegisterPowerSettingNotification is this patch. According to scottmg > it > > > was > > > > safe to simple have a VISTA runtime guard. > > > > scottmg: any ideas? > > > > You can't rely on delay loads for user32. You have to manually LoadLibrary and > > GetProcAddress for RegisterPowerSettingNotification and > > UnregisterPowerSettingNotification. That's why it's dying in the loader > (because > > the import is failing). > > Really? I had no idea. user32 is special? Well, I suck. Sorry about that Tim. > You'll need to switch back to the previous ps I guess. > > > > > > > > > Justin doe you have a link with the breakage? > > > > Nope, I think all the WinXP bots have been removed (so much for those 250mil > > users). I just ran into this because I was trying to test a CL on XP this > > morning and it won't start. Wait, but why isn't everything failing on the XP bots then? http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20(2) There's some seemingly minor telemetry tests, but wouldn't everything fail to load if it was lacking a function?
Message was sent while issue was closed.
On 2014/08/15 15:59:53, scottmg wrote: > On 2014/08/15 15:57:21, Justin Schuh wrote: > > On 2014/08/15 15:48:21, timvolodine wrote: > > > On 2014/08/15 15:47:22, timvolodine wrote: > > > > On 2014/08/15 15:45:47, Justin Schuh wrote: > > > > > One more chance. This broke WinXP, so we either need a fix ASAP, or I > will > > > be > > > > > forced to revert. > > > > > > > > yes RegisterPowerSettingNotification is this patch. According to scottmg > it > > > was > > > > safe to simple have a VISTA runtime guard. > > > > scottmg: any ideas? > > > > You can't rely on delay loads for user32. You have to manually LoadLibrary and > > GetProcAddress for RegisterPowerSettingNotification and > > UnregisterPowerSettingNotification. That's why it's dying in the loader > (because > > the import is failing). > > Really? I had no idea. user32 is special? Well, I suck. Sorry about that Tim. > You'll need to switch back to the previous ps I guess. We do things really weird. Rather than manually import, you might be able to just put the imports here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome.user... > > > > > > > > > Justin doe you have a link with the breakage? > > > > Nope, I think all the WinXP bots have been removed (so much for those 250mil > > users). I just ran into this because I was trying to test a CL on XP this > > morning and it won't start.
Message was sent while issue was closed.
A revert of this CL (patchset #5) has been created in https://codereview.chromium.org/481493002/ by timvolodine@chromium.org. The reason for reverting is: breaking XP.
Message was sent while issue was closed.
A revert of this CL (patchset #5) has been created in https://codereview.chromium.org/466223006/ by erikwright@chromium.org. The reason for reverting is: Breaks execution on XP. See these telemetry tests, which may be the only thing that actually launch the browser itself on XP: http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds... I manually built and ran on an XP VM and received a message that UnregisterPowerSettingNotification could not be located in user32.dll.
Message was sent while issue was closed.
On 2014/08/15 19:21:14, erikwright wrote: > A revert of this CL (patchset #5) has been created in > https://codereview.chromium.org/466223006/ by mailto:erikwright@chromium.org. > > The reason for reverting is: Breaks execution on XP. See these telemetry tests, > which may be the only thing that actually launch the browser itself on XP: > > http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds... > > I manually built and ran on an XP VM and received a message that > UnregisterPowerSettingNotification could not be located in user32.dll. Ah, didn't actually see that it was already reverted.
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/447853002/80001
On 2014/08/16 09:33:01, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/timvolodine@chromium.org/447853002/80001 fix for XP landed here: http://src.chromium.org/viewvc/chrome?revision=289956&view=revision
Message was sent while issue was closed.
Committed patchset #5 (80001) as 290147 |