Do not call GetDisplayDevice for older UPower versions.

device/battery/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 "device/battery/battery_status_manager_linux.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <limits>
 #include <memory>
 #include <string>
 #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"
 #include "base/threading/thread.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
@@ skipping 28 matching lines @@
 UPowerProperties::~UPowerProperties() {}
 
 base::Version UPowerProperties::daemon_version() {
   return (daemon_version_.is_valid() || daemon_version_.GetAndBlock())
              ? base::Version(daemon_version_.value())
              : base::Version();
 }
 
 class UPowerObject {
  public:
-  typedef dbus::PropertySet::PropertyChangedCallback PropertyChangedCallback;
+  using PropertyChangedCallback = dbus::PropertySet::PropertyChangedCallback;
 
   UPowerObject(dbus::Bus* dbus,
                const PropertyChangedCallback property_changed_callback);
   ~UPowerObject();
 
   std::vector<dbus::ObjectPath> EnumerateDevices();
   dbus::ObjectPath GetDisplayDevice();
 
   dbus::ObjectProxy* proxy() { return proxy_; }
   UPowerProperties* properties() { return properties_.get(); }
@@ skipping 143 matching lines @@
              : default_value;
 }
 
 uint32_t BatteryProperties::type(uint32_t default_value) {
   return (type_.is_valid() || type_.GetAndBlock()) ? type_.value()
                                                    : default_value;
 }
 
 class BatteryObject {
  public:
-  typedef dbus::PropertySet::PropertyChangedCallback PropertyChangedCallback;
+  using PropertyChangedCallback = dbus::PropertySet::PropertyChangedCallback;
 
   BatteryObject(dbus::Bus* dbus,
                 const dbus::ObjectPath& device_path,
                 const PropertyChangedCallback& property_changed_callback);
   ~BatteryObject();
 
   bool IsValid();
 
   dbus::ObjectProxy* proxy() { return proxy_; }
   BatteryProperties* properties() { return properties_.get(); }
@@ skipping 88 matching lines @@
 
   void StartListening() {
     DCHECK(OnWatcherThread());
 
     if (upower_)
       return;
 
     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,
                    base::Unretained(this)),
         base::Bind(&BatteryStatusNotificationThread::OnSignalConnected,
                    base::Unretained(this)));
     upower_->proxy()->ConnectToSignal(
         kUPowerServiceName, kUPowerSignalDeviceRemoved,
         base::Bind(&BatteryStatusNotificationThread::DeviceRemoved,
                    base::Unretained(this)),
@@ skipping 41 matching lines @@
     // - Or it may be a new device; then the previously monitored BatteryObject
     //   instance (if any) is released on leaving this function.
     // - Or we may not find a battery device; then on leaving this function
     //   battery_ will be nullptr and the previously monitored BatteryObject
     //   instance (if any) is no longer a battery and will be released.
     std::unique_ptr<BatteryObject> current = std::move(battery_);
     auto UseCurrentOrCreateBattery =
         [&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);
     }
 
     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;
 
         if (battery_) {
           // TODO(timvolodine): add support for multiple batteries. Currently we
           // only collect information from the first battery we encounter
           // (crbug.com/400780).
           LOG(WARNING) << "multiple batteries found, "
                        << "using status data of the first battery only.";
         } else {
           battery_ = std::move(battery);
         }
         num_batteries++;
       }
 
       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
       // to the Changed signal.
       battery_->proxy()->ConnectToSignal(
           kUPowerDeviceInterfaceName, kUPowerDeviceSignalChanged,
           base::Bind(&BatteryStatusNotificationThread::BatteryChanged,
                      base::Unretained(this)),
           base::Bind(&BatteryStatusNotificationThread::OnSignalConnected,
                      base::Unretained(this)));
@@ skipping 15 matching lines @@
         FROM_HERE, base::Bind(&dbus::Bus::ShutdownAndBlock, system_bus_));
     system_bus_ = NULL;
   }
 
   void OnSignalConnected(const std::string& interface_name,
                          const std::string& signal_name,
                          bool success) {}
 
   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 */) {
     // Re-iterate all devices to see if we need to monitor the added battery
     // instead of the currently monitored battery.
     FindBatteryDevice();
   }
 
   void DeviceRemoved(dbus::Signal* signal) {
     if (!battery_)
@@ skipping 44 matching lines @@
       return;
 
     // If the system uses a UPower daemon older than version 0.99
     // (see IsDaemonVersionBelow_0_99), then we are notified about changed
     // battery_ properties through the 'Changed' signal of the battery_
     // device (see BatteryChanged()). That is implemented to invalidate all
     // battery_ properties (so they are re-fetched from the dbus). Getting
     // 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_;
   scoped_refptr<dbus::Bus> system_bus_;
   std::unique_ptr<UPowerObject> upower_;
   std::unique_ptr<BatteryObject> battery_;
   bool notifying_battery_status_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(BatteryStatusNotificationThread);
 };
@@ skipping 21 matching lines @@
   notifier_thread_->task_runner()->PostTask(
       FROM_HERE, base::Bind(&BatteryStatusNotificationThread::StopListening,
                             base::Unretained(notifier_thread_.get())));
 }
 
 bool BatteryStatusManagerLinux::StartNotifierThreadIfNecessary() {
   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();
+  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;
 }
 
 base::Thread* BatteryStatusManagerLinux::GetNotifierThreadForTesting() {
   return notifier_thread_.get();
 }
 
 // static
 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