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

Review URL: https://codereview.chromium.org/833983002

Cr-Commit-Position: refs/heads/master@{#310539}

5 files changed, 74 insertions, 25 deletions

diff --git a/device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java b/device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
index e441589..364e21a 100644
--- a/device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
+++ b/device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
@@ -7,7 +7,9 @@ package org.chromium.device.battery;
 import org.chromium.mojo.system.MojoException;
 import org.chromium.mojom.device.BatteryMonitor;
 import org.chromium.mojom.device.BatteryStatus;
-import org.chromium.mojom.device.BatteryStatusObserver;
+
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Android implementation of the battery monitor service defined in
@@ -16,12 +18,15 @@ import org.chromium.mojom.device.BatteryStatusObserver;
 public class BatteryMonitorImpl implements BatteryMonitor {
     // Factory that created this instance and notifies it about battery status changes.
     private final BatteryMonitorFactory mFactory;
+    private final List<QueryNextStatusResponse> mCallbacks;
+    private BatteryStatus mStatus;
+    private boolean mHasStatusToReport;
     private boolean mSubscribed;
 
-    private BatteryStatusObserver mClient;
-
     public BatteryMonitorImpl(BatteryMonitorFactory batteryMonitorFactory) {
         mFactory = batteryMonitorFactory;
+        mCallbacks = new ArrayList<QueryNextStatusResponse>();
+        mHasStatusToReport = false;
         mSubscribed = true;
     }
 
@@ -33,11 +38,6 @@ public class BatteryMonitorImpl implements BatteryMonitor {
     }
 
     @Override
-    public void setClient(BatteryStatusObserver client) {
-        mClient = client;
-    }
-
-    @Override
     public void close() {
         unsubscribe();
     }
@@ -47,12 +47,29 @@ public class BatteryMonitorImpl implements BatteryMonitor {
         unsubscribe();
     }
 
-    /**
-     * Notifies the client passing the given battery status information.
-     */
+    @Override
+    public void queryNextStatus(QueryNextStatusResponse callback) {
+        mCallbacks.add(callback);
+
+        if (mHasStatusToReport) {
+            reportStatus();
+        }
+    }
+
     void didChange(BatteryStatus batteryStatus) {
-        if (mClient != null) {
-            mClient.didChange(batteryStatus);
+        mStatus = batteryStatus;
+        mHasStatusToReport = true;
+
+        if (!mCallbacks.isEmpty()) {
+            reportStatus();
         }
-    };
+    }
+
+    void reportStatus() {
+        for (QueryNextStatusResponse callback : mCallbacks) {
+            callback.call(mStatus);
+        }
+        mCallbacks.clear();
+        mHasStatusToReport = false;
+    }
 }
diff --git a/device/battery/battery_monitor.mojom b/device/battery/battery_monitor.mojom
index 880685e..88a43d2 100644
--- a/device/battery/battery_monitor.mojom
+++ b/device/battery/battery_monitor.mojom
@@ -6,11 +6,10 @@ module device;
 
 import "device/battery/battery_status.mojom";
 
-// A BatteryMonitor will periodically call its client's DidChange method.
-[Client=BatteryStatusObserver]
 interface BatteryMonitor {
-};
-
-interface BatteryStatusObserver {
-  DidChange(BatteryStatus status);
+  // Battery status is reported once it changes or immediately if this is the
+  // first call to QueryNextStatus on this instance. QueryNextStatus calls may
+  // be throttled by the service. Overlapping calls to QueryNextStatus are
+  // supported.
+  QueryNextStatus() => (BatteryStatus status);
 };
diff --git a/device/battery/battery_monitor_impl.cc b/device/battery/battery_monitor_impl.cc
index dc07b62..62cbf60 100644
--- a/device/battery/battery_monitor_impl.cc
+++ b/device/battery/battery_monitor_impl.cc
@@ -17,20 +17,41 @@ void BatteryMonitorImpl::Create(
 BatteryMonitorImpl::BatteryMonitorImpl(
     mojo::InterfaceRequest<BatteryMonitor> request)
     : binding_(this, request.Pass()),
-      subscription_(BatteryStatusService::GetInstance()->AddCallback(
-          base::Bind(&BatteryMonitorImpl::DidChange, base::Unretained(this)))) {
+      status_to_report_(false) {
+  // NOTE: DidChange may be called before AddCallback returns. This is done to
+  // report current status.
+  subscription_ = BatteryStatusService::GetInstance()->AddCallback(
+      base::Bind(&BatteryMonitorImpl::DidChange, base::Unretained(this)));
 }
 
 BatteryMonitorImpl::~BatteryMonitorImpl() {
 }
 
+void BatteryMonitorImpl::QueryNextStatus(
+    const BatteryStatusCallback& callback) {
+  callbacks_.push_back(callback);
+
+  if (status_to_report_)
+    ReportStatus();
+}
+
 void BatteryMonitorImpl::RegisterSubscription() {
 }
 
 void BatteryMonitorImpl::DidChange(const BatteryStatus& battery_status) {
-  BatteryStatusPtr status(BatteryStatus::New());
-  *status = battery_status;
-  binding_.client()->DidChange(status.Pass());
+  status_ = battery_status;
+  status_to_report_ = true;
+
+  if (!callbacks_.empty())
+    ReportStatus();
+}
+
+void BatteryMonitorImpl::ReportStatus() {
+  for (const auto& callback : callbacks_)
+    callback.Run(status_.Clone());
+  callbacks_.clear();
+
+  status_to_report_ = false;
 }
 
 }  // namespace device
diff --git a/device/battery/battery_monitor_impl.h b/device/battery/battery_monitor_impl.h
index 7916b0d..49e85708 100644
--- a/device/battery/battery_monitor_impl.h
+++ b/device/battery/battery_monitor_impl.h
@@ -5,6 +5,8 @@
 #ifndef DEVICE_BATTERY_BATTERY_MONITOR_IMPL_H_
 #define DEVICE_BATTERY_BATTERY_MONITOR_IMPL_H_
 
+#include <vector>
+
 #include "base/memory/scoped_ptr.h"
 #include "device/battery/battery_export.h"
 #include "device/battery/battery_monitor.mojom.h"
@@ -19,14 +21,23 @@ class BatteryMonitorImpl : public BatteryMonitor {
       mojo::InterfaceRequest<BatteryMonitor> request);
 
  private:
+  typedef mojo::Callback<void(BatteryStatusPtr)> BatteryStatusCallback;
+
   explicit BatteryMonitorImpl(mojo::InterfaceRequest<BatteryMonitor> request);
   ~BatteryMonitorImpl() override;
 
+  // BatteryMonitor methods:
+  void QueryNextStatus(const BatteryStatusCallback& callback) override;
+
   void RegisterSubscription();
   void DidChange(const BatteryStatus& battery_status);
+  void ReportStatus();
 
   mojo::StrongBinding<BatteryMonitor> binding_;
   scoped_ptr<BatteryStatusService::BatteryUpdateSubscription> subscription_;
+  std::vector<BatteryStatusCallback> callbacks_;
+  BatteryStatus status_;
+  bool status_to_report_;
 
   DISALLOW_COPY_AND_ASSIGN(BatteryMonitorImpl);
 };
diff --git a/device/battery/battery_status_service.h b/device/battery/battery_status_service.h
index fd52b6b..a4cfac7d 100644
--- a/device/battery/battery_status_service.h
+++ b/device/battery/battery_status_service.h
@@ -30,6 +30,7 @@ class DEVICE_BATTERY_EXPORT BatteryStatusService {
 
   // Adds a callback to receive battery status updates.  Must be called on the
   // main thread. The callback itself will be called on the main thread as well.
+  // NOTE: The callback may be run before AddCallback returns!
   scoped_ptr<BatteryUpdateSubscription> AddCallback(
       const BatteryUpdateCallback& callback);