Add Battery Status API support in content/renderer and IPC messages.

Add Battery Status API support in content/renderer and IPC messages.

This patch defines the async IPCs needed for the Battery Status API.
It also implements the platform-side part with a
setBatteryStatusListener() method and a dispatcher class for
propagating messages to blink. Unit tests for the dispatcher
class are also included.

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

[jochen (gone - plz use gerrit), 2014-04-30 04:54:07]

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher.cc (right):

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:9: #include
"content/public/renderer/render_thread.h"
please use the content/renderer version inside content

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:27:
thread->AddObserver(this);
Why not RenderThread::Get()->AddObserver() in the ctor?

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher.h (right):

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.h:24: virtual bool
OnControlMessageReceived(const IPC::Message& message) OVERRIDE;
please group overrides and add a comment which interface they implement

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.h:31: virtual bool
Start();
i guess your test needs to be a friend of this class, or those methods should be
protected?

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.h:36: };
disallow copy & assign

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:28:
blink::WebBatteryStatus status_;
members should be private and use disallow copy/assign

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:49: bool
start_invoked_;
same here

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:64:
scoped_ptr<BatteryStatusDispatcherForTesting> dispatcher_;
and here

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:66: 
nit. only one empty line

[timvolodine, 2014-05-01 10:32:38]

thanks for the comments, ptal.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher.cc (right):

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:9: #include
"content/public/renderer/render_thread.h"
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> please use the content/renderer version inside content

Done, if you mean "content/renderer/render_thread_impl.h".

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:27:
thread->AddObserver(this);
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> Why not RenderThread::Get()->AddObserver() in the ctor?

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher.h (right):

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.h:24: virtual bool
OnControlMessageReceived(const IPC::Message& message) OVERRIDE;
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> please group overrides and add a comment which interface they implement

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.h:31: virtual bool
Start();
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> i guess your test needs to be a friend of this class, or those methods should
be
> protected?

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.h:36: };
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> disallow copy & assign

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:28:
blink::WebBatteryStatus status_;
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> members should be private and use disallow copy/assign

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:49: bool
start_invoked_;
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> same here

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:64:
scoped_ptr<BatteryStatusDispatcherForTesting> dispatcher_;
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> and here

Done.

https://codereview.chromium.org/252113006/diff/20001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher_unittest.cc:66: 
On 2014/04/30 04:54:07, jochen (OOO Wed-Thu) wrote:
> nit. only one empty line

Done.

[Michael van Ouwerkerk, 2014-05-01 13:46:41]

We discussed a few questions I had in person. lgtm

[jochen (gone - plz use gerrit), 2014-05-02 14:08:53]

i wonder what the browser side looks like?

https://codereview.chromium.org/252113006/diff/40001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher.cc (right):

https://codereview.chromium.org/252113006/diff/40001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:20:
BatteryStatusDispatcher::~BatteryStatusDispatcher() { }
should this invoke Stop() if listener_ is non-NULL?

[timvolodine, 2014-05-02 15:03:55]

the browser-side is coming soon

https://codereview.chromium.org/252113006/diff/40001/content/renderer/battery...
File content/renderer/battery_status/battery_status_dispatcher.cc (right):

https://codereview.chromium.org/252113006/diff/40001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:20:
BatteryStatusDispatcher::~BatteryStatusDispatcher() { }
On 2014/05/02 14:08:53, jochen wrote:
> should this invoke Stop() if listener_ is non-NULL?

Done.

[timvolodine, 2014-05-02 15:25:03]

+jschuh for IPC messages review: battery_status_messages.h.

[jochen (gone - plz use gerrit), 2014-05-05 07:33:27]

lgtm

[jschuh, 2014-05-05 17:31:58]

Lgtm for ipc security (notes: opaque boolean and double values).

Have you run this by privacy team? At a minimum, I'd think they'd want to be
aware of it as another piece of fingerprinting data.

[timvolodine, 2014-05-14 17:16:33]

On 2014/05/05 17:31:58, Justin Schuh wrote:
> Lgtm for ipc security (notes: opaque boolean and double values).
> 
> Have you run this by privacy team? At a minimum, I'd think they'd want to be
> aware of it as another piece of fingerprinting data.

ok, thanks. I've informed the privacy team (kaichou@) about this feature.

[timvolodine, 2014-05-14 17:49:53]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-05-14 17:50:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/timvolodine@chromium.org/252113006/70001

[commit-bot: I haz the power, 2014-05-14 23:41:05]

Change committed as 270504