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..6f42ad657488145ff102ce641b77287f34604e27 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) {}
@@ -285,6 +288,15 @@ class PowerManagerClientImpl : public PowerManagerClient {
 
     power_manager_proxy_->ConnectToSignal(
         power_manager::kPowerManagerInterface,
+        power_manager::kDarkSuspendImminentSignal,
+        base::Bind(
+            &PowerManagerClientImpl::DarkSuspendImminentReceived,
+            weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&PowerManagerClientImpl::SignalConnected,
+                   weak_ptr_factory_.GetWeakPtr()));
+
+    power_manager_proxy_->ConnectToSignal(
+        power_manager::kPowerManagerInterface,
         power_manager::kIdleActionImminentSignal,
         base::Bind(
             &PowerManagerClientImpl::IdleActionImminentReceived,
@@ -301,7 +313,7 @@ class PowerManagerClientImpl : public PowerManagerClient {
         base::Bind(&PowerManagerClientImpl::SignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
 
-    RegisterSuspendDelay();
+    RegisterSuspendDelays();
   }
 
  private:
@@ -336,7 +348,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());
     }
@@ -424,54 +436,78 @@ class PowerManagerClientImpl : public PowerManagerClient {
     callback.Run(percent);
   }
 
-  void OnRegisterSuspendDelayReply(dbus::Response* response) {
+  void OnRegisterSuspendDelayReply(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";
+    SuspendImminentCommon(false, signal);
+  }
+
+  void DarkSuspendImminentReceived(dbus::Signal* signal) {

[Daniel Erat, 2014/08/15 23:49:31]

can you just eliminate these and bind true or false to the shared method and use
that as the callback?

[Chirantan Ekbote, 2014/08/18 17:49:08]

sure.  will fix.

+    SuspendImminentCommon(true, signal);
+  }
+
+  void SuspendImminentCommon(bool in_dark_resume,

[Daniel Erat, 2014/08/15 23:49:31]

nit: HandleSuspendImminent()?

+                             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_;

[Daniel Erat, 2014/08/15 23:49:31]

i'm trying to think of when this could happen -- when someone within chrome
registers an async delay and then waits too long to call it, resulting in the
delay timing out and powerd suspending anyway?

in general, does suspend_is_pending_ being true here indicate a problem?

[Chirantan Ekbote, 2014/08/18 17:49:08]

Yes, that's the situation in which this could happen.  I don't _think_ that it's
a big problem if suspend_is_pending_ is true here but I guess it depends on what
the delays are doing.  From the PowerManagerClient's perspective, we would let
any outstanding delays continue running and when they run the suspend readiness
callback we would just ignore it.

     }
 
     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_) {

[Daniel Erat, 2014/08/15 23:49:31]

nit: omit curly brackets

+      FOR_EACH_OBSERVER(Observer, observers_, DarkSuspendImminent());
+    } else {
+      FOR_EACH_OBSERVER(Observer, observers_, SuspendImminent());
+    }
     MaybeReportSuspendReadiness();
   }
 
@@ -556,18 +592,24 @@ 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() {
+  // 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() {
     // Throw out any old delay that was registered.
     suspend_delay_id_ = -1;
     has_suspend_delay_id_ = false;
+    dark_suspend_delay_id_ = -1;
+    has_dark_suspend_delay_id_ = false;
 
     dbus::MethodCall method_call(
         power_manager::kPowerManagerInterface,
         power_manager::kRegisterSuspendDelayMethod);
     dbus::MessageWriter writer(&method_call);
+    dbus::MethodCall dark_method_call(
+        power_manager::kPowerManagerInterface,
+        power_manager::kRegisterDarkSuspendDelayMethod);
+    dbus::MessageWriter dark_writer(&dark_method_call);
 
     power_manager::RegisterSuspendDelayRequest protobuf_request;
     base::TimeDelta timeout =
@@ -580,12 +622,23 @@ class PowerManagerClientImpl : public PowerManagerClient {
                  << power_manager::kRegisterSuspendDelayMethod;
       return;
     }
+    if (!dark_writer.AppendProtoAsArrayOfBytes(protobuf_request)) {
+      LOG(ERROR) << "Error constructing message for "
+                 << power_manager::kRegisterDarkSuspendDelayMethod;
+      return;
+    }
     power_manager_proxy_->CallMethod(
         &method_call,
         dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(
             &PowerManagerClientImpl::OnRegisterSuspendDelayReply,
-            weak_ptr_factory_.GetWeakPtr()));
+            weak_ptr_factory_.GetWeakPtr(), false));
+    power_manager_proxy_->CallMethod(

[Daniel Erat, 2014/08/15 23:49:31]

add a helper method for this to cut down on the duplicated code?

+        &dark_method_call,
+        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(
+            &PowerManagerClientImpl::OnRegisterSuspendDelayReply,
+            weak_ptr_factory_.GetWeakPtr(), true));
   }
 
   // Records the fact that an observer has finished doing asynchronous work
@@ -607,23 +660,31 @@ class PowerManagerClientImpl : public PowerManagerClient {
     if (!suspend_is_pending_ || num_pending_suspend_readiness_callbacks_ > 0)
       return;
 
+    std::string method_name;
+    int32 delay_id;
+    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(
@@ -642,10 +703,15 @@ class PowerManagerClientImpl : public PowerManagerClient {
   int32 suspend_delay_id_;
   bool has_suspend_delay_id_;
 
+  // The delay_id_ obtained from the RegisterDarkSuspendDelay request.
+  int32 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_;
   bool suspend_is_pending_;
+  bool suspending_from_dark_resume_;

[Daniel Erat, 2014/08/15 23:49:31]

add a comment clarifying what this means, and that it affects the interpretation
of pending_suspend_id_ and suspend_is_pending_

 
   // Number of callbacks that have been returned by
   // GetSuspendReadinessCallback() during the currently-pending suspend