Battery Status API: implementation for Linux.

content/browser/battery_status/battery_status_manager_linux.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_linux.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"
+
+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";
+const char kBatteryWatcherThreadName[] = "BatteryStatusWatcher";
+const char kDBusPropertiesInterface[] = "org.freedesktop.DBus.Properties";
+const char kDBusPropertiesGetAll[] = "GetAll";
+
+// UPowerDeviceType reflects the possible UPower.Device.Type values,
+// see upower.freedesktop.org/docs/Device.html#Device:Type.
+enum UPowerDeviceType {
+  UPOWER_DEVICE_TYPE_UNKNOWN = 0,
+  UPOWER_DEVICE_TYPE_LINE_POWER = 1,
+  UPOWER_DEVICE_TYPE_BATTERY = 2,
+  UPOWER_DEVICE_TYPE_UPS = 3,
+  UPOWER_DEVICE_TYPE_MONITOR = 4,
+  UPOWER_DEVICE_TYPE_MOUSE = 5,
+  UPOWER_DEVICE_TYPE_KEYBOARD = 6,
+  UPOWER_DEVICE_TYPE_PDA = 7,
+  UPOWER_DEVICE_TYPE_PHONE = 8,
+};
+
+// intended for uint32, int64, uint64, double.

[hashimoto, 2014/08/11 08:19:15]

I see no uint64 in this file.

[timvolodine, 2014/08/11 16:03:00]

Acknowledged.

+template<typename T>
+T GetProperty(const base::DictionaryValue& dictionary,

[hashimoto, 2014/08/11 08:19:15]

I don't see any merit of having this template.
It even makes it hard to locate a potential bug where precision is lost because
int64 is converted to double by PopDataAsValue().
Please use GetDouble() and GetBoolean() directly in the caller sites.

[timvolodine, 2014/08/11 16:02:59]

Done.

+              const std::string& property_name,
+              T default_value) {
+  double value;

[hashimoto, 2014/08/11 08:19:14]

Please don't leave a variable uninitialized.

[timvolodine, 2014/08/11 16:02:58]

Done.

+  return dictionary.GetDouble(property_name, &value) ? static_cast<T>(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(kDBusPropertiesInterface,
+                               kDBusPropertiesGetAll);

[hashimoto, 2014/08/11 08:19:15]

Why don't you use dbus/property.h?

[timvolodine, 2014/08/11 16:02:59]

Done.

[hashimoto, 2014/08/12 10:01:42]

Sorry for not being clear,
why don't you use dbus::PropertySet defined in property.h?

[timvolodine, 2014/08/12 19:27:53]

I'll look into that, not sure if this will simplify current code though.

[hashimoto, 2014/08/14 04:17:04]

ping?

+  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());

[hashimoto, 2014/08/11 08:19:14]

Why not returning a null scoped_ptr?

[timvolodine, 2014/08/11 16:02:59]

upon failure it returns an empty dictionary which will later correspond to
default values provided by the API (i.e. no information can be provided). That
way we avoid extra null checks.

[hashimoto, 2014/08/13 11:02:20]

Why should we perform totally redandunt allocation to save one NULL check?

[timvolodine, 2014/08/13 16:58:08]

ok made it to return null. Done.
it's actually more that 1 null check but still ok.

+}
+
+void GetPowerSourcesPaths(dbus::ObjectProxy* proxy,
+                          std::vector<dbus::ObjectPath>& paths) {

[hashimoto, 2014/08/11 08:19:15]

Please don't use non-const references.

[timvolodine, 2014/08/11 16:02:59]

Done.

+  if (!proxy)
+    return;
+
+  dbus::MethodCall method_call(kUPowerServiceName, "EnumerateDevices");

[hashimoto, 2014/08/11 08:19:15]

"EnumerateDevices" should be a constant.

[timvolodine, 2014/08/11 16:03:00]

Done.

+  scoped_ptr<dbus::Response> response(proxy->CallMethodAndBlock(&method_call,
+      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));

[hashimoto, 2014/08/11 08:19:14]

nit: Arguments should be aligned.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[timvolodine, 2014/08/11 16:03:00]

Done.

+
+  if (response) {
+    dbus::MessageReader reader(response.get());
+    if (reader.PopArrayOfObjectPaths(&paths))
+      return;
+  }
+  paths.clear();

[hashimoto, 2014/08/11 08:19:15]

This line is not needed.

[timvolodine, 2014/08/11 16:02:59]

Done.

+}
+

[hashimoto, 2014/08/11 08:19:14]

Please add comments for each class which describe the roles of the classes.

[timvolodine, 2014/08/11 16:03:00]

Done.

+class BatteryStatusNotificationThread : public base::Thread {

[hashimoto, 2014/08/11 08:19:15]

No need to inherit base::Thread.

[timvolodine, 2014/08/11 16:02:59]

? what do you mean, it's a thread.

[hashimoto, 2014/08/12 10:01:42]

Composition is preferred to inheritance.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
You can do the same thing by having base::Thread as a member.

[timvolodine, 2014/08/12 19:27:53]

oh I see, in this case I don't think it'll make things much better as I would
need to expose some start method in this class which then would be called from a
different thread..

[hashimoto, 2014/08/13 11:02:20]

Please don't violate the coding style and degrade the readability of the code to
save a few key strokes.
Also, you can pass a pointer to a SingleThreadTaskRunner (or appropriate
subclass of TaskRunner) to this class while owning base::Thread in the manager.

+ public:
+  BatteryStatusNotificationThread(
+      const BatteryStatusService::BatteryUpdateCallback& callback)
+      : base::Thread(kBatteryWatcherThreadName),
+        callback_(callback),
+        battery_proxy_(0) {}

[hashimoto, 2014/08/11 08:19:14]

Use NULL.

[timvolodine, 2014/08/11 16:02:59]

Done.

+
+  virtual ~BatteryStatusNotificationThread() {
+    if (system_bus_)
+      ShutdownDBusConnection();
+    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);

[hashimoto, 2014/08/11 08:19:14]

ObjectProxy won't be destroyed until the bus gets destroyed or RemoveObjectProxy
is called.
It might be better to call RemoveObjectProxy() for unused proxies if
device_paths.size() can be a large number.

[timvolodine, 2014/08/11 16:02:59]

Done.

+      scoped_ptr<base::DictionaryValue> dictionary =
+          GetPropertiesAsDictionary(device_proxy);
+
+      bool is_present = GetProperty<bool>(*dictionary, "IsPresent", false);
+      uint32 type = GetProperty<uint32>(*dictionary, "Type",
+                                        UPOWER_DEVICE_TYPE_UNKNOWN);
+
+      if (!is_present || type != UPOWER_DEVICE_TYPE_BATTERY)
+        continue;
+
+      if (battery_proxy_) {
+        // TODO(timvolodine): add support for multiple batteries. Currently we
+        // only collect information from the first battery we encounter
+        // (crbug.com/400780).
+        // TODO(timvolodine): add UMA logging for this case.
+        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;
+
+    ShutdownDBusConnection();
+  }
+
+ private:
+

[hashimoto, 2014/08/11 08:19:14]

nit: No need to have a blank line.

[timvolodine, 2014/08/11 16:03:00]

Done.

+  bool OnWatcherThread() {
+    return std::string(base::PlatformThread::GetName()).compare(

[hashimoto, 2014/08/11 08:19:15]

Use operator==.

[timvolodine, 2014/08/11 16:02:59]

Acknowledged.

+        kBatteryWatcherThreadName) == 0;

[hashimoto, 2014/08/11 08:19:14]

Why don't you directly compare the name of the thread?

[timvolodine, 2014/08/11 16:03:00]

PlatformThread::GetName() == kBatteryWatcherThreadName doesn't work. apparently
the names are interned by thread_id_name_manager.cc. switched to strcomp now.

[hashimoto, 2014/08/12 10:01:42]

Sorry, not then name, the ID.
Can't we do the same thing as MessageLoopProxyImpl::RunsTasksOnCurrentThread()?
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...

Or, how about calling task_runner()->RunsTasksOnCurrentThread()?

[timvolodine, 2014/08/12 19:27:53]

yes, RunsTasksOnCurrentThread appears to work. Done.

+  }
+
+  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 ShutdownDBusConnection() {
+    // Shutdown DBus connection later because there may be pending tasks on
+    // this thread.
+    message_loop()->PostTask(FROM_HERE,
+                             base::Bind(&dbus::Bus::ShutdownAndBlock,

[hashimoto, 2014/08/11 08:19:15]

Doesn't this result in a case where BatteryChanged() is called after this object
gets destryoed?

[timvolodine, 2014/08/11 16:02:59]

I've added a check in BatteryChanged to prevent the case when BatteryChanged is
called after StopListening has been called.

[hashimoto, 2014/08/12 10:01:42]

I'm not sure if that |system_bus_| check can be helpful.
It should result in a crash with SIGSEGV accessing any member of this object
after the dtor gets called.

[timvolodine, 2014/08/12 19:27:53]

I am not sure I understand your concern. The BatteryStatusNotificationThread
currently gets destroyed only on shutdown. At destruction time it makes sure to
call ShutdownDBusConnection() and then calls Stop() method of the Thread class
which makes sure that the message loop is drained and the bus is properly
disconnected. I don't see how we can get any callbacks after the
BatteryStatusNotificationThread is destroyed.

[hashimoto, 2014/08/13 11:02:20]

I was concerned about a case where this object is destroyed but the bus is still
alive.
But since you changed the dtor and ShutdownDBusConnection(), it seems it cannot
happen. (sorry for dismissing this)

[timvolodine, 2014/08/13 16:58:08]

Done.

+                                        system_bus_));
+    system_bus_ = 0;

[hashimoto, 2014/08/11 08:19:15]

Use NULL for pointers.

[timvolodine, 2014/08/11 16:03:00]

Done.

+    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) {

[hashimoto, 2014/08/11 08:19:15]

Use operator!=

[timvolodine, 2014/08/11 16:02:59]

Done.

+      return;
+    }
+
+    if (!system_bus_)
+      return;
+
+    if (success) {
+      BatteryChanged(0);

[hashimoto, 2014/08/11 08:19:15]

Use NULL for pointers.

[timvolodine, 2014/08/11 16:02:59]

Done.

+    } else {
+      // Failed to register for "Changed" signal, execute callback with the
+      // default values.

[hashimoto, 2014/08/11 08:19:15]

Why do we need to do this?

[timvolodine, 2014/08/11 16:03:00]

this means we cannot get battery updates, so need to propagate default values to
blink (i.e. meaning we cannot provide battery information according to
specification)

[hashimoto, 2014/08/12 10:01:42]

It seems the Chrome OS implmentation is not doing the same thing when
PowerManager fails to connect to the signal.
Is this really needed?

[timvolodine, 2014/08/12 19:27:53]

I believe this is needed to cover the following (unlikely) situation: imagine we
can get current battery status, but fail to connect to the signal. We have two
options:
1. either execute callback with the current status and do nothing, or
2. execute the callback with default values (meaning cannot provide data
according to spec).

I think option 2 is better as option 1 would result in stale data after some
time which would be misleading. ChromeOS probably follows option 1 and we should
see if this can be fixed.

[hashimoto, 2014/08/13 11:02:20]

When something goes wrong with D-Bus, it seems the current Chrome OS
implementation does nothing because PowerChanged() never gets called in error
cases.
Also, it's more natural to do nothing than running the observer callback with
the default value reserved to indicate an error.

[timvolodine, 2014/08/13 16:58:08]

I think ChromeOS actually forces one callback on start:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba...

The specification says that we should provide default values if they cannot be
provided by the system. There is no error case, but we need to send something
otherwise the promise in blink will never resolve. Also when we cannot update
battery information the values will be stale after some time.

[hashimoto, 2014/08/14 04:17:04]

That RequestStatusUpdate() results in nothing if an error happens.

[timvolodine, 2014/08/14 22:02:45]

What I meant is that callback SignalConnected() is ignored in
power_manager_client.cc (it simply logs a warning if it fails). So I was
referring to the (hypothetical) case where the SignalConnected fails but the
properties can be read from dbus in which case RequestStatusUpdate will
propagate to observers.
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/powe...

[hashimoto, 2014/08/15 02:07:00]

You shouldn't expect the service to return responses for method calls when
SignalConnected() fails.

+      callback_.Run(blink::WebBatteryStatus());
+    }
+  }
+
+  void BatteryChanged(dbus::Signal* signal /* unsused */) {
+    DCHECK(OnWatcherThread());
+
+    blink::WebBatteryStatus status;
+    scoped_ptr<base::DictionaryValue> dictionary =
+        GetPropertiesAsDictionary(battery_proxy_);
+    ComputeWebBatteryStatus(*dictionary, status);
+
+    callback_.Run(status);
+  }
+
+  BatteryStatusService::BatteryUpdateCallback callback_;
+  scoped_refptr<dbus::Bus> system_bus_;
+  dbus::ObjectProxy* battery_proxy_;  // owned by dbus

[hashimoto, 2014/08/11 08:19:14]

"owned by dbus" is ambiguos.
It's owned by the bus.

[timvolodine, 2014/08/11 16:03:00]

Done.

+
+  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 (!NotifierThreadStarted())
+      return false;
+
+    notifier_thread_->message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(&BatteryStatusNotificationThread::StartListening,
+                   base::Unretained(notifier_thread_.get())));

[hashimoto, 2014/08/11 08:19:14]

Doesn't this result in a crash if StartListening() is run after the notifier
thread is destryoed?

[timvolodine, 2014/08/11 16:03:00]

this cannot happen, currently the thread is destroyed during shutdown.

[hashimoto, 2014/08/12 10:01:42]

Are you sure that StartListening() and StopListening() tasks are not run after
notifier_thread_ gets destroyed considering all possible scenarios?

[timvolodine, 2014/08/12 19:27:53]

notifier_thead_ currently get destroyed at shutdown by the chrome main thread
after the IO thread has been stopped already. {Start,Stop}Listening() come in on
IO thread so not possible to receive them after notifier_thread_ destruction.

+    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())));
+  }
+
+  bool NotifierThreadStarted() {

[hashimoto, 2014/08/11 08:19:15]

"NotifierThreadStarted" sounds like a getter which returns the thread's status,
or a handler which handles the thread initialization errors while what it
actually does is to start the thread.
Please give it a more descriptive name.

[timvolodine, 2014/08/11 16:03:00]

Done.

+    if (notifier_thread_)
+      return true;
+
+    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();
+      LOG(ERROR) << "Could not start the " << kBatteryWatcherThreadName
+                 << " thread";
+      return false;
+    }
+    return true;
+  }
+
+  BatteryStatusService::BatteryUpdateCallback callback_;
+  scoped_ptr<BatteryStatusNotificationThread> notifier_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatteryStatusManagerLinux);
+};
+
+}  // namespace
+
+void ComputeWebBatteryStatus(const base::DictionaryValue& dictionary,
+                             blink::WebBatteryStatus& status) {
+  if (!dictionary.HasKey("State"))
+    return;
+  uint32 state = GetProperty<uint32>(dictionary, "State",
+                                     UPOWER_DEVICE_STATE_UNKNOWN);
+  status.charging = state != UPOWER_DEVICE_STATE_DISCHARGING &&
+                    state != UPOWER_DEVICE_STATE_EMPTY;
+  double percentage = GetProperty<double>(dictionary, "Percentage", 100);
+  // Convert percentage to a value between 0 and 1 with 3 digits of precision.
+  status.level = round(percentage * 10) / 1000.f;

[hashimoto, 2014/08/11 08:19:14]

Why should we intentionally degrade precision?

[timvolodine, 2014/08/11 16:02:59]

this is to avoid fingerprinting and not to trigger level changes in blink more
often than needed.

[hashimoto, 2014/08/12 10:01:42]

I'm not sure what "fingerprinting" means in this context.
Could you add a comment which describes why this is needed and why it should be
3 digits of precision (not 2, 4, 5 or whatever)?

Also, it seems existing implementations for Mac and Chrome OS are not doing
this.
Don't we need this for those platforms too?
If needed, how about moving this precision degrade code to the caller site (e.g.
Blink code)?

[timvolodine, 2014/08/12 19:27:52]

Mac and Android do not need this rounding, don't know about chrome OS.
Interesting suggestion about moving to blink, added some comments + TODO for
this.

[hashimoto, 2014/08/13 11:02:20]

Chrome OS is much more important than Linux.
Please implement the same logic or resolve the TODO in short term if we should
really care about this problem.

[timvolodine, 2014/08/13 16:58:08]

I'll check with the chromeOS guys on this.

+
+  switch (state) {
+    case UPOWER_DEVICE_STATE_CHARGING : {
+      int64 time_to_full = GetProperty<int64>(dictionary, "TimeToFull", 0);

[hashimoto, 2014/08/11 08:19:15]

Why do we need to convert a double value stored in the dictionary to int64 and
later convert it again into double?

[timvolodine, 2014/08/11 16:02:59]

Done.

+      status.chargingTime =
+          (time_to_full > 0) ? time_to_full
+                             : std::numeric_limits<double>::infinity();
+      break;
+    }
+    case UPOWER_DEVICE_STATE_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;
+      status.chargingTime = std::numeric_limits<double>::infinity();
+      break;
+    }
+    case UPOWER_DEVICE_STATE_FULL : {
+      break;
+    }
+    default: {
+      status.chargingTime = std::numeric_limits<double>::infinity();
+    }
+  }
+}
+
+// static
+scoped_ptr<BatteryStatusManager> BatteryStatusManager::Create(
+    const BatteryStatusService::BatteryUpdateCallback& callback) {
+  return scoped_ptr<BatteryStatusManager>(
+      new BatteryStatusManagerLinux(callback));
+}
+
+}  // namespace content