battery-status: Implement the battery-status API for chromeos.

battery-status: Implement the battery-status API for chromeos.

This patch changes BatteryStatusManager to be a pure interface, and moves
platform-specific sub-classes to provide the necessary implementations.

BUG=122593
TEST=manual, using http://jsbin.com/battery-status-diagnostics
R=brettw@chromium.org, jdduke@chromium.org, timvolodine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283015

[sadrul, 2014-06-26 04:50:44]

Hi! I haven't had a chance to try this out on a device yet. But it works with
the stub cros-power implementation.

[sadrul, 2014-06-30 04:09:29]

On 2014/06/26 04:50:44, sadrul wrote:
> Hi! I haven't had a chance to try this out on a device yet. But it works with
> the stub cros-power implementation.

Tried this out on a device. Seems to work fairly well.

[timvolodine, 2014-07-02 19:41:39]

Thanks Sadrul,
just a few comments below. also I think you can refer to crbug.com/122593 in
description.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
File content/browser/battery_status/battery_status_manager.h (right):

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:18: // Note that
|callback| should be called in the UI thread.
I don't think we should force implementations to invoke callback on UI thread.
This comment could just say "can be called on any thread" or just remove it
altogether.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:28:
StopOnUI();
nit: so the destructor should be called on the UI thread only? maybe mention
this in a comment..

I was actually wondering if there is an easy solution to make it destructable on
any thread, in case we would decide to clean-up the object when it's not needed
(and not only in BatteryStatusService::Shutdown()).

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:35:
power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
nit: indent +4 spaces as in the method below. this seems to apply to
IsBatteryCharging and IsBatteryFull as well.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:63: if
(!currently_listening_) {
nit: early return as in Stop?

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:101: virtual
void PowerChanged(
just out of curiosity: is this usually called when level changes by 1percent on
chromeos?

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:111: // Do not
set |chargingTime| and |dischargingTime| if the time is still
If the chargingTime cannot be provided (maybe temporarily) it should be set to
+infinity. This is different from the blink::WebBatteryStatus defaults because
they are designed to emulate a "full" battery.

We recently had some discussions regarding the default values on
public-device-apis@w3.org, the spec should be updated now to make it more clear
(https://dvcs.w3.org/hg/dap/raw-file/default/battery/Overview.html)

[sadrul, 2014-07-04 16:58:19]

I have addressed your comments. I have also changed
BatteryStatusManager::Create() to return a scoped_ptr<> instead of a raw pointer
to clarify the ownership transfer. PTAL.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
File content/browser/battery_status/battery_status_manager.h (right):

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:18: // Note that
|callback| should be called in the UI thread.
On 2014/07/02 19:41:39, timvolodine wrote:
> I don't think we should force implementations to invoke callback on UI thread.
> This comment could just say "can be called on any thread" or just remove it
> altogether.

Removed.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:28:
StopOnUI();
On 2014/07/02 19:41:39, timvolodine wrote:
> nit: so the destructor should be called on the UI thread only? maybe mention
> this in a comment..
> 
> I was actually wondering if there is an easy solution to make it destructable
on
> any thread, in case we would decide to clean-up the object when it's not
needed
> (and not only in BatteryStatusService::Shutdown()).

I have changed this so the BatteryStatusManager object can be destroyed on any
thread.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:35:
power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
On 2014/07/02 19:41:39, timvolodine wrote:
> nit: indent +4 spaces as in the method below. this seems to apply to
> IsBatteryCharging and IsBatteryFull as well.

This is how 'git cl format' formats it. Do you feel strongly that I should
change this to 4-space indent regardless?

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:63: if
(!currently_listening_) {
On 2014/07/02 19:41:39, timvolodine wrote:
> nit: early return as in Stop?

Done.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:101: virtual
void PowerChanged(
On 2014/07/02 19:41:39, timvolodine wrote:
> just out of curiosity: is this usually called when level changes by 1percent
on
> chromeos?

From a quick test: this can trigger more often than that, e.g. to update the
charging/discharging time (on average, it looks like there are two updates every
minute). This is the same notification used to update the battery indicator in
the system-tray on Chrome OS.

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:111: // Do not
set |chargingTime| and |dischargingTime| if the time is still
On 2014/07/02 19:41:39, timvolodine wrote:
> If the chargingTime cannot be provided (maybe temporarily) it should be set to
> +infinity. This is different from the blink::WebBatteryStatus defaults because
> they are designed to emulate a "full" battery.
> 
> We recently had some discussions regarding the default values on
> mailto:public-device-apis@w3.org, the spec should be updated now to make it
more clear
> (https://dvcs.w3.org/hg/dap/raw-file/default/battery/Overview.html)

Done.

[timvolodine, 2014-07-09 11:35:02]

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:35:
power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
On 2014/07/04 16:58:18, sadrul (OOO) wrote:
> On 2014/07/02 19:41:39, timvolodine wrote:
> > nit: indent +4 spaces as in the method below. this seems to apply to
> > IsBatteryCharging and IsBatteryFull as well.
> 
> This is how 'git cl format' formats it. Do you feel strongly that I should
> change this to 4-space indent regardless?

I don't feel strongly about those indents as long as they are consistent, which
they are now :)

https://codereview.chromium.org/356873002/diff/190001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:101: virtual
void PowerChanged(
On 2014/07/04 16:58:18, sadrul (OOO) wrote:
> On 2014/07/02 19:41:39, timvolodine wrote:
> > just out of curiosity: is this usually called when level changes by 1percent
> on
> > chromeos?
> 
> From a quick test: this can trigger more often than that, e.g. to update the
> charging/discharging time (on average, it looks like there are two updates
every
> minute). This is the same notification used to update the battery indicator in
> the system-tray on Chrome OS.

yeah, looks like this depends on device/system..

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:32: if
(!currently_listening_)
This looks like there is a potential race condition here. What if on lets say on
IO thread:
StartListeningBatteryChange()
~BatteryStatusManager()
at the time of destructor it has not started yet, so it returns early. Then
start is invoked on the UI thread (before the actual destruction of weak
reference) so the overall result is that the observer is never removed. Removing
the "if (!currently_listening_)" check might resolve this, but in general it
looks we can have destruction in parallel with StartOnUI execution which sounds
scary. Probably the easiest way is to make the destructor obligatory on UI
thread as it was before (+maybe add a comment to clarify).

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:75: void
StartOnUI() {
not sure if this is relevant for chromeOS, but alternatively I am considering a
dedicated thread to receive notifications on and to offload the UI thread on
desktop systems (e.g. macOS).

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:102:
weak_factory_.GetWeakPtr()));
Probably false alarm, but I was curious whether WeakPtr was thread-safe. In
callback.h it says they are not:
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&q=...
however in weak_ptr.h the comments appear to vaguely allow this:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

wdyt?

[sadrul, 2014-07-13 09:04:55]

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:32: if
(!currently_listening_)
On 2014/07/09 11:35:01, timvolodine wrote:
> This looks like there is a potential race condition here. What if on lets say
on
> IO thread:
> StartListeningBatteryChange()
> ~BatteryStatusManager()
> at the time of destructor it has not started yet, so it returns early. Then
> start is invoked on the UI thread (before the actual destruction of weak
> reference) so the overall result is that the observer is never removed.
Removing
> the "if (!currently_listening_)" check might resolve this, but in general it
> looks we can have destruction in parallel with StartOnUI execution which
sounds
> scary. Probably the easiest way is to make the destructor obligatory on UI
> thread as it was before (+maybe add a comment to clarify).

I have moved the main code out of this into a RefCountedThreadSafe<> object,
which performs all the relevant tasks in the UI thread. I think that takes care
of the concerns here.

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:75: void
StartOnUI() {
On 2014/07/09 11:35:01, timvolodine wrote:
> not sure if this is relevant for chromeOS, but alternatively I am considering
a
> dedicated thread to receive notifications on and to offload the UI thread on
> desktop systems (e.g. macOS).

In the case of Chrome OS, the interaction with the PowerManagerClient needs to
always happen in the UI thread. So it will still be necessary to bounce between
threads.

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:102:
weak_factory_.GetWeakPtr()));
On 2014/07/09 11:35:01, timvolodine wrote:
> Probably false alarm, but I was curious whether WeakPtr was thread-safe. In
> callback.h it says they are not:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&q=...
> however in weak_ptr.h the comments appear to vaguely allow this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> 
> wdyt?

Good catch! It looks like WeakPtr indeed is not thread-safe. The comment in
callback.h implies the weak-ptr cannot be dereferenced in another thread, and
for the callback to run, we do need to do that. I think the comment in
weak_ptr.h allows using the weak-ptr in another thread in a manner that it is
dereferenced only in the originating thread (e.g. to post-task back to the first
thread).

As commented above, I am using a RefCountedThreadSafe<> object to get around
this issue.

[timvolodine, 2014-07-14 17:38:56]

thanks! lgtm.
in description could you please add TEST=manual+url for future reference.

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/230001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:75: void
StartOnUI() {
On 2014/07/13 09:04:54, sadrul wrote:
> On 2014/07/09 11:35:01, timvolodine wrote:
> > not sure if this is relevant for chromeOS, but alternatively I am
considering
> a
> > dedicated thread to receive notifications on and to offload the UI thread on
> > desktop systems (e.g. macOS).
> 
> In the case of Chrome OS, the interaction with the PowerManagerClient needs to
> always happen in the UI thread. So it will still be necessary to bounce
between
> threads.

fyi: on macOS it is sufficient to have a thread with UI-type message loop.

https://codereview.chromium.org/356873002/diff/290001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/290001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:8: #include
"base/memory/weak_ptr.h"
is this still needed?

[sadrul, 2014-07-14 17:46:31]

jdduke@chromium.org: Please review changes in browser_jni_registrar.cc

brettw@chromium.org: Please review changes in BUILD.gn

https://codereview.chromium.org/356873002/diff/290001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_chromeos.cc (right):

https://codereview.chromium.org/356873002/diff/290001/content/browser/battery...
content/browser/battery_status/battery_status_manager_chromeos.cc:8: #include
"base/memory/weak_ptr.h"
On 2014/07/14 17:38:56, timvolodine wrote:
> is this still needed?

Nope. Removed.

[jdduke (slow), 2014-07-14 18:09:18]

On 2014/07/14 17:46:31, sadrul wrote:
> mailto:jdduke@chromium.org: Please review changes in browser_jni_registrar.cc

lgtm

[brettw, 2014-07-14 19:38:59]

BUILD.gn LGTM

[sadrul, 2014-07-14 20:55:47]

Committed patchset #11 manually as r283015 (presubmit successful).