Replace Chrome IPC with Mojo IPC for querying BatteryStatus service

Replace Chrome IPC with Mojo IPC for querying BatteryStatus service

This is based on patch set 26 from Darin's https://codereview.chromium.org/457933002/ .

BUG=420623
Committed: https://crrev.com/9e6d2387b6d9b28ab6defe7f5feb715a5abbc242
Cr-Commit-Position: refs/heads/master@{#301380}

[ppi, 2014-09-23 11:14:11]

Patchset #3 (id:40001) has been deleted

[ppi, 2014-09-23 14:27:14]

Patchset #3 (id:60001) has been deleted

[ppi, 2014-09-23 17:58:02]

ppi@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org, timvolodine@chromium.org

[ppi, 2014-09-23 18:04:02]

ppi@chromium.org changed reviewers:
+ darin@chromium.org

[ppi, 2014-09-23 18:04:03]

Hello,

This is taken over from Darin's https://codereview.chromium.org/457933002/ .

Adding
- timvolodine@ for battery status
- rockot@ for device/
- jam@ for content/

ptal.

[ppi, 2014-09-25 18:38:36]

ping - Tim, Ken, John, could you take a look?

[timvolodine, 2014-09-25 23:08:33]

thanks ppi@, some initial comments on the battery API code.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager.h (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager.h:9: #include "build/build_config.h"
do we still need this include?

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:323: const base::DictionaryValue&
dictionary) {
fits on one line?

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager_mac.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager_mac.cc:233:
scoped_refptr<BatteryStatusObserver> notifier_;
now that the whole thing runs on UI thread we can probably get rid of
scoped_refptr and use scoped_ptr instead..

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager_win.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager_win.cc:141:
scoped_refptr<BatteryStatusObserver> battery_observer_;
same here, doesn't look like we need a scoped_refptr anymore?

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_service.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_service.cc:39: bool success =
battery_fetcher_->StartListeningBatteryChange();
we need to be careful here because the StartListeningBatteryChange() call can
invoke NotifyConsumers() directly. probably a post task for NotifyConsumers
would solve this issue (and also the race condition as noted below).

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_service.cc:41: callback.Run(BatteryStatus());
do we need a comment here? something like "on failure execute callback with
default values".

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_service.cc:62: void
BatteryStatusService::NotifyConsumers(const BatteryStatus& status) {
I think we could have a concurrency issue here due to Linux implementation
running on a separate "notifier thread", where the callback is executed.

[jam, 2014-09-27 00:55:35]

jam@chromium.org changed reviewers:
- jam@chromium.org

[jam, 2014-09-27 00:55:59]

On 2014/09/25 18:38:36, ppi wrote:
> ping - Tim, Ken, John, could you take a look?

since darin is added as a reviewer, I'll defer to him.

[ppi, 2014-09-29 15:50:30]

Patchset #9 (id:200001) has been deleted

[ppi, 2014-09-29 16:17:41]

Patchset #10 (id:230001) has been deleted

[ppi, 2014-09-29 16:43:24]

ppi@chromium.org changed reviewers:
+ jam@chromium.org
- darin@chromium.org

[ppi, 2014-09-29 16:43:27]

Thanks Tim, ptal.

John, Darin is the original author of the CL, so I think it would make sense if
you could take a look here. Moving Darin to cc.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager.h (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager.h:9: #include "build/build_config.h"
On 2014/09/25 23:08:32, timvolodine wrote:
> do we still need this include?

Done.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager_linux.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager_linux.cc:323: const base::DictionaryValue&
dictionary) {
On 2014/09/25 23:08:32, timvolodine wrote:
> fits on one line?

Done.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager_mac.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager_mac.cc:233:
scoped_refptr<BatteryStatusObserver> notifier_;
On 2014/09/25 23:08:33, timvolodine wrote:
> now that the whole thing runs on UI thread we can probably get rid of
> scoped_refptr and use scoped_ptr instead..

Done.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_manager_win.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_manager_win.cc:141:
scoped_refptr<BatteryStatusObserver> battery_observer_;
On 2014/09/25 23:08:33, timvolodine wrote:
> same here, doesn't look like we need a scoped_refptr anymore?

Done.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
File device/battery/battery_status_service.cc (right):

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_service.cc:39: bool success =
battery_fetcher_->StartListeningBatteryChange();
On 2014/09/25 23:08:33, timvolodine wrote:
> we need to be careful here because the StartListeningBatteryChange() call can
> invoke NotifyConsumers() directly. probably a post task for NotifyConsumers
> would solve this issue (and also the race condition as noted below).

Done.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_service.cc:41: callback.Run(BatteryStatus());
On 2014/09/25 23:08:33, timvolodine wrote:
> do we need a comment here? something like "on failure execute callback with
> default values".

Done.

https://codereview.chromium.org/592153002/diff/120001/device/battery/battery_...
device/battery/battery_status_service.cc:62: void
BatteryStatusService::NotifyConsumers(const BatteryStatus& status) {
On 2014/09/25 23:08:33, timvolodine wrote:
> I think we could have a concurrency issue here due to Linux implementation
> running on a separate "notifier thread", where the callback is executed.

Done.

[Ken Rockot(use gerrit already), 2014-09-29 16:52:26]

device/ lgtm

[timvolodine, 2014-09-30 16:26:02]

ok looks good, just a few more comments.

https://codereview.chromium.org/592153002/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/592153002/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2094:
mojo_application_host_->Activate(this, GetHandle());
as qsr@ already mentioned on my patch this appears to be problematic (basically
crashing in a non-deterministic way). I've chatted with morrita@ regarding this
and he said he'll be looking how to fix this. until then looks like we cannot
land mojo patches :(

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
File device/battery/battery_monitor_impl.cc (right):

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_monitor_impl.cc:11: //static
nit: space before static

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
File device/battery/battery_status_service.cc (right):

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_status_service.cc:35:
BatteryStatusService::AddCallback(const BatteryUpdateCallback& callback) {
how about adding a check that this only runs on the right thread.
since you have the main_thread_task_runner_ I think it can be simply done with
"BelongsToCurrentThread()" DCHECK.

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_status_service.cc:76: const BatteryStatus& status) {
also I think we need DCHECK here that this always runs on main thread.

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_status_service.cc:77: status_ = status;
I think these two lines should come after the if(callback_list_.empty()) check.
Notify() can be executed after there are no more consumers anymore and in that
case status will be updated so that on next start a stale value will be
returned.

[ppi, 2014-10-01 16:27:49]

Patchset #11 (id:270001) has been deleted

[ppi, 2014-10-06 13:50:02]

Thanks Tim, ptal.

https://codereview.chromium.org/592153002/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/592153002/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2094:
mojo_application_host_->Activate(this, GetHandle());
On 2014/09/30 16:26:01, timvolodine wrote:
> as qsr@ already mentioned on my patch this appears to be problematic
(basically
> crashing in a non-deterministic way). I've chatted with morrita@ regarding
this
> and he said he'll be looking how to fix this. until then looks like we cannot
> land mojo patches :(

Yup, both patches need to wait for the fix before they land:(. Ack.

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
File device/battery/battery_monitor_impl.cc (right):

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_monitor_impl.cc:11: //static
On 2014/09/30 16:26:01, timvolodine wrote:
> nit: space before static

Done.

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
File device/battery/battery_status_service.cc (right):

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_status_service.cc:35:
BatteryStatusService::AddCallback(const BatteryUpdateCallback& callback) {
On 2014/09/30 16:26:01, timvolodine wrote:
> how about adding a check that this only runs on the right thread.
> since you have the main_thread_task_runner_ I think it can be simply done with
> "BelongsToCurrentThread()" DCHECK.

Done.

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_status_service.cc:76: const BatteryStatus& status) {
On 2014/09/30 16:26:02, timvolodine wrote:
> also I think we need DCHECK here that this always runs on main thread.

Done.

https://codereview.chromium.org/592153002/diff/250001/device/battery/battery_...
device/battery/battery_status_service.cc:77: status_ = status;
On 2014/09/30 16:26:01, timvolodine wrote:
> I think these two lines should come after the if(callback_list_.empty())
check.
> Notify() can be executed after there are no more consumers anymore and in that
> case status will be updated so that on next start a stale value will be
> returned.

Done.

[ppi, 2014-10-09 12:34:57]

Patchset #18 (id:430001) has been deleted

[timvolodine, 2014-10-21 11:39:41]

thanks ppi@! lgtm battery status api code :)

[ppi, 2014-10-21 12:14:54]

Thanks, Tim!

This should be ready for Jam's review now - Jam, could you take a look?

[ppi, 2014-10-22 21:57:45]

ppi@chromium.org changed reviewers:
+ raymes@chromium.org, viettrungluu@chromium.org

[ppi, 2014-10-22 22:02:27]

ppi@chromium.org changed reviewers:
- raymes@chromium.org

[ppi, 2014-10-22 22:02:28]

After chatting with jam@, adding Trung for the mojom files.

Trung, do you think there's anyone else that should take a look at this?

[viettrungluu, 2014-10-22 22:05:32]

https://codereview.chromium.org/592153002/diff/510001/device/battery/battery_...
File device/battery/battery_monitor.mojom (right):

https://codereview.chromium.org/592153002/diff/510001/device/battery/battery_...
device/battery/battery_monitor.mojom:11: interface BatteryMonitor {
I guess this interface would work, but there's something a bit unsatisfying
about it (e.g., the vagueness of the "periodically call ..." above).

Maybe it should be something like:

interface BatteryMonitor {
  StartObserving(BatteryMonitorObserver observer);
};

(Maybe StartObserving() would have some parameters to help specify something
about observation frequency; these could be added later.)

(Maybe it shouldn't even be BatteryMonitor, but just "Battery".)

That said, I'm not opposed to this interface as it stands now.

[ppi, 2014-10-23 15:42:40]

Thanks, Trung, please tal at the reply inline. Wdyt?

https://codereview.chromium.org/592153002/diff/510001/device/battery/battery_...
File device/battery/battery_monitor.mojom (right):

https://codereview.chromium.org/592153002/diff/510001/device/battery/battery_...
device/battery/battery_monitor.mojom:11: interface BatteryMonitor {
On 2014/10/22 22:05:31, viettrungluu wrote:
> I guess this interface would work, but there's something a bit unsatisfying
> about it (e.g., the vagueness of the "periodically call ..." above).
> 
> Maybe it should be something like:
> 
> interface BatteryMonitor {
>   StartObserving(BatteryMonitorObserver observer);
> };
> 
> (Maybe StartObserving() would have some parameters to help specify something
> about observation frequency; these could be added later.)
> 
> (Maybe it shouldn't even be BatteryMonitor, but just "Battery".)
> 
> That said, I'm not opposed to this interface as it stands now.

As for the observation frequency, I think that in this CL we aim at keeping the
existing contracts while transitioning from the old IPC mechanism to mojo IPC.
I'd be happy to discuss this separately (I'm sure Tim would be interested too).

I hear your point about vagueness of the interface, yet I'm not sure if
introducing the StartObserving bit would be worth it at this point - before we
have additional parameters to pass there's no functional change or value, right?

[jam, 2014-10-23 16:19:17]

content lgtm

[ppi, 2014-10-23 16:33:39]

ppi@chromium.org changed reviewers:
+ jln@chromium.org

[ppi, 2014-10-23 16:34:58]

+jln@ for the removal of battery_status_messages.h - ptal

[viettrungluu, 2014-10-24 01:21:34]

On 2014/10/23 15:42:40, ppi wrote:
> Thanks, Trung, please tal at the reply inline. Wdyt?
> 
>
https://codereview.chromium.org/592153002/diff/510001/device/battery/battery_...
> File device/battery/battery_monitor.mojom (right):
> 
>
https://codereview.chromium.org/592153002/diff/510001/device/battery/battery_...
> device/battery/battery_monitor.mojom:11: interface BatteryMonitor {
> On 2014/10/22 22:05:31, viettrungluu wrote:
> > I guess this interface would work, but there's something a bit unsatisfying
> > about it (e.g., the vagueness of the "periodically call ..." above).
> > 
> > Maybe it should be something like:
> > 
> > interface BatteryMonitor {
> >   StartObserving(BatteryMonitorObserver observer);
> > };
> > 
> > (Maybe StartObserving() would have some parameters to help specify something
> > about observation frequency; these could be added later.)
> > 
> > (Maybe it shouldn't even be BatteryMonitor, but just "Battery".)
> > 
> > That said, I'm not opposed to this interface as it stands now.
> 
> As for the observation frequency, I think that in this CL we aim at keeping
the
> existing contracts while transitioning from the old IPC mechanism to mojo IPC.
> I'd be happy to discuss this separately (I'm sure Tim would be interested
too).
> 
> I hear your point about vagueness of the interface, yet I'm not sure if
> introducing the StartObserving bit would be worth it at this point - before we
> have additional parameters to pass there's no functional change or value,
right?

It's mostly a question of how we structure interfaces.

I think this is fine as a starting point though, so LGTM (but see the note
below).

The only thing I'd consider changing would be to rename "BatteryMonitorClient"
to "BatteryStatusObserver". (If only we had some name aliasing feature in
.mojom.)

[jln (very slow on Chromium), 2014-10-24 01:23:19]

jln@chromium.org changed reviewers:
+ tsepez@chromium.org

[jln (very slow on Chromium), 2014-10-24 01:25:02]

I'm adding tsepez@ who followed the work on Mojo IPCs. This is the first CL that
I'm seeing moving to Mojo.

Should we add security owners to the mojom files? How should we review this?

[blundell, 2014-10-24 06:33:21]

On 2014/10/24 01:25:02, jln wrote:
> I'm adding tsepez@ who followed the work on Mojo IPCs. This is the first CL
that
> I'm seeing moving to Mojo.
> 
> Should we add security owners to the mojom files? How should we review this?

We made the decision that yes, mojom files should follow the same OWNER rules as
"*messages*" files. tsepez@ is a good security reviewer for mojom files I think.

[blundell, 2014-10-24 06:34:34]

On 2014/10/24 06:33:21, blundell wrote:
> On 2014/10/24 01:25:02, jln wrote:
> > I'm adding tsepez@ who followed the work on Mojo IPCs. This is the first CL
> that
> > I'm seeing moving to Mojo.
> > 
> > Should we add security owners to the mojom files? How should we review this?
> 
> We made the decision that yes, mojom files should follow the same OWNER rules
as
> "*messages*" files. tsepez@ is a good security reviewer for mojom files I
think.

(Sorry, when I say "we" made the decision, what I'm referring to is the
mojo-dev@ thread titled "Review process for (internal) mojo APIs).

[Tom Sepez, 2014-10-24 16:43:47]

LGTM once comment is addressed.

https://codereview.chromium.org/592153002/diff/510001/device/battery/OWNERS
File device/battery/OWNERS (right):

https://codereview.chromium.org/592153002/diff/510001/device/battery/OWNERS#n...
device/battery/OWNERS:1: timvolodine@chromium.org
add:
per-file *.mojom=set noparent
per-file *.mojom=tsepez@chromium.org

[ppi, 2014-10-27 15:40:59]

Thanks, Tom!

Trung, I renamed BatteryMonitorClient to BatteryStatusObserver as suggested,
thanks!

https://codereview.chromium.org/592153002/diff/510001/device/battery/OWNERS
File device/battery/OWNERS (right):

https://codereview.chromium.org/592153002/diff/510001/device/battery/OWNERS#n...
device/battery/OWNERS:1: timvolodine@chromium.org
On 2014/10/24 16:43:47, Tom Sepez wrote:
> add:
> per-file *.mojom=set noparent
> per-file mailto:*.mojom=tsepez@chromium.org

Done.

[ppi, 2014-10-27 15:42:02]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-10-27 15:43:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/592153002/570001

[commit-bot: I haz the power, 2014-10-27 16:48:39]

Committed patchset #23 (id:570001)

[commit-bot: I haz the power, 2014-10-27 16:49:47]

Patchset 23 (id:??) landed as
https://crrev.com/9e6d2387b6d9b28ab6defe7f5feb715a5abbc242
Cr-Commit-Position: refs/heads/master@{#301380}

[leviw_travelin_and_unemployed, 2014-10-27 18:50:40]

A revert of this CL (patchset #23 id:570001) has been created in
https://codereview.chromium.org/676953005/ by leviw@chromium.org.

The reason for reverting is: Broke battery-status LayoutTests:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=bat....