Do not call GetDisplayDevice for older UPower versions.

device/battery/battery_status_manager_linux.cc

Index: device/battery/battery_status_manager_linux.cc
diff --git a/device/battery/battery_status_manager_linux.cc b/device/battery/battery_status_manager_linux.cc
index 7d9469ec29789b2c257a8ac43fc8a0559a7962cf..0ec4398c86a93324710f4c27646ffd2b6157c9cb 100644
--- a/device/battery/battery_status_manager_linux.cc
+++ b/device/battery/battery_status_manager_linux.cc
@@ -13,6 +13,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/auto_reset.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
@@ -61,7 +62,7 @@ base::Version UPowerProperties::daemon_version() {
 
 class UPowerObject {
  public:
-  typedef dbus::PropertySet::PropertyChangedCallback PropertyChangedCallback;
+  using PropertyChangedCallback = dbus::PropertySet::PropertyChangedCallback;
 
   UPowerObject(dbus::Bus* dbus,
                const PropertyChangedCallback property_changed_callback);
@@ -225,7 +226,7 @@ uint32_t BatteryProperties::type(uint32_t default_value) {
 
 class BatteryObject {
  public:
-  typedef dbus::PropertySet::PropertyChangedCallback PropertyChangedCallback;
+  using PropertyChangedCallback = dbus::PropertySet::PropertyChangedCallback;
 
   BatteryObject(dbus::Bus* dbus,
                 const dbus::ObjectPath& device_path,
@@ -334,8 +335,8 @@ class BatteryStatusManagerLinux::BatteryStatusNotificationThread
     if (!system_bus_)
       InitDBus();
 
-    upower_.reset(new UPowerObject(system_bus_.get(),
-                                   UPowerObject::PropertyChangedCallback()));
+    upower_ = base::MakeUnique<UPowerObject>(
+        system_bus_.get(), UPowerObject::PropertyChangedCallback());
     upower_->proxy()->ConnectToSignal(
         kUPowerServiceName, kUPowerSignalDeviceAdded,
         base::Bind(&BatteryStatusNotificationThread::DeviceAdded,
@@ -397,14 +398,14 @@ class BatteryStatusManagerLinux::BatteryStatusNotificationThread
         [&current, this](const dbus::ObjectPath& device_path) {
           if (current && current->proxy()->object_path() == device_path)
             return std::move(current);
-          else
-            return CreateBattery(device_path);
+          return CreateBattery(device_path);
         };
 
-    dbus::ObjectPath display_device_path = upower_->GetDisplayDevice();
+    dbus::ObjectPath display_device_path;
+    if (!IsDaemonVersionBelow_0_99())
+      display_device_path = upower_->GetDisplayDevice();
     if (display_device_path.IsValid()) {
-      std::unique_ptr<BatteryObject> battery =
-          UseCurrentOrCreateBattery(display_device_path);
+      auto battery = UseCurrentOrCreateBattery(display_device_path);
       if (battery->IsValid())
         battery_ = std::move(battery);
     }
@@ -412,9 +413,7 @@ class BatteryStatusManagerLinux::BatteryStatusNotificationThread
     if (!battery_) {
       int num_batteries = 0;
       for (const auto& device_path : upower_->EnumerateDevices()) {
-        std::unique_ptr<BatteryObject> battery =
-            UseCurrentOrCreateBattery(device_path);
-
+        auto battery = UseCurrentOrCreateBattery(device_path);
         if (!battery->IsValid())
           continue;
 
@@ -433,14 +432,14 @@ class BatteryStatusManagerLinux::BatteryStatusNotificationThread
       UpdateNumberBatteriesHistogram(num_batteries);
     }
 
-    if (battery_) {
-      battery_->properties()->ConnectSignals();
-      NotifyBatteryStatus();
-    } else {
+    if (!battery_) {
       callback_.Run(BatteryStatus());
       return;
     }
 
+    battery_->properties()->ConnectSignals();
+    NotifyBatteryStatus();
+
     if (IsDaemonVersionBelow_0_99()) {
       // UPower Version 0.99 replaced the Changed signal with the
       // PropertyChanged signal. For older versions we need to listen
@@ -476,11 +475,10 @@ class BatteryStatusManagerLinux::BatteryStatusNotificationThread
 
   std::unique_ptr<BatteryObject> CreateBattery(
       const dbus::ObjectPath& device_path) {
-    std::unique_ptr<BatteryObject> battery(new BatteryObject(
+    return base::MakeUnique<BatteryObject>(
         system_bus_.get(), device_path,
         base::Bind(&BatteryStatusNotificationThread::BatteryPropertyChanged,
-                   base::Unretained(this))));
-    return battery;
+                   base::Unretained(this)));
   }
 
   void DeviceAdded(dbus::Signal* signal /* unused */) {
@@ -545,9 +543,9 @@ class BatteryStatusManagerLinux::BatteryStatusNotificationThread
     // the new property-value triggers a callback to BatteryPropertyChanged().
     // notifying_battery_status_ is set to avoid recursion and computing the
     // status too often.
-    notifying_battery_status_ = true;
+    base::AutoReset<bool> notifying_battery_status(&notifying_battery_status_,
+                                                   true);

[timvolodine, 2017/04/10 12:50:58]

nit: not sure if this is actually cleaner than the original implementation. The
AutoReset eventually resets the variable to the original value, which is
technically different from the previous implementation. Luckily the
notifying_battery_status_ cannot be true at this point as it would have returned
early due to preceding if-check, so in this case this is equivalent (but needed
checking that this is the case). I think I would prefer the original version
though..

[Lei Zhang, 2017/04/10 18:14:47]

You are right. It could have potentially started out as true. Reverted.

     callback_.Run(ComputeWebBatteryStatus(battery_->properties()));
-    notifying_battery_status_ = false;
   }
 
   BatteryStatusService::BatteryUpdateCallback callback_;
@@ -589,13 +587,14 @@ bool BatteryStatusManagerLinux::StartNotifierThreadIfNecessary() {
     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();
+  auto notifier_thread =
+      base::MakeUnique<BatteryStatusNotificationThread>(callback_);
+  if (!notifier_thread->StartWithOptions(thread_options)) {
     LOG(ERROR) << "Could not start the " << kBatteryNotifierThreadName
                << " thread";
     return false;
   }
+  notifier_thread_ = std::move(notifier_thread);
   return true;
 }
 
@@ -608,20 +607,17 @@ std::unique_ptr<BatteryStatusManagerLinux>
 BatteryStatusManagerLinux::CreateForTesting(
     const BatteryStatusService::BatteryUpdateCallback& callback,
     dbus::Bus* bus) {
-  std::unique_ptr<BatteryStatusManagerLinux> manager(
-      new BatteryStatusManagerLinux(callback));
-  if (manager->StartNotifierThreadIfNecessary())
-    manager->notifier_thread_->SetDBusForTesting(bus);
-  else
-    manager.reset();
+  auto manager = base::MakeUnique<BatteryStatusManagerLinux>(callback);
+  if (!manager->StartNotifierThreadIfNecessary())
+    return nullptr;
+  manager->notifier_thread_->SetDBusForTesting(bus);
   return manager;
 }
 
 // static
 std::unique_ptr<BatteryStatusManager> BatteryStatusManager::Create(
     const BatteryStatusService::BatteryUpdateCallback& callback) {
-  return std::unique_ptr<BatteryStatusManager>(
-      new BatteryStatusManagerLinux(callback));
+  return base::MakeUnique<BatteryStatusManagerLinux>(callback);
 }
 
 }  // namespace device