Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

During this process I noticed the initial status for "is on
battery power" is incorrect. On Android, ChromeOS the initial
state is later supplied by Java and DBus respectively. On
Windows it was using a OneShotTimer. This replaces the Windows
implementation with a new protected setter which can be used
by the device source implementations to set the initial value
before a PowerMonitor exists.

BUG=649166
TEST=OSX properly detects battery changes. Initial state correct.

Committed: https://crrev.com/8621728865773df979e061a3b4ab5e6ef80a2cc2
Cr-Commit-Position: refs/heads/master@{#421350}

[DaleCurtis, 2016-09-20 00:02:15]

Patchset #1 (id:1) has been deleted

[Nico, 2016-09-20 00:04:13]

https://codereview.chromium.org/2351593004/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2351593004/diff/20001/base/BUILD.gn#newcode1310
base/BUILD.gn:1310: sources -= [
"power_monitor/power_monitor_device_source_posix.cc" ]
(fwiw, instead of "add all, then subtract", in gn we do "only add what we need
in the first place" -- so instead of removing it here, only add it if !is_mac
further up)

[Nico, 2016-09-20 00:05:08]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-09-20 00:05:08]

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
base/power_monitor/power_monitor_device_source_mac.mm:24: bool
PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
how often do we poll this? there's a "this changed" observer interface on os x i
think

[DaleCurtis, 2016-09-20 00:15:13]

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
base/power_monitor/power_monitor_device_source_mac.mm:24: bool
PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
On 2016/09/20 at 00:05:08, Nico wrote:
> how often do we poll this? there's a "this changed" observer interface on os x
i think

Doesn't look like it's called very much anywhere. It looks like media/ may be
the biggest user:
https://cs.chromium.org/search/?q=IsOnBatteryPower&type=cs&sq=package:chromium

We're calling it probably every 5 seconds. I look for another interface, but I
think the one you're talking about is used here: 
https://cs.chromium.org/chromium/src/device/battery/battery_status_manager_ma...

Even then it requires repolling all sources on any state change notification. I
guess the state change notification isn't specific enough. Unfortunately this
API also requires a run loop :/

I think I'm going to need that here after all though, since sources are supposed
to send PowerMonitorSource::POWER_STATE_EVENT when the battery status changed.

[shrike, 2016-09-20 17:07:23]

Description was changed from

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

BUG=tbd
TEST=tbd
==========

to

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

BUG=tbd
TEST=tbd
==========

[shrike, 2016-09-20 17:09:09]

shrike@chromium.org changed reviewers:
+ shrike@chromium.org

[shrike, 2016-09-20 17:09:10]

+rsesek@

rsesek@ - is this another case where we should be waiting for a notification
from macOS instead of polling (are we actually polling)?

[DaleCurtis, 2016-09-20 18:18:40]

I'm planning to use a notification + cached value update like battery status
monitor does; so polling should be low cost.

[Robert Sesek, 2016-09-20 18:19:56]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2016-09-20 18:19:56]

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
base/power_monitor/power_monitor_device_source_mac.mm:24: bool
PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
On 2016/09/20 00:15:13, DaleCurtis wrote:
> On 2016/09/20 at 00:05:08, Nico wrote:
> > how often do we poll this? there's a "this changed" observer interface on os
x
> i think
> 
> Doesn't look like it's called very much anywhere. It looks like media/ may be
> the biggest user:
> https://cs.chromium.org/search/?q=IsOnBatteryPower&type=cs&sq=package:chromium
> 
> We're calling it probably every 5 seconds. I look for another interface, but I
> think the one you're talking about is used here: 
>
https://cs.chromium.org/chromium/src/device/battery/battery_status_manager_ma...
> 
> Even then it requires repolling all sources on any state change notification.
I
> guess the state change notification isn't specific enough. Unfortunately this
> API also requires a run loop :/

I do think we should use the IOPSNotificationCreateRunLoopSource() interface
instead. The run loop source could be installed on the main thread without
issue, and when it's received, it could bounce the IOPSCopyPowerSourcesInfo()
work onto a worker pool, and the result could then be cached.

> I think I'm going to need that here after all though, since sources are
supposed
> to send PowerMonitorSource::POWER_STATE_EVENT when the battery status changed.

That's my conclusion as well. It looks like the caching may be done for you,
too:
https://cs.chromium.org/chromium/src/base/power_monitor/power_monitor_source....

[Robert Sesek, 2016-09-20 18:23:35]

On 2016/09/20 18:18:40, DaleCurtis wrote:
> I'm planning to use a notification + cached value update like battery status
> monitor does; so polling should be low cost.

Great!

[shrike, 2016-09-20 18:56:19]

On 2016/09/20 18:23:35, Robert Sesek wrote:
> On 2016/09/20 18:18:40, DaleCurtis wrote:
> > I'm planning to use a notification + cached value update like battery status
> > monitor does; so polling should be low cost.
> 
> Great!

So what polling will be occurring, exactly? Chrome polling the OS for the latest
battery use status, or clients polling Chrome;s  power monitor system for
changes?

[DaleCurtis, 2016-09-20 19:07:25]

On 2016/09/20 at 18:56:19, shrike wrote:
> On 2016/09/20 18:23:35, Robert Sesek wrote:
> > On 2016/09/20 18:18:40, DaleCurtis wrote:
> > > I'm planning to use a notification + cached value update like battery
status
> > > monitor does; so polling should be low cost.
> > 
> > Great!
> 
> So what polling will be occurring, exactly? Chrome polling the OS for the
latest battery use status, or clients polling Chrome;s  power monitor system for
changes?

I meant client's polling the PowerMonitor. But, actually I had forgotten my own
implementation, there's no polling in the clients I know of. They respond to the
power state changed events instead.

[shrike, 2016-09-20 19:08:31]

On 2016/09/20 19:07:25, DaleCurtis wrote:
> I meant client's polling the PowerMonitor. But, actually I had forgotten my
own
> implementation, there's no polling in the clients I know of. They respond to
the
> power state changed events instead.

OK great.

[DaleCurtis, 2016-09-21 22:47:45]

Description was changed from

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

BUG=tbd
TEST=tbd
==========

to

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

During this process I noticed the initial status for "is on
battery power" is incorrect. So this is also fixed. This removes
a timer on windows in favor of setting the initial status
immediately.

BUG=649166
TEST=OSX properly detects battery changes. Initial state correct.
==========

[DaleCurtis, 2016-09-21 22:49:13]

Okay this should be ready to go for review. Notably I've also fixed the initial
status on all platforms which was only correct on Windows after a 10 second
timer fired. I've replaced this timer with a new protected setter for
implementations and call it during construction of PowerMonitorDeviceSources.

I can't find anything explaining why a 10 second timer was used here; though I
will keep digging. If anyone knows offhand that'd be great.

[DaleCurtis, 2016-09-21 22:49:39]

https://codereview.chromium.org/2351593004/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2351593004/diff/20001/base/BUILD.gn#newcode1310
base/BUILD.gn:1310: sources -= [
"power_monitor/power_monitor_device_source_posix.cc" ]
On 2016/09/20 at 00:04:13, Nico wrote:
> (fwiw, instead of "add all, then subtract", in gn we do "only add what we need
in the first place" -- so instead of removing it here, only add it if !is_mac
further up)

Done. Fixed all the existing cases.

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/2351593004/diff/20001/base/power_monitor/powe...
base/power_monitor/power_monitor_device_source_mac.mm:24: bool
PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
On 2016/09/20 at 18:19:56, Robert Sesek wrote:
> On 2016/09/20 00:15:13, DaleCurtis wrote:
> > On 2016/09/20 at 00:05:08, Nico wrote:
> > > how often do we poll this? there's a "this changed" observer interface on
os x
> > i think
> > 
> > Doesn't look like it's called very much anywhere. It looks like media/ may
be
> > the biggest user:
> >
https://cs.chromium.org/search/?q=IsOnBatteryPower&type=cs&sq=package:chromium
> > 
> > We're calling it probably every 5 seconds. I look for another interface, but
I
> > think the one you're talking about is used here: 
> >
https://cs.chromium.org/chromium/src/device/battery/battery_status_manager_ma...
> > 
> > Even then it requires repolling all sources on any state change
notification. I
> > guess the state change notification isn't specific enough. Unfortunately
this
> > API also requires a run loop :/
> 
> I do think we should use the IOPSNotificationCreateRunLoopSource() interface
instead. The run loop source could be installed on the main thread without
issue, and when it's received, it could bounce the IOPSCopyPowerSourcesInfo()
work onto a worker pool, and the result could then be cached.
> 
> > I think I'm going to need that here after all though, since sources are
supposed
> > to send PowerMonitorSource::POWER_STATE_EVENT when the battery status
changed.
> 
> That's my conclusion as well. It looks like the caching may be done for you,
too:
https://cs.chromium.org/chromium/src/base/power_monitor/power_monitor_source....

Done.

[DaleCurtis, 2016-09-21 22:49:44]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 22:50:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 23:03:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:03:10]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2016-09-22 00:07:01]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 00:07:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 00:19:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 00:19:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[DaleCurtis, 2016-09-22 00:21:20]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 00:21:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 00:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 00:33:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[DaleCurtis, 2016-09-22 00:35:26]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 00:36:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 00:49:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 00:49:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[DaleCurtis, 2016-09-22 02:13:56]

Hmm, looks like Android and iOS need some work. Will ping when fixed.

[DaleCurtis, 2016-09-23 01:30:20]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 01:30:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 02:00:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 02:00:49]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[DaleCurtis, 2016-09-23 22:48:53]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 22:49:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2016-09-23 23:53:49]

Phew, 7 iterations later all the bots now pass. PTAL.

[DaleCurtis, 2016-09-23 23:55:31]

Description was changed from

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

During this process I noticed the initial status for "is on
battery power" is incorrect. So this is also fixed. This removes
a timer on windows in favor of setting the initial status
immediately.

BUG=649166
TEST=OSX properly detects battery changes. Initial state correct.
==========

to

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

During this process I noticed the initial status for "is on
battery power" is incorrect. On Android, ChromeOS the initial
state is later supplied by Java and DBus respectively. On
Windows it was using a OneShotTimer. This replaces the Windows
implementation with a new protected setter which can be used
by the device source implementations to set the initial value
before a PowerMonitor exists.

BUG=649166
TEST=OSX properly detects battery changes. Initial state correct.
==========

[commit-bot: I haz the power, 2016-09-23 23:56:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 23:56:31]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2016-09-26 22:21:19]

LGTM

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
File base/power_monitor/power_monitor_device_source.h (right):

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
base/power_monitor/power_monitor_device_source.h:28: class BASE_EXPORT
PowerMonitorDeviceSource : public PowerMonitorSource {
This class has kind of a weird design with all the platform-specific #ifs. It
seems like each platform should get to define its own version of
PowerMonitorDeviceSource (like base/memory/memory_pressure_monitor.h does). But
that's outside the scope of this CL.

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
base/power_monitor/power_monitor_device_source_mac.mm:59: CFRunLoopSourceRef
g_battery_status_ref = 0;
... like these could be member variables instead of just random globals.

[DaleCurtis, 2016-09-27 17:04:17]

thakis: Ping?

[Nico, 2016-09-27 20:19:27]

lgtm

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
File base/power_monitor/power_monitor_device_source_ios.mm (right):

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
base/power_monitor/power_monitor_device_source_ios.mm:11: bool
PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
Is this used by anything on iOS? If not, maybe we can not provide an
implementation so we'll get a link error if something starts using it. If it is
used on iOS, there should probably be a bug about for implementing it.

[DaleCurtis, 2016-09-27 20:53:41]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-09-27 20:53:42]

Thanks for review!

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
File base/power_monitor/power_monitor_device_source_ios.mm (right):

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
base/power_monitor/power_monitor_device_source_ios.mm:11: bool
PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
On 2016/09/27 at 20:19:27, Nico (mostly away until Oct 3) wrote:
> Is this used by anything on iOS? If not, maybe we can not provide an
implementation so we'll get a link error if something starts using it. If it is
used on iOS, there should probably be a bug about for implementing it.

ios_chrome_unittests fails to link without this, but I haven't found anything
using it explicitly. See any of the previous patch set try jobs for more
details. Here's the exact error w/o this for posterity:

FAILED: obj/ios/chrome/arm/ios_chrome_unittests 
...
Undefined symbols for architecture armv7:
  "base::PowerMonitorDeviceSource::IsOnBatteryPowerImpl()", referenced from:
      vtable for base::PowerMonitorDeviceSource in
libbase.a(power_monitor_device_source.o)

https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/build...

[commit-bot: I haz the power, 2016-09-27 20:54:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2016-09-27 20:59:33]

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
File base/power_monitor/power_monitor_device_source.h (right):

https://codereview.chromium.org/2351593004/diff/140001/base/power_monitor/pow...
base/power_monitor/power_monitor_device_source.h:84: bool IsOnBatteryPowerImpl()
override;
Ah, because this is in the vtable. But what references the vtable?

...probably this
https://cs.chromium.org/chromium/src/ios/web/app/web_main_loop.mm?rcl=0&l=75

So now we're in a situation where the function isn't implemented, but we have a
stub. I guess Rohit figured it's not super important, so landing this as-is
seems fine, but we should probably delete that ref (not in this Cl, and not you)
and then the ios implementation of the empty function, so that if someone
actually wants to use it they'll get a linker error informing them they need to
implement it.

[commit-bot: I haz the power, 2016-09-27 21:56:13]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-09-27 21:59:59]

Description was changed from

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

During this process I noticed the initial status for "is on
battery power" is incorrect. On Android, ChromeOS the initial
state is later supplied by Java and DBus respectively. On
Windows it was using a OneShotTimer. This replaces the Windows
implementation with a new protected setter which can be used
by the device source implementations to set the initial value
before a PowerMonitor exists.

BUG=649166
TEST=OSX properly detects battery changes. Initial state correct.
==========

to

==========
Implement base::PowerMonitor::IsOnBatteryPower() for OSX.

For whatever reason this was never implemented; this is slimmed
down variant of the implementation in devices/battery/.

During this process I noticed the initial status for "is on
battery power" is incorrect. On Android, ChromeOS the initial
state is later supplied by Java and DBus respectively. On
Windows it was using a OneShotTimer. This replaces the Windows
implementation with a new protected setter which can be used
by the device source implementations to set the initial value
before a PowerMonitor exists.

BUG=649166
TEST=OSX properly detects battery changes. Initial state correct.

Committed: https://crrev.com/8621728865773df979e061a3b4ab5e6ef80a2cc2
Cr-Commit-Position: refs/heads/master@{#421350}
==========

[commit-bot: I haz the power, 2016-09-27 22:00:00]

Patchset 7 (id:??) landed as
https://crrev.com/8621728865773df979e061a3b4ab5e6ef80a2cc2
Cr-Commit-Position: refs/heads/master@{#421350}