battery-status: Implement the battery-status API for chromeos.

content/browser/battery_status/battery_status_manager_chromeos.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/battery_status/battery_status_manager.h"
+
+#include "base/memory/weak_ptr.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
+#include "chromeos/dbus/power_manager_client.h"
+#include "content/public/browser/browser_thread.h"
+#include "third_party/WebKit/public/platform/WebBatteryStatus.h"
+
+namespace content {
+
+namespace {
+
+void RemoveObserverOnUI(chromeos::PowerManagerClient::Observer* observer) {
+  chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(
+      observer);
+}
+
+class BatteryStatusManagerChromeOS
+    : public BatteryStatusManager,
+      public chromeos::PowerManagerClient::Observer {
+ public:
+  explicit BatteryStatusManagerChromeOS(
+      const BatteryStatusService::BatteryUpdateCallback& callback)
+      : callback_(callback), currently_listening_(false), weak_factory_(this) {}
+
+  virtual ~BatteryStatusManagerChromeOS() {
+    if (!currently_listening_)

[timvolodine, 2014/07/09 11:35:01]

This looks like there is a potential race condition here. What if on lets say on
IO thread:
StartListeningBatteryChange()
~BatteryStatusManager()
at the time of destructor it has not started yet, so it returns early. Then
start is invoked on the UI thread (before the actual destruction of weak
reference) so the overall result is that the observer is never removed. Removing
the "if (!currently_listening_)" check might resolve this, but in general it
looks we can have destruction in parallel with StartOnUI execution which sounds
scary. Probably the easiest way is to make the destructor obligatory on UI
thread as it was before (+maybe add a comment to clarify).

[sadrul, 2014/07/13 09:04:54]

I have moved the main code out of this into a RefCountedThreadSafe<> object,
which performs all the relevant tasks in the UI thread. I think that takes care
of the concerns here.

+      return;
+
+    if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+      StopOnUI();
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&RemoveObserverOnUI, base::Unretained(this)));
+    }
+  }
+
+ private:
+  bool IsBatteryPresent(
+      const power_manager::PowerSupplyProperties& proto) const {
+    return proto.battery_state() !=
+           power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
+  }
+
+  bool IsUsbChargerConnected(
+      const power_manager::PowerSupplyProperties& proto) const {
+    return proto.external_power() ==
+           power_manager::PowerSupplyProperties_ExternalPower_USB;
+  }
+
+  bool IsBatteryCharging(
+      const power_manager::PowerSupplyProperties& proto) const {
+    return proto.battery_state() !=
+           power_manager::PowerSupplyProperties_BatteryState_DISCHARGING;
+  }
+
+  bool IsBatteryFull(const power_manager::PowerSupplyProperties& proto) const {
+    return proto.battery_state() ==
+           power_manager::PowerSupplyProperties_BatteryState_FULL;
+  }
+
+  double GetBatteryLevel(
+      const power_manager::PowerSupplyProperties& proto) const {
+    const double kMaxBatteryLevelProto = 100.f;
+    return proto.battery_percent() / kMaxBatteryLevelProto;
+  }
+
+  void StartOnUI() {

[timvolodine, 2014/07/09 11:35:01]

not sure if this is relevant for chromeOS, but alternatively I am considering a
dedicated thread to receive notifications on and to offload the UI thread on
desktop systems (e.g. macOS).

[sadrul, 2014/07/13 09:04:54]

In the case of Chrome OS, the interaction with the PowerManagerClient needs to
always happen in the UI thread. So it will still be necessary to bounce between
threads.

[timvolodine, 2014/07/14 17:38:56]

fyi: on macOS it is sufficient to have a thread with UI-type message loop.

+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (currently_listening_)
+      return;
+    chromeos::PowerManagerClient* power_client =
+        chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
+    power_client->AddObserver(this);
+    power_client->RequestStatusUpdate();
+    currently_listening_ = true;
+  }
+
+  void StopOnUI() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (!currently_listening_)
+      return;
+    chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(
+        this);
+    currently_listening_ = false;
+  }
+
+  // BatteryStatusManager:
+  virtual bool StartListeningBatteryChange() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    weak_factory_.InvalidateWeakPtrs();
+    BrowserThread::PostTask(BrowserThread::UI,
+                            FROM_HERE,
+                            base::Bind(&BatteryStatusManagerChromeOS::StartOnUI,
+                                       weak_factory_.GetWeakPtr()));

[timvolodine, 2014/07/09 11:35:01]

Probably false alarm, but I was curious whether WeakPtr was thread-safe. In
callback.h it says they are not:
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&q=...
however in weak_ptr.h the comments appear to vaguely allow this:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

wdyt?

[sadrul, 2014/07/13 09:04:54]

Good catch! It looks like WeakPtr indeed is not thread-safe. The comment in
callback.h implies the weak-ptr cannot be dereferenced in another thread, and
for the callback to run, we do need to do that. I think the comment in
weak_ptr.h allows using the weak-ptr in another thread in a manner that it is
dereferenced only in the originating thread (e.g. to post-task back to the first
thread).

As commented above, I am using a RefCountedThreadSafe<> object to get around
this issue.

+    return true;
+  }
+
+  virtual void StopListeningBatteryChange() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    weak_factory_.InvalidateWeakPtrs();
+    BrowserThread::PostTask(BrowserThread::UI,
+                            FROM_HERE,
+                            base::Bind(&BatteryStatusManagerChromeOS::StopOnUI,
+                                       weak_factory_.GetWeakPtr()));
+  }
+
+  // chromeos::PowerManagerClient::Observer:
+  virtual void PowerChanged(
+      const power_manager::PowerSupplyProperties& proto) OVERRIDE {
+    blink::WebBatteryStatus status;
+    // Use the default values if there is no battery in the system.
+    if (IsBatteryPresent(proto)) {
+      // The charging status is unreliable if a low power charger is connected
+      // (i.e. usb).
+      bool status_unreliable = IsUsbChargerConnected(proto);
+      // Battery time is unreliable if it is still being computed.
+      bool time_unreliable =
+          status_unreliable || proto.is_calculating_battery_time();
+
+      // Set |charging| only if the status is reliable. Otherwise, keep the
+      // default (which is |true|).
+      if (!status_unreliable)
+        status.charging = IsBatteryCharging(proto);
+
+      // Set |chargingTime| to +infinity if the battery is discharging, or if
+      // the time is unreliable. Keep the default value (which is 0) if the
+      // battery is full.
+      if (time_unreliable || !status.charging)
+        status.chargingTime = std::numeric_limits<double>::infinity();
+      else if (!IsBatteryFull(proto))
+        status.chargingTime = proto.battery_time_to_full_sec();
+
+      // Keep the default value for |dischargingTime| (which is +infinity) if
+      // the time is unreliable, or if the battery is charging.
+      if (!time_unreliable && !status.charging)
+        status.dischargingTime = proto.battery_time_to_empty_sec();
+
+      status.level = GetBatteryLevel(proto);
+    }
+    callback_.Run(status);
+  }
+
+  BatteryStatusService::BatteryUpdateCallback callback_;
+  bool currently_listening_;
+  base::WeakPtrFactory<BatteryStatusManagerChromeOS> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatteryStatusManagerChromeOS);
+};
+
+}  // namespace
+
+// static
+scoped_ptr<BatteryStatusManager> BatteryStatusManager::Create(
+    const BatteryStatusService::BatteryUpdateCallback& callback) {
+  return scoped_ptr<BatteryStatusManager>(
+      new BatteryStatusManagerChromeOS(callback));
+}
+
+}  // namespace content