Add dark resume methods to PowerManagerClient

chromeos/dbus/power_manager_client.cc

Index: chromeos/dbus/power_manager_client.cc
diff --git a/chromeos/dbus/power_manager_client.cc b/chromeos/dbus/power_manager_client.cc
index 587e43f79992af6b6acaa62f82eb49448ce849d8..2983add68aa7e36a38c3b6ff8aa178fa85bc80a2 100644
--- a/chromeos/dbus/power_manager_client.cc
+++ b/chromeos/dbus/power_manager_client.cc
@@ -48,8 +48,11 @@ class PowerManagerClientImpl : public PowerManagerClient {
         power_manager_proxy_(NULL),
         suspend_delay_id_(-1),
         has_suspend_delay_id_(false),
+        dark_suspend_delay_id_(-1),
+        has_dark_suspend_delay_id_(false),
         pending_suspend_id_(-1),
         suspend_is_pending_(false),
+        suspending_from_dark_resume_(false),
         num_pending_suspend_readiness_callbacks_(0),
         last_is_projecting_(false),
         weak_ptr_factory_(this) {}
@@ -214,7 +217,8 @@ class PowerManagerClientImpl : public PowerManagerClient {
     DCHECK(suspend_is_pending_);
     num_pending_suspend_readiness_callbacks_++;
     return base::Bind(&PowerManagerClientImpl::HandleObserverSuspendReadiness,
-                      weak_ptr_factory_.GetWeakPtr(), pending_suspend_id_);
+                      weak_ptr_factory_.GetWeakPtr(), pending_suspend_id_,
+                      suspending_from_dark_resume_);
   }
 
   virtual int GetNumPendingSuspendReadinessCallbacks() OVERRIDE {
@@ -270,8 +274,8 @@ class PowerManagerClientImpl : public PowerManagerClient {
         power_manager::kPowerManagerInterface,
         power_manager::kSuspendImminentSignal,
         base::Bind(
-            &PowerManagerClientImpl::SuspendImminentReceived,
-            weak_ptr_factory_.GetWeakPtr()),
+            &PowerManagerClientImpl::HandleSuspendImminent,
+            weak_ptr_factory_.GetWeakPtr(), false),
         base::Bind(&PowerManagerClientImpl::SignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
 
@@ -285,6 +289,15 @@ class PowerManagerClientImpl : public PowerManagerClient {
 
     power_manager_proxy_->ConnectToSignal(
         power_manager::kPowerManagerInterface,
+        power_manager::kDarkSuspendImminentSignal,
+        base::Bind(
+            &PowerManagerClientImpl::HandleSuspendImminent,
+            weak_ptr_factory_.GetWeakPtr(), true),
+        base::Bind(&PowerManagerClientImpl::SignalConnected,
+                   weak_ptr_factory_.GetWeakPtr()));
+
+    power_manager_proxy_->ConnectToSignal(
+        power_manager::kPowerManagerInterface,
         power_manager::kIdleActionImminentSignal,
         base::Bind(
             &PowerManagerClientImpl::IdleActionImminentReceived,
@@ -301,7 +314,7 @@ class PowerManagerClientImpl : public PowerManagerClient {
         base::Bind(&PowerManagerClientImpl::SignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
 
-    RegisterSuspendDelay();
+    RegisterSuspendDelays();
   }
 
  private:
@@ -336,7 +349,7 @@ class PowerManagerClientImpl : public PowerManagerClient {
             << (new_owner.empty() ? "[none]" : new_owner.c_str()) << ")";
     if (!new_owner.empty()) {
       VLOG(1) << "Sending initial state to power manager";
-      RegisterSuspendDelay();
+      RegisterSuspendDelays();
       SetIsProjecting(last_is_projecting_);
       FOR_EACH_OBSERVER(Observer, observers_, PowerManagerRestarted());
     }
@@ -344,7 +357,7 @@ class PowerManagerClientImpl : public PowerManagerClient {
 
   void BrightnessChangedReceived(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
-    int32 brightness_level = 0;
+    int32_t brightness_level = 0;
     bool user_initiated = 0;
     if (!(reader.PopInt32(&brightness_level) &&
           reader.PopBool(&user_initiated))) {
@@ -424,54 +437,68 @@ class PowerManagerClientImpl : public PowerManagerClient {
     callback.Run(percent);
   }
 
-  void OnRegisterSuspendDelayReply(dbus::Response* response) {
+  void HandleRegisterSuspendDelayReply(bool dark_suspend,
+                                       dbus::Response* response) {
     if (!response) {
-      LOG(ERROR) << "Error calling "
-                 << power_manager::kRegisterSuspendDelayMethod;
+      LOG(ERROR) << "Error calling " << response->GetMember();
       return;
     }
 
     dbus::MessageReader reader(response);
     power_manager::RegisterSuspendDelayReply protobuf;
     if (!reader.PopArrayOfBytesAsProto(&protobuf)) {
-      LOG(ERROR) << "Unable to parse reply from "
-                 << power_manager::kRegisterSuspendDelayMethod;
+      LOG(ERROR) << "Unable to parse reply from " << response->GetMember();
       return;
     }
 
-    suspend_delay_id_ = protobuf.delay_id();
-    has_suspend_delay_id_ = true;
-    VLOG(1) << "Registered suspend delay " << suspend_delay_id_;
+    if (dark_suspend) {
+      dark_suspend_delay_id_ = protobuf.delay_id();
+      has_dark_suspend_delay_id_ = true;
+      VLOG(1) << "Registered dark suspend delay " << dark_suspend_delay_id_;
+    } else {
+      suspend_delay_id_ = protobuf.delay_id();
+      has_suspend_delay_id_ = true;
+      VLOG(1) << "Registered suspend delay " << suspend_delay_id_;
+    }
   }
 
-  void SuspendImminentReceived(dbus::Signal* signal) {
-    if (!has_suspend_delay_id_) {
-      LOG(ERROR) << "Received unrequested "
-                 << power_manager::kSuspendImminentSignal << " signal";
+  void HandleSuspendImminent(bool in_dark_resume, dbus::Signal* signal) {
+    std::string signal_name = signal->GetMember();
+    if ((in_dark_resume && !has_dark_suspend_delay_id_) ||
+        (!in_dark_resume && !has_suspend_delay_id_)) {
+      LOG(ERROR) << "Received unrequested " << signal_name << " signal";
       return;
     }
 
     dbus::MessageReader reader(signal);
     power_manager::SuspendImminent proto;
     if (!reader.PopArrayOfBytesAsProto(&proto)) {
-      LOG(ERROR) << "Unable to decode protocol buffer from "
-                 << power_manager::kSuspendImminentSignal << " signal";
+      LOG(ERROR) << "Unable to decode protocol buffer from " << signal_name
+                 << " signal";
       return;
     }
 
-    VLOG(1) << "Got " << power_manager::kSuspendImminentSignal << " signal "
-            << "announcing suspend attempt " << proto.suspend_id();
-    if (suspend_is_pending_) {
-      LOG(WARNING) << "Got " << power_manager::kSuspendImminentSignal
-                   << " signal about pending suspend attempt "
-                   << proto.suspend_id() << " while still waiting "
-                   << "on attempt " << pending_suspend_id_;
+    VLOG(1) << "Got " << signal_name << " signal announcing suspend attempt "
+            << proto.suspend_id();
+
+    // If a previous suspend is pending from the same state we are currently in
+    // (fully powered on or in dark resume), then something's gone a little
+    // wonky.
+    if (suspend_is_pending_ &&
+        suspending_from_dark_resume_ == in_dark_resume) {
+      LOG(WARNING) << "Got " << signal_name << " signal about pending suspend "
+                   << "attempt " << proto.suspend_id() << " while still "
+                   << "waiting on attempt " << pending_suspend_id_;
     }
 
     pending_suspend_id_ = proto.suspend_id();
     suspend_is_pending_ = true;
+    suspending_from_dark_resume_ = in_dark_resume;
     num_pending_suspend_readiness_callbacks_ = 0;
-    FOR_EACH_OBSERVER(Observer, observers_, SuspendImminent());
+    if (suspending_from_dark_resume_)
+      FOR_EACH_OBSERVER(Observer, observers_, DarkSuspendImminent());
+    else
+      FOR_EACH_OBSERVER(Observer, observers_, SuspendImminent());
     MaybeReportSuspendReadiness();
   }
 
@@ -556,18 +583,32 @@ class PowerManagerClientImpl : public PowerManagerClient {
     }
   }
 
-  // Registers a suspend delay with the power manager.  This is usually
-  // only called at startup, but if the power manager restarts, we need to
-  // create a new delay.
-  void RegisterSuspendDelay() {
+  void RegisterSuspendDelayImpl(
+      const std::string& method_name,
+      const power_manager::RegisterSuspendDelayRequest& protobuf_request,
+      dbus::ObjectProxy::ResponseCallback callback) {
+    dbus::MethodCall method_call(
+        power_manager::kPowerManagerInterface, method_name);
+    dbus::MessageWriter writer(&method_call);
+
+    if (!writer.AppendProtoAsArrayOfBytes(protobuf_request)) {
+      LOG(ERROR) << "Error constructing message for " << method_name;
+      return;
+    }
+
+    power_manager_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, callback);
+  }
+
+  // Registers suspend delays with the power manager.  This is usually only
+  // called at startup, but if the power manager restarts, we need to create new
+  // delays.
+  void RegisterSuspendDelays() {

[pprabhu, 2014/08/26 18:13:18]

Fly by late comment: Shouldn't this also reset |suspend_is_pending_| and
|suspending_from_dark_resume_|?

Consider the case when power_manager dies after we receive a
(Dark)SuspendImminent signal, we will reset the state of PowerManagerClient, but
will end up with incorrect initial values for these two variables.

[Daniel Erat, 2014/08/26 18:16:33]

probably wouldn't hurt to do that here, but i think that powerd has an ugly hack
to send a fake SuspendDone event at startup in this case

[Chirantan Ekbote, 2014/08/26 19:10:54]

But it looks like |suspend_is_pending_| is not cleared in SuspendDoneReceived. 
It only gets cleared in MaybeReportSuspendReadiness.  So we could have a problem
if the following sequence occurs:

- Chrome receives a {Dark}SuspendImminent
- One of the observers takes a callback
- powerd goes away
- powerd restarts
- The observer runs the callback

In this case, chrome would report suspend readiness with an id of -1 or whatever
id powerd assigned to it (depending on whether or not chrome received the
RegisterSuspendDelay reply before running the observer callback).  I think
powerd can deal with either case but we should probably prevent chrome from
sending the message in the first place.  Plus, if for whatever reason powerd
doesn't restart, then chrome would still report suspend readiness only to
receive a ServiceUnknown error in return.

Maybe this should be cleared in NameOwnerChangedReceived?  I'll upload a CL to
do this.

     // Throw out any old delay that was registered.
     suspend_delay_id_ = -1;
     has_suspend_delay_id_ = false;
-
-    dbus::MethodCall method_call(
-        power_manager::kPowerManagerInterface,
-        power_manager::kRegisterSuspendDelayMethod);
-    dbus::MessageWriter writer(&method_call);
+    dark_suspend_delay_id_ = -1;
+    has_dark_suspend_delay_id_ = false;
 
     power_manager::RegisterSuspendDelayRequest protobuf_request;
     base::TimeDelta timeout =
@@ -575,26 +616,28 @@ class PowerManagerClientImpl : public PowerManagerClient {
     protobuf_request.set_timeout(timeout.ToInternalValue());
     protobuf_request.set_description(kSuspendDelayDescription);
 
-    if (!writer.AppendProtoAsArrayOfBytes(protobuf_request)) {
-      LOG(ERROR) << "Error constructing message for "
-                 << power_manager::kRegisterSuspendDelayMethod;
-      return;
-    }
-    power_manager_proxy_->CallMethod(
-        &method_call,
-        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+    RegisterSuspendDelayImpl(
+        power_manager::kRegisterSuspendDelayMethod,
+        protobuf_request,
+        base::Bind(
+            &PowerManagerClientImpl::HandleRegisterSuspendDelayReply,
+            weak_ptr_factory_.GetWeakPtr(), false));
+    RegisterSuspendDelayImpl(
+        power_manager::kRegisterDarkSuspendDelayMethod,
+        protobuf_request,
         base::Bind(
-            &PowerManagerClientImpl::OnRegisterSuspendDelayReply,
-            weak_ptr_factory_.GetWeakPtr()));
+            &PowerManagerClientImpl::HandleRegisterSuspendDelayReply,
+            weak_ptr_factory_.GetWeakPtr(), true));
   }
 
   // Records the fact that an observer has finished doing asynchronous work
   // that was blocking a pending suspend attempt and possibly reports
   // suspend readiness to powerd.  Called by callbacks returned via
   // GetSuspendReadinessCallback().
-  void HandleObserverSuspendReadiness(int32 suspend_id) {
+  void HandleObserverSuspendReadiness(int32_t suspend_id, bool in_dark_resume) {
     DCHECK(OnOriginThread());
-    if (!suspend_is_pending_ || suspend_id != pending_suspend_id_)
+    if (!suspend_is_pending_ || suspend_id != pending_suspend_id_ ||
+        in_dark_resume != suspending_from_dark_resume_)
       return;
 
     num_pending_suspend_readiness_callbacks_--;
@@ -607,23 +650,31 @@ class PowerManagerClientImpl : public PowerManagerClient {
     if (!suspend_is_pending_ || num_pending_suspend_readiness_callbacks_ > 0)
       return;
 
+    std::string method_name;
+    int32_t delay_id = -1;
+    if (suspending_from_dark_resume_) {
+      method_name = power_manager::kHandleDarkSuspendReadinessMethod;
+      delay_id = dark_suspend_delay_id_;
+    } else {
+      method_name = power_manager::kHandleSuspendReadinessMethod;
+      delay_id = suspend_delay_id_;
+    }
+
     dbus::MethodCall method_call(
-        power_manager::kPowerManagerInterface,
-        power_manager::kHandleSuspendReadinessMethod);
+        power_manager::kPowerManagerInterface, method_name);
     dbus::MessageWriter writer(&method_call);
 
-    VLOG(1) << "Announcing readiness of suspend delay " << suspend_delay_id_
+    VLOG(1) << "Announcing readiness of suspend delay " << delay_id
             << " for suspend attempt " << pending_suspend_id_;
     power_manager::SuspendReadinessInfo protobuf_request;
-    protobuf_request.set_delay_id(suspend_delay_id_);
+    protobuf_request.set_delay_id(delay_id);
     protobuf_request.set_suspend_id(pending_suspend_id_);
 
     pending_suspend_id_ = -1;
     suspend_is_pending_ = false;
 
     if (!writer.AppendProtoAsArrayOfBytes(protobuf_request)) {
-      LOG(ERROR) << "Error constructing message for "
-                 << power_manager::kHandleSuspendReadinessMethod;
+      LOG(ERROR) << "Error constructing message for " << method_name;
       return;
     }
     power_manager_proxy_->CallMethod(
@@ -639,14 +690,24 @@ class PowerManagerClientImpl : public PowerManagerClient {
   ObserverList<Observer> observers_;
 
   // The delay_id_ obtained from the RegisterSuspendDelay request.
-  int32 suspend_delay_id_;
+  int32_t suspend_delay_id_;
   bool has_suspend_delay_id_;
 
+  // The delay_id_ obtained from the RegisterDarkSuspendDelay request.
+  int32_t dark_suspend_delay_id_;
+  bool has_dark_suspend_delay_id_;
+
   // powerd-supplied ID corresponding to an imminent suspend attempt that is
   // currently being delayed.
-  int32 pending_suspend_id_;
+  int32_t pending_suspend_id_;
   bool suspend_is_pending_;
 
+  // Set to true when the suspend currently being delayed was triggered during a
+  // dark resume.  Since |pending_suspend_id_| and |suspend_is_pending_| are
+  // both shared by normal and dark suspends, |suspending_from_dark_resume_|
+  // helps distinguish the context within which these variables are being used.
+  bool suspending_from_dark_resume_;
+
   // Number of callbacks that have been returned by
   // GetSuspendReadinessCallback() during the currently-pending suspend
   // attempt but have not yet been called.