Implement support for monitor suspend.

chromeos/dbus/power_manager_client.cc

 // Copyright (c) 2012 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 "chromeos/dbus/power_manager_client.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "chromeos/dbus/power_manager/input_event.pb.h"
 #include "chromeos/dbus/power_manager/suspend.pb.h"
 #include "chromeos/dbus/power_state_control.pb.h"
 #include "chromeos/dbus/power_supply_properties.pb.h"
 #include "chromeos/dbus/video_activity_update.pb.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
+const int kSuspendDelayTimeoutMs = 5000;
+
 // The PowerManagerClient implementation used in production.
 class PowerManagerClientImpl : public PowerManagerClient {
  public:
   explicit PowerManagerClientImpl(dbus::Bus* bus)
       : power_manager_proxy_(NULL),
+        has_delay_id_(false),

[Daniel Erat, 2013/01/08 00:51:35]

nit: rename to be more descriptive: has_suspend_delay_id_

also initialize suspend_delay_id_ to something like 0 or -1 here (just so it
doesn't contain a random value that could make it hard to investigate bugs
later)

[marcheu, 2013/01/10 01:03:49]

Done.

         weak_ptr_factory_(this) {
     power_manager_proxy_ = bus->GetObjectProxy(
         power_manager::kPowerManagerServiceName,
         dbus::ObjectPath(power_manager::kPowerManagerServicePath));
 
     // Monitor the D-Bus signal for brightness changes. Only the power
     // manager knows the actual brightness level. We don't cache the
     // brightness level in Chrome as it'll make things less reliable.
     power_manager_proxy_->ConnectToSignal(
         power_manager::kPowerManagerInterface,
@@ skipping 44 matching lines @@
         base::Bind(&PowerManagerClientImpl::SignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
 
     power_manager_proxy_->ConnectToSignal(
         power_manager::kPowerManagerInterface,
         power_manager::kSuspendStateChangedSignal,
         base::Bind(&PowerManagerClientImpl::SuspendStateChangedReceived,
                    weak_ptr_factory_.GetWeakPtr()),
         base::Bind(&PowerManagerClientImpl::SignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
+
+    power_manager_proxy_->ConnectToSignal(
+        power_manager::kPowerManagerInterface,
+        power_manager::kSuspendImminentSignal,
+        base::Bind(
+            &PowerManagerClientImpl::SuspendImminentReceived,
+            weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&PowerManagerClientImpl::SignalConnected,
+                   weak_ptr_factory_.GetWeakPtr()));
+
+    // Register to powerd for suspend notifications.
+    dbus::MethodCall method_call(
+        power_manager::kPowerManagerInterface,
+    power_manager::kRegisterSuspendDelayMethod);

[Daniel Erat, 2013/01/08 00:51:35]

nit: fix indenting

[marcheu, 2013/01/10 01:03:49]

Done.

+    dbus::MessageWriter writer(&method_call);
+
+    power_manager::RegisterSuspendDelayRequest protobuf_request;
+    base::TimeDelta timeout =
+        base::TimeDelta::FromMilliseconds(kSuspendDelayTimeoutMs);
+    protobuf_request.Clear();

[Daniel Erat, 2013/01/08 00:51:35]

nit: don't need to call Clear() on a new protobuf, i don't think

[marcheu, 2013/01/10 01:03:49]

Done.

+    protobuf_request.set_timeout(timeout.ToInternalValue());
+
+    if (!writer.AppendProtoAsArrayOfBytes(protobuf_request)) {
+      LOG(ERROR) << "Error calling "

[Daniel Erat, 2013/01/08 00:51:35]

nit: change this to "Error constructing message for " <<
power_manager::kRegisterSuspendDelayMethod;

[marcheu, 2013/01/10 01:03:49]

Done.

+                 << power_manager::kRegisterSuspendDelayMethod;
+      return;
+    }
+    power_manager_proxy_->CallMethod(
+        &method_call,
+        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(
+            &PowerManagerClientImpl::OnRegisterSuspendDelayReply,
+            weak_ptr_factory_.GetWeakPtr()));
   }
 
   virtual ~PowerManagerClientImpl() {
+    // Unregister from powerd for suspend notifications.
+    if (!has_delay_id_) {
+      LOG(ERROR) << "Destroying PowerManagerClientImpl without a delay_id_";
+      return;

[Daniel Erat, 2013/01/08 00:51:35]

i'm pretty sure you don't want to bail out early from the d'tor like this --
what if someone else adds something unrelated to it later?  i'd do:

  if (has_delay_id_) {
    dbus::MethodCall ...
  }

instead.  if you want to log an error if registration fails, it'd be better to
do it in response to the RegisterSuspendDelay call failing.

[marcheu, 2013/01/10 01:03:49]

Done.

+    }
+
+    dbus::MethodCall method_call(
+        power_manager::kPowerManagerInterface,
+    power_manager::kUnregisterSuspendDelayMethod);
+    dbus::MessageWriter writer(&method_call);
+
+    power_manager::UnregisterSuspendDelayRequest protobuf;
+    protobuf.Clear();
+    protobuf.set_delay_id(delay_id_);
+
+    power_manager_proxy_->CallMethod(
+        &method_call,
+        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        dbus::ObjectProxy::EmptyResponseCallback());
   }
 
   // PowerManagerClient overrides:
 
   virtual void AddObserver(Observer* observer) OVERRIDE {
     CHECK(observer);  // http://crbug.com/119976
     observers_.AddObserver(observer);
   }
 
   virtual void RemoveObserver(Observer* observer) OVERRIDE {
@@ skipping 325 matching lines @@
       return;
     }
     dbus::MessageReader reader(response);
     double percent = 0.0;
     if (!reader.PopDouble(&percent))
       LOG(ERROR) << "Error reading response from powerd: "
                  << response->ToString();
     callback.Run(percent);
   }
 
+  void OnRegisterSuspendDelayReply(
+      dbus::Response* response) {

[Daniel Erat, 2013/01/08 00:51:35]

nit: unwrap this line

[marcheu, 2013/01/10 01:03:49]

Done.

+    if (!response) {
+      LOG(ERROR) << "Error calling "
+                 << power_manager::kRegisterSuspendDelayMethod;
+      return;
+    }
+    dbus::MessageReader reader(response);
+    power_manager::RegisterSuspendDelayReply protobuf;
+    if (!reader.PopArrayOfBytesAsProto(&protobuf)) {
+      LOG(ERROR) << "Error calling "
+                 << power_manager::kRegisterSuspendDelayMethod
+                 << response->ToString();

[Daniel Erat, 2013/01/08 00:51:35]

nit: change this to be more descriptive, like:

  "Unable to parse reply from " << power_manager::kRegisterSuspendDelayMethod

i don't think that logging the response here is going to be useful; it'll just
be a bunch of binary data

[marcheu, 2013/01/10 01:03:49]

Done.

+      return;
+    }
+    delay_id_ = protobuf.delay_id();
+    has_delay_id_ = true;
+  }
+
   void IdleNotifySignalReceived(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
     int64 threshold = 0;
     if (!reader.PopInt64(&threshold)) {
       LOG(ERROR) << "Idle Notify signal had incorrect parameters: "
                  << signal->ToString();
       return;
     }
     DCHECK_GT(threshold, 0);
 
@@ skipping 17 matching lines @@
         break;
       case power_manager::kSoftwareScreenDimmingIdle:
         state = Observer::SCREEN_DIMMING_IDLE;
         break;
       default:
         LOG(ERROR) << "Unhandled screen dimming state " << signal_state;
     }
     FOR_EACH_OBSERVER(Observer, observers_, ScreenDimmingRequested(state));
   }
 
+  void SuspendImminentReceived(dbus::Signal* signal) {
+    if (!has_delay_id_) {
+      LOG(ERROR) << "Received unrequested "
+                 << power_manager::kSuspendImminentSignal << " signal";
+      return;
+    }
+
+    dbus::MessageReader reader(signal);
+    power_manager::SuspendImminent protobuf_imminent;
+    if (!reader.PopArrayOfBytesAsProto(&protobuf_imminent)) {
+      LOG(ERROR) << "Unable to decode protocol buffer from "
+                 << power_manager::kSuspendImminentSignal << " signal";
+      return;
+    }
+    int32 suspend_id = protobuf_imminent.suspend_id();
+
+    FOR_EACH_OBSERVER(Observer, observers_, SuspendImminent());
+
+    dbus::MethodCall method_call(
+        power_manager::kPowerManagerInterface,
+    power_manager::kHandleSuspendReadinessMethod);

[Daniel Erat, 2013/01/08 00:51:35]

nit: fix indenting

[marcheu, 2013/01/10 01:03:49]

Done.

+    dbus::MessageWriter writer(&method_call);
+
+    power_manager::SuspendReadinessInfo protobuf_request;
+    protobuf_request.Clear();

[Daniel Erat, 2013/01/08 00:51:35]

nit: don't need Clear() here

[marcheu, 2013/01/10 01:03:49]

Done.

+    protobuf_request.set_delay_id(delay_id_);
+    protobuf_request.set_suspend_id(suspend_id);
+
+    if (!writer.AppendProtoAsArrayOfBytes(protobuf_request)) {
+      LOG(ERROR) << "Error calling "

[Daniel Erat, 2013/01/08 00:51:35]

nit: "Error constructing message for " << ...

[marcheu, 2013/01/10 01:03:49]

Done.

+                 << power_manager::kHandleSuspendReadinessMethod;
+      return;
+    }
+    power_manager_proxy_->CallMethod(
+        &method_call,
+        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        dbus::ObjectProxy::EmptyResponseCallback());
+  }
+
   void InputEventReceived(dbus::Signal* signal) {
     dbus::MessageReader reader(signal);
     power_manager::InputEvent proto;
     if (!reader.PopArrayOfBytesAsProto(&proto)) {
       LOG(ERROR) << "Unable to decode protocol buffer from "
                  << power_manager::kInputEventSignal << " signal";
       return;
     }
 
     base::TimeTicks timestamp =
@@ skipping 41 matching lines @@
         FOR_EACH_OBSERVER(
             PowerManagerClient::Observer, observers_,
             SystemResumed(wall_time - last_suspend_wall_time_));
         break;
     }
   }
 
   dbus::ObjectProxy* power_manager_proxy_;
   ObserverList<Observer> observers_;
 
+  // The delay_id_ obtained from the RegisterSuspendDelay request.
+  int32 delay_id_;

[Daniel Erat, 2013/01/08 00:51:35]

nit: rename this to suspend_delay_id_, too

[marcheu, 2013/01/10 01:03:49]

Done.

+  bool has_delay_id_;
+
   // Wall time from the latest signal telling us that the system was about to
   // suspend to memory.
   base::Time last_suspend_wall_time_;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<PowerManagerClientImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PowerManagerClientImpl);
 };
@@ skipping 159 matching lines @@
 PowerManagerClient* PowerManagerClient::Create(
     DBusClientImplementationType type,
     dbus::Bus* bus) {
   if (type == REAL_DBUS_CLIENT_IMPLEMENTATION)
     return new PowerManagerClientImpl(bus);
   DCHECK_EQ(STUB_DBUS_CLIENT_IMPLEMENTATION, type);
   return new PowerManagerClientStubImpl();
 }
 
 }  // namespace chromeos