Stop using [Client=...] feature of Mojom for BatteryMonitor

Stop using [Client=...] feature of Mojom for BatteryMonitor

Change BatteryMonitor to instead have a QueryNextStatus() method that clients can use to poll for changes to battery status. The first call will complete immediately, reporting the current battery status. After that, QueryNextStatus() will "hang" until there is a change to report.

This also solves a defect (anti-pattern) with the older design, which could result in the service consuming a lot of memory. That could happen if the client failed to consume any of the DidChange messages. Eventually, the pipe would back up, and queuing of MojoWriteMessage calls would occur in the service's process. By switching to a polling design, this problem is eliminated.

See further description of the anti-pattern here: https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo

Committed: https://crrev.com/3e80dbd6aab92f759c262b10fe6fee902e9a6d15
Cr-Commit-Position: refs/heads/master@{#310539}

[jamesr, 2015-01-05 23:33:09]

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

[jamesr, 2015-01-05 23:33:10]

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

https://codereview.chromium.org/833983002/diff/30001/content/renderer/battery...
content/renderer/battery_status/battery_status_dispatcher.cc:19:
RenderThread::Get()->GetServiceRegistry()->ConnectToRemoteService(&monitor_);
i'm not as familiar with content::'s service registry hookup, but it feels weird
to first have this code connect to a named Service only to tell it to set a
client back.  Can we instead just ask the service to connect to us directly and
cut out the Client thingy completely?

In the pure-mojo world, this would work by connecting to the battery monitor
service (by url) and then exposing a monitor interface on the service provider
that's provided to the battery monitor.  so the mojo messages for 'app' to start
listening to battery status updates would be:

app->shell ConnectToApplication("mojo:battery_status", ..)
shell->battery_status_service AcceptConnection(app_url, ...)
battery_status_service->app ConnectToService("device.BatteryMonitor", ...) (via
the mojo.ServiceProvider pipe)
battery_status_service->app DidChange(xxx) (via the device.BatteryMonitor pipe)

https://codereview.chromium.org/833983002/diff/30001/device/battery/battery_m...
File device/battery/battery_monitor.mojom (right):

https://codereview.chromium.org/833983002/diff/30001/device/battery/battery_m...
device/battery/battery_monitor.mojom:10: interface BatteryMonitor {
can we get rid of this interface completely? it doesn't add much

https://codereview.chromium.org/833983002/diff/30001/device/battery/battery_m...
device/battery/battery_monitor.mojom:14: interface BatteryMonitorClient {
and just name this BatteryMonitor?

[darin (slow to review), 2015-01-06 00:52:12]

On 2015/01/05 23:33:10, jamesr wrote:
>
https://codereview.chromium.org/833983002/diff/30001/content/renderer/battery...
> File content/renderer/battery_status/battery_status_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/833983002/diff/30001/content/renderer/battery...
> content/renderer/battery_status/battery_status_dispatcher.cc:19:
> RenderThread::Get()->GetServiceRegistry()->ConnectToRemoteService(&monitor_);
> i'm not as familiar with content::'s service registry hookup, but it feels
weird
> to first have this code connect to a named Service only to tell it to set a
> client back.  Can we instead just ask the service to connect to us directly
and
> cut out the Client thingy completely?
> 
> In the pure-mojo world, this would work by connecting to the battery monitor
> service (by url) and then exposing a monitor interface on the service provider
> that's provided to the battery monitor.  so the mojo messages for 'app' to
start
> listening to battery status updates would be:
> 
> app->shell ConnectToApplication("mojo:battery_status", ..)
> shell->battery_status_service AcceptConnection(app_url, ...)
> battery_status_service->app ConnectToService("device.BatteryMonitor", ...)
(via
> the mojo.ServiceProvider pipe)
> battery_status_service->app DidChange(xxx) (via the device.BatteryMonitor
pipe)

Hmm, there's a ServiceProvider between renderer and browser processes. It is
on that ServiceProvider that the BatteryMonitor interface is "queried". We
could probably arrange for an alternative to ConnectToRemoteService that allows
the caller to pass a proxy to a locally implemented interface to the remote
service for the remote service to call methods on. I'm not sure how to concisely
describe that.

However, another way to look at it is that BatteryMonitor could grow other
methods in the future. For example, it could have a method to query the current
battery status in one-shot fashion. The setClient method could also change to
a start method, and in addition to a client interface it could also have a param
to set the frequency of updates. The BatteryMonitor could also have a stop
method.
It could be a controller interface in the general sense. This could be a
reasonable
design pattern for more things like the battery monitor. Not everything can be
modeled as a simple sink to be connected to a named remote service. More things
can
be modeled as a sink connected to a named remote service with parameters.

Hmm...


>
https://codereview.chromium.org/833983002/diff/30001/device/battery/battery_m...
> File device/battery/battery_monitor.mojom (right):
> 
>
https://codereview.chromium.org/833983002/diff/30001/device/battery/battery_m...
> device/battery/battery_monitor.mojom:10: interface BatteryMonitor {
> can we get rid of this interface completely? it doesn't add much
> 
>
https://codereview.chromium.org/833983002/diff/30001/device/battery/battery_m...
> device/battery/battery_monitor.mojom:14: interface BatteryMonitorClient {
> and just name this BatteryMonitor?

[darin (slow to review), 2015-01-07 08:58:51]

darin@chromium.org changed reviewers:
+ ppi@chromium.org, timvolodine@chromium.org
- jamesr@chromium.org

[darin (slow to review), 2015-01-07 08:58:52]

+ppi to review java bits
+tim to review c++ bits

+jamesr -> FYI, but feedback welcomed

[qsr, 2015-01-07 09:04:59]

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

[qsr, 2015-01-07 09:05:00]

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

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor.mojom:14: QueryNextStatus() => (BatteryStatus
status);
Any reason you choose this design, instead of having something like:

  StartObserving(BatteryStatusObserver observer);

The reason I asked is because this doubled the number of message for this API =>
Until now, the monitor was sending all updates, now the client must call this
method back each time it reveives an update.

[timvolodine, 2015-01-07 14:38:51]

c++ bits look fine to me in general. just a few comments/questions below:

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
File device/battery/battery_monitor.mojom (left):

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor.mojom:10: [Client=BatteryStatusObserver]
does this mean the [Client=..] feature is deprecated and should not be used
anymore? the idea seemed kind of nice and resulted in simpler monitor
implementation. I understand the new design is because we care about
producer/consumer synchronization. If this is a general concern maybe there is a
way to have a mojo client primitive which would not queue and transmit only the
last data (similar to what we have with shared memory).

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

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor.mojom:13: // supported.
description for this patch talks about an anti-pattern issue - is there any bug
associated with this? if so maybe add it to description.

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

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor_impl.h:38: std::deque<BatteryStatusCallback>
callbacks_;
nit: std::vector is probably sufficient here?

[darin (slow to review), 2015-01-07 19:32:57]

On 2015/01/07 09:05:00, qsr wrote:
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> File device/battery/battery_monitor.mojom (right):
> 
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> device/battery/battery_monitor.mojom:14: QueryNextStatus() => (BatteryStatus
> status);
> Any reason you choose this design, instead of having something like:
> 
>   StartObserving(BatteryStatusObserver observer);
> 
> The reason I asked is because this doubled the number of message for this API
=>
> Until now, the monitor was sending all updates, now the client must call this
> method back each time it reveives an update.

Yes, please see the CL description. The StartObserving approach is
an anti-pattern as it can bloat the sender's process if the client
is not reading the pipe.

[darin (slow to review), 2015-01-07 19:35:34]

On 2015/01/07 14:38:51, timvolodine wrote:
> c++ bits look fine to me in general. just a few comments/questions below:
> 
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> File device/battery/battery_monitor.mojom (left):
> 
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> device/battery/battery_monitor.mojom:10: [Client=BatteryStatusObserver]
> does this mean the [Client=..] feature is deprecated and should not be used
> anymore? the idea seemed kind of nice and resulted in simpler monitor
> implementation. I understand the new design is because we care about
> producer/consumer synchronization. If this is a general concern maybe there is
a
> way to have a mojo client primitive which would not queue and transmit only
the
> last data (similar to what we have with shared memory).

There is a lengthy discussion thread on mojo-dev@ explaining why we are
deprecating [Client=..]. In short, we are trying to more faithfully model
objects, which just have methods.


>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> File device/battery/battery_monitor.mojom (right):
> 
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> device/battery/battery_monitor.mojom:13: // supported.
> description for this patch talks about an anti-pattern issue - is there any
bug
> associated with this? if so maybe add it to description.

I will create a bug or post mojo-dev@ and reference that.


> 
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> File device/battery/battery_monitor_impl.h (right):
> 
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> device/battery/battery_monitor_impl.h:38: std::deque<BatteryStatusCallback>
> callbacks_;
> nit: std::vector is probably sufficient here?

Yes.

[ppi, 2015-01-07 19:37:36]

https://codereview.chromium.org/833983002/diff/250001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/833983002/diff/250001/android_webview/java_li...
android_webview/java_library_common.mk:78: $(call
intermediates-dir-for,GYP,shared)/java_mojo/device_battery_mojo_bindings/src/org/chromium/mojom/device/BatteryStatusObserver_Internal.java
This line should go away too.

https://codereview.chromium.org/833983002/diff/250001/device/battery/android/...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
(right):

https://codereview.chromium.org/833983002/diff/250001/device/battery/android/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:23:
private boolean mStatusToReport;
nit: maybe "mHasStatusToReport"?

https://codereview.chromium.org/833983002/diff/250001/device/battery/android/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:54:
if (mStatusToReport)
As per AOSP Java style guide[1], this should be either

  if (mStatusToReport) reportStatus();

or

  if (mStatusToReport) {
      reportStatus();
  }

[1] https://source.android.com/source/code-style.html - "Use standard brace
style"

https://codereview.chromium.org/833983002/diff/250001/device/battery/android/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:62:
if (!mCallbacks.isEmpty())
As mentioned above - for conditionals we need to add braces or put the entire
statement in one line.

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

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor.mojom:13: // supported.
Did you mean s/supported/not supported/?

[ppi, 2015-01-07 19:41:12]

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

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor.mojom:13: // supported.
Ach, you probably did mean "supported", please disregard.

[darin (slow to review), 2015-01-07 20:39:53]

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

https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
device/battery/battery_monitor.mojom:13: // supported.
On 2015/01/07 19:41:12, ppi wrote:
> Ach, you probably did mean "supported", please disregard.

Yup, this is what the queuing of callbacks is all about in the implementation.

[darin (slow to review), 2015-01-07 21:10:18]

darin@chromium.org changed reviewers:
+ tsepez@chromium.org
- qsr@chromium.org

[darin (slow to review), 2015-01-07 21:10:19]

Thanks for the feedback. PTAL

+tsepez for IPC review

[Tom Sepez, 2015-01-07 21:26:02]

Messages LGTM

[timvolodine, 2015-01-07 22:50:22]

On 2015/01/07 19:35:34, darin wrote:
> On 2015/01/07 14:38:51, timvolodine wrote:
>
https://codereview.chromium.org/833983002/diff/250001/device/battery/battery_...
> > device/battery/battery_monitor.mojom:10: [Client=BatteryStatusObserver]
> > does this mean the [Client=..] feature is deprecated and should not be used
> > anymore? the idea seemed kind of nice and resulted in simpler monitor
> > implementation. I understand the new design is because we care about
> > producer/consumer synchronization. If this is a general concern maybe there
is
> a
> > way to have a mojo client primitive which would not queue and transmit only
> the
> > last data (similar to what we have with shared memory).
> 
> There is a lengthy discussion thread on mojo-dev@ explaining why we are
> deprecating [Client=..]. In short, we are trying to more faithfully model
> objects, which just have methods.
>

lgtm

thanks for the clarification, very interesting mojo-dev thread. I was wondering
if this is now the preferred way to model APIs, which it appears to be since
"Clients" and "StartObserving()" are considered anti-patterns.

also curious if it would make sense to add some throttling/congestion management
support to mojo messaging itself in the future..

[ppi, 2015-01-07 23:35:16]

lgtm % comment, thanks

https://codereview.chromium.org/833983002/diff/270001/android_webview/java_li...
File android_webview/java_library_common.mk (right):

https://codereview.chromium.org/833983002/diff/270001/android_webview/java_li...
android_webview/java_library_common.mk:78: $(call
intermediates-dir-for,GYP,shared)/java_mojo/device_battery_mojo_bindings/src/org/chromium/mojom/device/BatteryStatusObserver_Internal.java
Not sure if you saw the comment on the previous patchset - I think this line
should go away too. Like the one above, it's code generated for
BatterryStatusObserver.

[darin (slow to review), 2015-01-08 17:06:49]

On 2015/01/07 22:50:22, timvolodine wrote:
...
> also curious if it would make sense to add some throttling/congestion
management
> support to mojo messaging itself in the future..

it very well may.

[darin (slow to review), 2015-01-08 17:07:00]

On 2015/01/07 23:35:16, ppi wrote:
> lgtm % comment, thanks
> 
>
https://codereview.chromium.org/833983002/diff/270001/android_webview/java_li...
> File android_webview/java_library_common.mk (right):
> 
>
https://codereview.chromium.org/833983002/diff/270001/android_webview/java_li...
> android_webview/java_library_common.mk:78: $(call
>
intermediates-dir-for,GYP,shared)/java_mojo/device_battery_mojo_bindings/src/org/chromium/mojom/device/BatteryStatusObserver_Internal.java
> Not sure if you saw the comment on the previous patchset - I think this line
> should go away too. Like the one above, it's code generated for
> BatterryStatusObserver.

oops, thanks!

[darin (slow to review), 2015-01-08 17:22:32]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2015-01-08 17:24:01]

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

[commit-bot: I haz the power, 2015-01-08 18:21:18]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2015-01-08 18:24:13]

Patchset 16 (id:??) landed as
https://crrev.com/3e80dbd6aab92f759c262b10fe6fee902e9a6d15
Cr-Commit-Position: refs/heads/master@{#310539}