Battery Status API: browser-side implementation.

Battery Status API: browser-side implementation.

This patch implements the browser side bits needed for the
Battery Status API. It includes a message filter, a singleton
service which owns and controls the platform-specific battery
status manager. Updates are implemented using callbacks and
async IPCs between the browser and the renderer.

A unit test for the service using a fake battery status manager
and callbacks is also provided.

Corresponding renderer-side patch:
https://codereview.chromium.org/252113006/

BUG=122593, 360068
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273336

[timvolodine, 2014-05-16 12:35:27]

some bots are red, because they haven't rolled yet

[Michael van Ouwerkerk, 2014-05-19 11:19:22]

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

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:31: // Stop listenting
for battery status changes.
Nit: s/listenting/listening/

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_interface.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_manager_interface.h:19: // Stop
listenting for battery status changes.
Nit: s/listenting/listening/

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_message_filter.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_message_filter.h:28: void
SendBatteryChange(const blink::WebBatteryStatus& status);
Nit: add an include for WebBatteryStatus?

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_service.cc (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.cc:90:
BrowserThread::PostTask(BrowserThread::IO,
I think you can turn BatteryStatusMessageFilter into a WebContentsObserver, so
that all the messages are handled on the UI thread. You can omit all the thread
checking and hopping code, and the comments, that way. Probably rename it to
BatteryStatusDispatcher then.

See this patch for the kind of unnecessary complexity you might otherwise end up
with: https://codereview.chromium.org/273523007/

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_service.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:54:
std::vector<BatteryStatusUpdateCallback> callbacks_;
Looks like base::CallbackList would be suitable here.

[jochen (gone - plz use gerrit), 2014-05-19 13:14:22]

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_interface.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_manager_interface.h:11: class
BatteryStatusManagerInterface {
it's uncommon to have classes named ...Interface. Why not just make the methods
of the concrete class virtual, so your test class can derive from the concrete
class?

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_service.cc (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.cc:66:
AddCallbackIfNecessary(callbacks_, callback);
can this actually happen that the same callback is added muliple times?

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_service.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:26: void
AddCallback(const BatteryStatusUpdateCallback& callback);
can you move BatteryStatusUpdateCallback's definition to a class? It shouldn't
be in the content namespace and doesn't warrant its own header.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:36: void
SetBatteryManagerForTests(
ForTesting instead of ForTests

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:40: const
BatteryStatusUpdateCallback& update_callback() const;
GetUpdateCallbackForTesting

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:54:
std::vector<BatteryStatusUpdateCallback> callbacks_;
On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> Looks like base::CallbackList would be suitable here.

+1

[timvolodine, 2014-05-22 10:02:33]

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

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:31: // Stop listenting
for battery status changes.
On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> Nit: s/listenting/listening/

Done.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_interface.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_manager_interface.h:11: class
BatteryStatusManagerInterface {
On 2014/05/19 13:14:22, jochen wrote:
> it's uncommon to have classes named ...Interface. Why not just make the
methods
> of the concrete class virtual, so your test class can derive from the concrete
> class?

the reason I did this is because there was a StopListeningBatteryChange() in the
destructor of battery_status_manager_android.cc, which made it impossible to
derive directly from the BatteryStatusManager class. I've currently removed the
interface, but not completely sure if that's the best way because the test now
derives directly from BatteryStatusManager which is platform-dependent so future
implementations need to be careful not to do anything hardware specific in the
header and in the destructor...

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_manager_interface.h:19: // Stop
listenting for battery status changes.
On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> Nit: s/listenting/listening/

Done.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_message_filter.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_message_filter.h:28: void
SendBatteryChange(const blink::WebBatteryStatus& status);
On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> Nit: add an include for WebBatteryStatus?

Done.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_service.cc (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.cc:66:
AddCallbackIfNecessary(callbacks_, callback);
On 2014/05/19 13:14:22, jochen wrote:
> can this actually happen that the same callback is added muliple times?

actually no, the message filter keeps track of whether it is started, so I've
probably over-engineered this a bit. This is now solved by using CallbackList.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.cc:90:
BrowserThread::PostTask(BrowserThread::IO,
On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> I think you can turn BatteryStatusMessageFilter into a WebContentsObserver, so
> that all the messages are handled on the UI thread. You can omit all the
thread
> checking and hopping code, and the comments, that way. Probably rename it to
> BatteryStatusDispatcher then.
> 
> See this patch for the kind of unnecessary complexity you might otherwise end
up
> with: https://codereview.chromium.org/273523007/

The idea is to keep non-UI things away from the UI thread. Battery is clearly IO
so should preferably be handled on IO thread as much as possible. In the current
approach the UpdateBatteryStatus method can be called from any thread also
allowing simpler implementations for other platforms if they happen to use yet
another thread for battery fetching.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
File content/browser/battery_status/battery_status_service.h (right):

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:26: void
AddCallback(const BatteryStatusUpdateCallback& callback);
On 2014/05/19 13:14:22, jochen wrote:
> can you move BatteryStatusUpdateCallback's definition to a class? It shouldn't
> be in the content namespace and doesn't warrant its own header.

done. I've put it inside this class.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:36: void
SetBatteryManagerForTests(
On 2014/05/19 13:14:22, jochen wrote:
> ForTesting instead of ForTests

Done.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:40: const
BatteryStatusUpdateCallback& update_callback() const;
On 2014/05/19 13:14:22, jochen wrote:
> GetUpdateCallbackForTesting

Done.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:54:
std::vector<BatteryStatusUpdateCallback> callbacks_;
On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> Looks like base::CallbackList would be suitable here.

Done.

https://codereview.chromium.org/278043002/diff/110001/content/browser/battery...
content/browser/battery_status/battery_status_service.h:54:
std::vector<BatteryStatusUpdateCallback> callbacks_;
On 2014/05/19 13:14:22, jochen wrote:
> On 2014/05/19 11:19:22, Michael van Ouwerkerk wrote:
> > Looks like base::CallbackList would be suitable here.
> 
> +1

Done.

[Michael van Ouwerkerk, 2014-05-22 10:36:52]

Thanks Tim! lgtm

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

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:25: // Methods from
BatteryStatusManagerInterface.
Nit: does this comment still apply?

[jochen (gone - plz use gerrit), 2014-05-23 08:13:35]

lgtm with comments

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

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:23: virtual
~BatteryStatusManager() { }
please keep non-trivial dtors in the .cc file (it'll destruct the scoped java
global ref)

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_android.cc (right):

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
content/browser/battery_status/battery_status_manager_android.cc:16: :
callback_(callback) {
is that clang-formatted?

[timvolodine, 2014-05-27 12:32:04]

thanks for the review!

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

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:23: virtual
~BatteryStatusManager() { }
On 2014/05/23 08:13:36, jochen wrote:
> please keep non-trivial dtors in the .cc file (it'll destruct the scoped java
> global ref)

Done.

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
content/browser/battery_status/battery_status_manager.h:25: // Methods from
BatteryStatusManagerInterface.
On 2014/05/22 10:36:52, Michael van Ouwerkerk wrote:
> Nit: does this comment still apply?

Done.

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
File content/browser/battery_status/battery_status_manager_android.cc (right):

https://codereview.chromium.org/278043002/diff/130001/content/browser/battery...
content/browser/battery_status/battery_status_manager_android.cc:16: :
callback_(callback) {
On 2014/05/23 08:13:36, jochen wrote:
> is that clang-formatted?

Done.

[timvolodine, 2014-05-28 17:50:08]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-05-28 17:51:19]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/timvolodine@chromium.org/278043002/170001

[commit-bot: I haz the power, 2014-05-28 20:07:54]

Change committed as 273336