Battery Status API: implementation for Linux.

content/browser/battery_status/battery_status_manager_linux.cc

Index: content/browser/battery_status/battery_status_manager_linux.cc
diff --git a/content/browser/battery_status/battery_status_manager_linux.cc b/content/browser/battery_status/battery_status_manager_linux.cc
new file mode 100644
index 0000000000000000000000000000000000000000..23756133383182987024c2ff954dbac78b4b42d9
--- /dev/null
+++ b/content/browser/battery_status/battery_status_manager_linux.cc
@@ -0,0 +1,306 @@
+// 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/macros.h"
+#include "base/threading/thread.h"
+#include "base/values.h"
+#include "content/public/browser/browser_thread.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_path.h"
+#include "dbus/object_proxy.h"
+#include "dbus/values_util.h"
+#include "third_party/WebKit/public/platform/WebBatteryStatus.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"

[mlamouri (slow - plz ping), 2014/08/01 17:14:14]

I think you should remove that #include. Isn't it for Chrome OS? It sounds wrong
to use Chrome OS stuff in battery_status_manager_linux.cc.

[timvolodine, 2014/08/06 16:37:42]

Done.

+
+namespace content {
+
+namespace {
+
+const char kUPowerServiceName[] = "org.freedesktop.UPower";
+const char kUPowerDeviceName[] = "org.freedesktop.UPower.Device";
+const char kUPowerPath[] = "/org/freedesktop/UPower";
+const char kUPowerDeviceSignalChanged[] = "Changed";

[mlamouri (slow - plz ping), 2014/08/01 17:14:13]

I think you can use "DeviceChanged" instead of "Changed". In which case, you
could check at the beginning of the callback whether the change is related to
the device you are currently tracking (ie. |battery_proxy_|).

[timvolodine, 2014/08/06 16:37:42]

UPower.Device only has the Changed() signal, in line 140 we explicitly register
for a specific battery: "battery_proxy_->ConnectToSignal("..

[mlamouri (slow - plz ping), 2014/08/07 16:17:04]

My bad, I misread. I didn't see that you were registering to Device.Changed.

+const char kBatteryWatcherThreadName[] = "BatteryStatusWatcher";
+
+// intended for unit32, int64, uint64, double.
+template<typename T>
+T GetProperty(const base::DictionaryValue& dictionary,
+              const std::string& property_name,
+              T default_value) {
+  double value;
+  return dictionary.GetDouble(property_name, &value) ? value : default_value;
+}
+
+// bool
+template<>
+bool GetProperty(const base::DictionaryValue& dictionary,
+                 const std::string& property_name,
+                 bool default_value) {
+  bool value;
+  return dictionary.GetBoolean(property_name, &value) ? value : default_value;
+}
+
+scoped_ptr<base::DictionaryValue> GetPropertiesAsDictionary(
+    dbus::ObjectProxy* proxy) {
+  dbus::MethodCall method_call(dbus::kDBusPropertiesInterface,
+                               dbus::kDBusPropertiesGetAll);
+  dbus::MessageWriter builder(&method_call);
+  builder.AppendString(kUPowerDeviceName);
+
+  scoped_ptr<dbus::Response> response(
+      proxy->CallMethodAndBlock(&method_call,
+                                dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+  if (response) {
+    dbus::MessageReader reader(response.get());
+    scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
+    base::DictionaryValue* dictionary_value = NULL;
+    if (value && value->GetAsDictionary(&dictionary_value)) {
+      ignore_result(value.release());
+      return scoped_ptr<base::DictionaryValue>(dictionary_value);
+    }
+  }
+  return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue());
+}
+
+void GetPowerSourcesPaths(dbus::ObjectProxy* proxy,
+                          std::vector<dbus::ObjectPath>& paths) {
+  if (!proxy)
+    return;
+
+  dbus::MethodCall method_call(kUPowerServiceName, "EnumerateDevices");
+  scoped_ptr<dbus::Response> response(proxy->CallMethodAndBlock(&method_call,
+      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+
+  if (response) {
+    dbus::MessageReader reader(response.get());
+    if (reader.PopArrayOfObjectPaths(&paths))
+      return;
+  }
+  paths.clear();
+}
+
+class BatteryStatusNotificationThread : public base::Thread {
+ public:
+  BatteryStatusNotificationThread(
+      const BatteryStatusService::BatteryUpdateCallback& callback)
+      : base::Thread(kBatteryWatcherThreadName),
+        callback_(callback),
+        battery_proxy_(0) {}
+
+  virtual ~BatteryStatusNotificationThread() {
+    if (system_bus_)
+      ShutdownDBus();
+    Stop();
+  }
+
+  void StartListening() {
+    DCHECK(OnWatcherThread());
+
+    if (system_bus_)
+      return;
+
+    InitDBus();
+    dbus::ObjectProxy* power_proxy =
+        system_bus_->GetObjectProxy(kUPowerServiceName,
+                                    dbus::ObjectPath(kUPowerPath));
+    std::vector<dbus::ObjectPath> device_paths;
+    GetPowerSourcesPaths(power_proxy, device_paths);
+
+    for (size_t i = 0; i < device_paths.size(); ++i) {
+      const dbus::ObjectPath& device_path = device_paths[i];
+      dbus::ObjectProxy* device_proxy = system_bus_->GetObjectProxy(
+          kUPowerServiceName, device_path);
+      scoped_ptr<base::DictionaryValue> dictionary =
+          GetPropertiesAsDictionary(device_proxy);
+
+      bool is_present = GetProperty<bool>(*dictionary, "IsPresent", false);
+      uint32 type = GetProperty<uint32>(*dictionary, "Type", 0);
+
+      if (is_present && type == 2 /* battery */) {
+        if (battery_proxy_) {
+          // TODO(timvolodine): add support for multiple batteries. Currently we

[Michael van Ouwerkerk, 2014/08/01 15:24:39]

Please file a bug for this. It would be nice to have UMA logging as well, which
would be suitable as a TODO.

[timvolodine, 2014/08/06 16:37:42]

Done.

+          // only collect information from the first battery we encounter.
+          LOG(WARNING) << "multiple batteries found, "
+                       << "using status data of the first battery only.";
+        } else {
+          battery_proxy_ = device_proxy;
+        }
+      }
+    }
+
+    if (!battery_proxy_) {
+      callback_.Run(blink::WebBatteryStatus());
+      return;
+    }
+
+    battery_proxy_->ConnectToSignal(
+        kUPowerDeviceName,
+        kUPowerDeviceSignalChanged,
+        base::Bind(&BatteryStatusNotificationThread::BatteryChanged,
+                   base::Unretained(this)),
+        base::Bind(&BatteryStatusNotificationThread::OnSignalConnected,
+                   base::Unretained(this)));
+  }
+
+  void StopListening() {
+    DCHECK(OnWatcherThread());
+
+    if (!system_bus_)
+      return;
+
+    ShutdownDBus();

[mlamouri (slow - plz ping), 2014/08/01 17:14:13]

nit: could you rename this ShutdownDBusConnection() or something clearer. The
name is slightly confusing.

[timvolodine, 2014/08/06 16:37:42]

Done.

+  }
+
+ private:
+
+  bool OnWatcherThread() {
+    return std::string(base::PlatformThread::GetName()).compare(
+        kBatteryWatcherThreadName) == 0;
+  }
+
+  void InitDBus() {
+    DCHECK(OnWatcherThread());
+
+    dbus::Bus::Options options;
+    options.bus_type = dbus::Bus::SYSTEM;
+    options.connection_type = dbus::Bus::PRIVATE;
+    system_bus_ = new dbus::Bus(options);
+  }
+
+  void ShutdownDBus() {
+    // Shutdown DBus connection later because there may be pending tasks on
+    // this thread.
+    message_loop()->PostTask(FROM_HERE,
+                             base::Bind(&dbus::Bus::ShutdownAndBlock,
+                                        system_bus_));
+    system_bus_ = 0;
+    battery_proxy_ = 0;
+  }
+
+  void OnSignalConnected(const std::string& interface_name,
+                         const std::string& signal_name,
+                         bool success) {
+    DCHECK(OnWatcherThread());
+
+    if (interface_name.compare(kUPowerDeviceName) != 0 ||
+        signal_name.compare(kUPowerDeviceSignalChanged) != 0) {
+      return;
+    }
+
+    if (!system_bus_)
+      return;
+
+    if (success) {
+      BatteryChanged(0);
+    } else {
+      // Failed to register for "Changed" signal, execute callback with the
+      // default values.
+      callback_.Run(blink::WebBatteryStatus());
+    }
+  }
+
+  void BatteryChanged(dbus::Signal* signal) {

[Michael van Ouwerkerk, 2014/08/01 15:24:39]

Maybe a unit test would be appropriate here. I think you could extract a static
method from BatteryChanged that is easy to test, something like:
blink::WebBatteryStatus GetStatus(base::DictionaryValue data);

[Michael van Ouwerkerk, 2014/08/01 15:24:39]

The signal param initially confused me. How about a comment along the lines of
/* unused as the only connected signal is for battery changes */

[timvolodine, 2014/08/06 16:37:42]

Done.

[timvolodine, 2014/08/06 16:37:42]

I've extracted a method for testing (ComputeWebBatteryStatus) and added
unittests. I had to add a header file as well in order to expose that method for
unit testing.

+    DCHECK(OnWatcherThread());
+
+    blink::WebBatteryStatus status;
+    scoped_ptr<base::DictionaryValue> dictionary =
+        GetPropertiesAsDictionary(battery_proxy_);
+
+    uint32 state = GetProperty<uint32>(*dictionary, "State", -1);
+    status.charging = state != 2; // true if not discharging

[mlamouri (slow - plz ping), 2014/08/01 17:14:14]

Could you have an enum that mirror the possible values of state
(http://upower.freedesktop.org/docs/Device.html#Device:State). It would make
things way easier to understand and help spot bugs. Here, it's hard to judge
whether you are doing the right thing.

[timvolodine, 2014/08/06 16:37:42]

Done.

+    double percentage = GetProperty<double>(*dictionary, "Percentage", 1);
+    // Convert percentage to a value between 0 and 1 with 3 digits of precision.
+    status.level = round(percentage * 10) / 1000.f;

[mlamouri (slow - plz ping), 2014/08/01 17:14:13]

Why not *0.01.f or /10.f?

[timvolodine, 2014/08/06 16:37:42]

dbus reports the value with lost of fractional digits, here the rounding to 3
significant digits is happening.

+
+    switch (state) {
+      case 1 : { // charging
+        int64 time_to_full = GetProperty<int64>(*dictionary, "TimeToFull", 0);
+        status.chargingTime =

[mlamouri (slow - plz ping), 2014/08/01 17:14:13]

Are we using the right default value in Blink if this is not set?

[timvolodine, 2014/08/06 16:37:42]

here we explicitly set it to either the time or +inf, because the default is 0.

+            (time_to_full > 0) ? time_to_full
+                               : std::numeric_limits<double>::infinity();
+        break;
+      }
+      case 2 : { // discharging
+        int64 time_to_empty = GetProperty<int64>(*dictionary, "TimeToEmpty", 0);
+        // Set dischargingTime if it's available. Otherwise leave the default
+        // value which is +infinity.
+        if (time_to_empty > 0)
+          status.dischargingTime = time_to_empty;

[mlamouri (slow - plz ping), 2014/08/01 17:14:13]

ditto

[timvolodine, 2014/08/06 16:37:43]

dischargingTime is +inf by default so we are fine here (as explained in the
comment)

+        status.chargingTime = std::numeric_limits<double>::infinity();
+        break;
+      }
+      case 4 : { // fully charged
+        break;
+      }
+      default: {
+        status.chargingTime = std::numeric_limits<double>::infinity();
+      }
+    }
+
+    callback_.Run(status);
+  }
+
+  BatteryStatusService::BatteryUpdateCallback callback_;
+  scoped_refptr<dbus::Bus> system_bus_;
+  dbus::ObjectProxy* battery_proxy_;  // owned by dbus
+
+  DISALLOW_COPY_AND_ASSIGN(BatteryStatusNotificationThread);
+};
+
+class BatteryStatusManagerLinux : public BatteryStatusManager {
+ public:
+  explicit BatteryStatusManagerLinux(
+      const BatteryStatusService::BatteryUpdateCallback& callback)
+      : callback_(callback) {}
+
+  virtual ~BatteryStatusManagerLinux() {}
+
+ private:
+  // BatteryStatusManager:
+  virtual bool StartListeningBatteryChange() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    if (!notifier_thread_) {
+      base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
+      notifier_thread_.reset(new BatteryStatusNotificationThread(callback_));
+      if (!notifier_thread_->StartWithOptions(thread_options)) {
+        notifier_thread_.reset();
+        return false;

[mlamouri (slow - plz ping), 2014/08/01 17:14:14]

That's not really something we expect to happen, right? Shouldn't we at least
LOG this? Maybe LOG(ERROR)?

[timvolodine, 2014/08/06 16:37:42]

right, added LOG(ERROR).

+      }
+    }
+
+    notifier_thread_->message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(&BatteryStatusNotificationThread::StartListening,
+                   base::Unretained(notifier_thread_.get())));
+    return true;
+  }
+
+  virtual void StopListeningBatteryChange() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+    notifier_thread_->message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(&BatteryStatusNotificationThread::StopListening,
+                   base::Unretained(notifier_thread_.get())));
+  }
+
+  BatteryStatusService::BatteryUpdateCallback callback_;
+  scoped_ptr<BatteryStatusNotificationThread> notifier_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatteryStatusManagerLinux);
+};
+
+}  // namespace
+
+// static
+scoped_ptr<BatteryStatusManager> BatteryStatusManager::Create(
+    const BatteryStatusService::BatteryUpdateCallback& callback) {
+  return scoped_ptr<BatteryStatusManager>(
+      new BatteryStatusManagerLinux(callback));
+}
+
+}  // namespace content