chromeos: Clean up power status polling code.

This makes Chrome decode PowerSupplyProperties protocol
buffers from PowerSupplyPoll D-Bus signals sent by powerd.
It also removes an unnecessary UpdateRequestType enum from
PowerManagerClient.

BUG=245108,234671

Review URL: https://chromiumcodereview.appspot.com/16493002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204614 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 86 insertions, 90 deletions

diff --git a/chromeos/dbus/fake_power_manager_client.cc b/chromeos/dbus/fake_power_manager_client.cc
index 9240eb4d..df1ef54 100644
--- a/chromeos/dbus/fake_power_manager_client.cc
+++ b/chromeos/dbus/fake_power_manager_client.cc
@@ -17,8 +17,7 @@ FakePowerManagerClient::~FakePowerManagerClient() {
 void FakePowerManagerClient::AddObserver(Observer* observer) {
 }
 
-void FakePowerManagerClient::RequestStatusUpdate(
-    UpdateRequestType update_type) {
+void FakePowerManagerClient::RequestStatusUpdate() {
 }
 
 void FakePowerManagerClient::SetPolicy(
diff --git a/chromeos/dbus/fake_power_manager_client.h b/chromeos/dbus/fake_power_manager_client.h
index c08a4c7..89d2b6e 100644
--- a/chromeos/dbus/fake_power_manager_client.h
+++ b/chromeos/dbus/fake_power_manager_client.h
@@ -32,7 +32,7 @@ class FakePowerManagerClient : public PowerManagerClient {
       const GetScreenBrightnessPercentCallback& callback) OVERRIDE;
   virtual void DecreaseKeyboardBrightness() OVERRIDE;
   virtual void IncreaseKeyboardBrightness() OVERRIDE;
-  virtual void RequestStatusUpdate(UpdateRequestType update_type) OVERRIDE;
+  virtual void RequestStatusUpdate() OVERRIDE;
   virtual void RequestRestart() OVERRIDE;
   virtual void RequestShutdown() OVERRIDE;
   virtual void RequestIdleNotification(int64 threshold_secs) OVERRIDE;
diff --git a/chromeos/dbus/power_manager_client.cc b/chromeos/dbus/power_manager_client.cc
index f8c2220..1d3f2e3 100644
--- a/chromeos/dbus/power_manager_client.cc
+++ b/chromeos/dbus/power_manager_client.cc
@@ -216,7 +216,7 @@ class PowerManagerClientImpl : public PowerManagerClient {
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  virtual void RequestStatusUpdate(UpdateRequestType update_type) OVERRIDE {
+  virtual void RequestStatusUpdate() OVERRIDE {
     dbus::MethodCall method_call(
         power_manager::kPowerManagerInterface,
         power_manager::kGetPowerSupplyPropertiesMethod);
@@ -378,9 +378,16 @@ class PowerManagerClientImpl : public PowerManagerClient {
                       PeripheralBatteryStatusReceived(path, name, level));
   }
 
-  void PowerSupplyPollReceived(dbus::Signal* unused_signal) {
+  void PowerSupplyPollReceived(dbus::Signal* signal) {
     VLOG(1) << "Received power supply poll signal.";
-    RequestStatusUpdate(UPDATE_POLL);
+    dbus::MessageReader reader(signal);
+    power_manager::PowerSupplyProperties protobuf;
+    if (reader.PopArrayOfBytesAsProto(&protobuf)) {
+      HandlePowerSupplyProperties(protobuf);
+    } else {
+      LOG(ERROR) << "Unable to decode "
+                 << power_manager::kPowerSupplyPollSignal << "signal";
+    }
   }
 
   void OnGetPowerSupplyPropertiesMethod(dbus::Response* response) {
@@ -392,56 +399,13 @@ class PowerManagerClientImpl : public PowerManagerClient {
 
     dbus::MessageReader reader(response);
     power_manager::PowerSupplyProperties protobuf;
-    if (!reader.PopArrayOfBytesAsProto(&protobuf)) {
-      LOG(ERROR) << "Error calling "
+    if (reader.PopArrayOfBytesAsProto(&protobuf)) {
+      HandlePowerSupplyProperties(protobuf);
+    } else {
+      LOG(ERROR) << "Unable to decode "
                  << power_manager::kGetPowerSupplyPropertiesMethod
-                 << response->ToString();
-      return;
+                 << " response";
     }
-
-    // TODO(derat): Update PowerSupplyStatus to keep the ExternalPower and
-    // BatteryState information from PowerSupplyProperties intact.
-    PowerSupplyStatus status;
-    status.line_power_on = protobuf.external_power() !=
-        power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED;
-    status.battery_is_present = protobuf.battery_state() !=
-        power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
-    status.battery_is_full = protobuf.battery_state() ==
-        power_manager::PowerSupplyProperties_BatteryState_FULL;
-    status.battery_seconds_to_empty = protobuf.battery_time_to_empty_sec();
-    status.battery_seconds_to_full = protobuf.battery_time_to_full_sec();
-    status.battery_percentage = protobuf.battery_percent();
-    status.is_calculating_battery_time = protobuf.is_calculating_battery_time();
-
-    switch (protobuf.external_power()) {
-      case power_manager::PowerSupplyProperties_ExternalPower_AC:
-        status.battery_state = PowerSupplyStatus::CHARGING;
-        break;
-      case power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED:
-        status.battery_state = PowerSupplyStatus::DISCHARGING;
-        break;
-      case power_manager::PowerSupplyProperties_ExternalPower_USB:
-        status.battery_state = PowerSupplyStatus::CONNECTED_TO_USB;
-        break;
-      default:
-        NOTREACHED() << "Unhandled external power state "
-                     << protobuf.external_power();
-    }
-
-    // Check power status values are consistent
-    if (!status.is_calculating_battery_time) {
-      int64 battery_seconds_to_goal = status.line_power_on ?
-          status.battery_seconds_to_full : status.battery_seconds_to_empty;
-      if (battery_seconds_to_goal < 0) {
-        LOG(ERROR) << "Received power supply status with negative seconds to "
-            << (status.line_power_on ? "full" : "empty")
-            << ". Assume time is still being calculated.";
-        status.is_calculating_battery_time = true;
-      }
-    }
-
-    VLOG(1) << "Power status: " << status.ToString();
-    FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(status));
   }
 
   void OnGetScreenBrightnessPercent(
@@ -663,6 +627,54 @@ class PowerManagerClientImpl : public PowerManagerClient {
         dbus::ObjectProxy::EmptyResponseCallback());
   }
 
+  // Handles a PowerSupplyProperties protocol buffer from the power manager.
+  void HandlePowerSupplyProperties(
+      const power_manager::PowerSupplyProperties& protobuf) {
+    // TODO(derat): Kill PowerSupplyStatus and just use the
+    // PowerSupplyProperties protobuf within Ash.
+    PowerSupplyStatus status;
+    status.line_power_on = protobuf.external_power() !=
+        power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED;
+    status.battery_is_present = protobuf.battery_state() !=
+        power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT;
+    status.battery_is_full = protobuf.battery_state() ==
+        power_manager::PowerSupplyProperties_BatteryState_FULL;
+    status.battery_seconds_to_empty = protobuf.battery_time_to_empty_sec();
+    status.battery_seconds_to_full = protobuf.battery_time_to_full_sec();
+    status.battery_percentage = protobuf.battery_percent();
+    status.is_calculating_battery_time = protobuf.is_calculating_battery_time();
+
+    switch (protobuf.external_power()) {
+      case power_manager::PowerSupplyProperties_ExternalPower_AC:
+        status.battery_state = PowerSupplyStatus::CHARGING;
+        break;
+      case power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED:
+        status.battery_state = PowerSupplyStatus::DISCHARGING;
+        break;
+      case power_manager::PowerSupplyProperties_ExternalPower_USB:
+        status.battery_state = PowerSupplyStatus::CONNECTED_TO_USB;
+        break;
+      default:
+        NOTREACHED() << "Unhandled external power state "
+                     << protobuf.external_power();
+    }
+
+    // Check power status values are consistent
+    if (!status.is_calculating_battery_time) {
+      int64 battery_seconds_to_goal = status.line_power_on ?
+          status.battery_seconds_to_full : status.battery_seconds_to_empty;
+      if (battery_seconds_to_goal < 0) {
+        LOG(ERROR) << "Received power supply status with negative seconds to "
+            << (status.line_power_on ? "full" : "empty")
+            << ". Assume time is still being calculated.";
+        status.is_calculating_battery_time = true;
+      }
+    }
+
+    VLOG(1) << "Power status: " << status.ToString();
+    FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(status));
+  }
+
   // Origin thread (i.e. the UI thread in production).
   base::PlatformThreadId origin_thread_id_;
 
@@ -707,6 +719,10 @@ class PowerManagerClientStubImpl : public PowerManagerClient {
         brightness_(50.0),
         pause_count_(2),
         cycle_count_(0) {
+    const int kStatusUpdateMs = 1000;
+    update_timer_.Start(FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kStatusUpdateMs), this,
+        &PowerManagerClientStubImpl::UpdateStatus);
   }
 
   virtual ~PowerManagerClientStubImpl() {}
@@ -755,21 +771,10 @@ class PowerManagerClientStubImpl : public PowerManagerClient {
     VLOG(1) << "Requested to increase keyboard brightness";
   }
 
-  virtual void RequestStatusUpdate(UpdateRequestType update_type) OVERRIDE {
-    if (update_type == UPDATE_INITIAL) {
-      Update();
-      return;
-    }
-    if (!timer_.IsRunning() && update_type == UPDATE_USER) {
-      Update();
-      timer_.Start(
-          FROM_HERE,
-          base::TimeDelta::FromMilliseconds(1000),
-          this,
-          &PowerManagerClientStubImpl::Update);
-    } else {
-      timer_.Stop();
-    }
+  virtual void RequestStatusUpdate() OVERRIDE {
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+        base::Bind(&PowerManagerClientStubImpl::UpdateStatus,
+                   base::Unretained(this)));
   }
 
   virtual void RequestRestart() OVERRIDE {}
@@ -796,7 +801,7 @@ class PowerManagerClientStubImpl : public PowerManagerClient {
   }
 
  private:
-  void Update() {
+  void UpdateStatus() {
     if (pause_count_ > 0) {
       pause_count_--;
     } else {
@@ -821,9 +826,7 @@ class PowerManagerClientStubImpl : public PowerManagerClient {
       status_.battery_state = discharging_ ?
           PowerSupplyStatus::DISCHARGING : PowerSupplyStatus::CHARGING;
     } else {
-      // Simulating a non-standard weak power supply.
-      status_.battery_state =
-          PowerSupplyStatus::NEITHER_CHARGING_NOR_DISCHARGING;
+      status_.battery_state = PowerSupplyStatus::CONNECTED_TO_USB;
     }
 
     int64 remaining_battery_time =
@@ -855,7 +858,7 @@ class PowerManagerClientStubImpl : public PowerManagerClient {
   int pause_count_;
   int cycle_count_;
   ObserverList<Observer> observers_;
-  base::RepeatingTimer<PowerManagerClientStubImpl> timer_;
+  base::RepeatingTimer<PowerManagerClientStubImpl> update_timer_;
   PowerSupplyStatus status_;
 };
 
diff --git a/chromeos/dbus/power_manager_client.h b/chromeos/dbus/power_manager_client.h
index 2e77799..c3fb5b9 100644
--- a/chromeos/dbus/power_manager_client.h
+++ b/chromeos/dbus/power_manager_client.h
@@ -56,8 +56,9 @@ class CHROMEOS_EXPORT PowerManagerClient {
                                                  const std::string& name,
                                                  int level) {}
 
-    // Called when power supply polling takes place.  |status| is a data
-    // structure that contains the current state of the power supply.
+    // Called when updated information about the power supply is available.
+    // The status is automatically updated periodically, but
+    // RequestStatusUpdate() can be used to trigger an immediate update.
     virtual void PowerChanged(const PowerSupplyStatus& status) {}
 
     // Called when we go idle for threshold time.
@@ -91,12 +92,6 @@ class CHROMEOS_EXPORT PowerManagerClient {
     virtual void IdleActionDeferred() {}
   };
 
-  enum UpdateRequestType {
-    UPDATE_INITIAL,  // Initial update request.
-    UPDATE_USER,     // User initialted update request.
-    UPDATE_POLL      // Update requested by poll signal.
-  };
-
   // Adds and removes the observer.
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
@@ -124,8 +119,9 @@ class CHROMEOS_EXPORT PowerManagerClient {
   // Increases the keyboard brightness.
   virtual void IncreaseKeyboardBrightness() = 0;
 
-  // Request for power supply status update.
-  virtual void RequestStatusUpdate(UpdateRequestType update_type) = 0;
+  // Requests an updated copy of the power status. Observer::PowerChanged()
+  // will be called asynchronously.
+  virtual void RequestStatusUpdate() = 0;
 
   // Requests restart of the system.
   virtual void RequestRestart() = 0;
diff --git a/chromeos/power/power_manager_handler.cc b/chromeos/power/power_manager_handler.cc
index c510ee4..02e3864 100644
--- a/chromeos/power/power_manager_handler.cc
+++ b/chromeos/power/power_manager_handler.cc
@@ -70,8 +70,7 @@ PowerManagerHandler::PowerManagerHandler() {
       !DBusThreadManager::Get()->GetPowerManagerClient())
     return;
   DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
-  DBusThreadManager::Get()->GetPowerManagerClient()->RequestStatusUpdate(
-      PowerManagerClient::UPDATE_INITIAL);
+  DBusThreadManager::Get()->GetPowerManagerClient()->RequestStatusUpdate();
 }
 
 PowerManagerHandler::~PowerManagerHandler() {
@@ -83,8 +82,7 @@ PowerManagerHandler::~PowerManagerHandler() {
 }
 
 void PowerManagerHandler::RequestStatusUpdate() {
-  DBusThreadManager::Get()->GetPowerManagerClient()->RequestStatusUpdate(
-      PowerManagerClient::UPDATE_USER);
+  DBusThreadManager::Get()->GetPowerManagerClient()->RequestStatusUpdate();
 }
 
 PowerSupplyStatus PowerManagerHandler::GetPowerSupplyStatus() const {
diff --git a/chromeos/power/power_manager_handler_unittest.cc b/chromeos/power/power_manager_handler_unittest.cc
index e66c7cb..ec1cb36 100644
--- a/chromeos/power/power_manager_handler_unittest.cc
+++ b/chromeos/power/power_manager_handler_unittest.cc
@@ -88,7 +88,7 @@ TEST_F(PowerManagerHandlerTest, PowerManagerInitializeAndUpdate) {
   // requests, pretends there is a battery present, and generate some
   // valid power supply status data.
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(test_observer_->power_changed_count(), 0);
+  const int initial_changed_count = test_observer_->power_changed_count();
   PowerSupplyStatus init_status = power_handler_->GetPowerSupplyStatus();
   ValidatePowerSupplyStatus(init_status);
 
@@ -96,7 +96,7 @@ TEST_F(PowerManagerHandlerTest, PowerManagerInitializeAndUpdate) {
   // status change.
   power_handler_->RequestStatusUpdate();
   message_loop_.RunUntilIdle();
-  EXPECT_GE(test_observer_->power_changed_count(), 1);
+  EXPECT_GT(test_observer_->power_changed_count(), initial_changed_count);
 }
 
 }  // namespace chromeos