Implement PowerSaveBlocker2 for Linux. Much simpler than the original!

content/browser/power_save_blocker_linux.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 "content/browser/power_save_blocker.h"
 
 #include <X11/Xlib.h>
 #include <X11/extensions/dpms.h>
 // Xlib #defines Status, but we can't have that for some of our headers.
 #ifdef Status
@@ skipping 486 matching lines @@
 }
 
 }  // namespace
 
 // Called only from UI thread.
 // static
 void PowerSaveBlocker::ApplyBlock(PowerSaveBlockerType type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DBusPowerSaveBlocker::GetInstance()->ApplyBlock(type);
 }
+
+// TODO(mdm): remove from the beginning of the file up to (and including) this
+// line to switch to the new implementation.
+#define PowerSaveBlocker PowerSaveBlocker2

[satorux1, 2012/06/08 14:59:41]

This looks ugly. Why do you need to keep the old implementation? 

[Mike Mammarella, 2012/06/08 18:33:36]

We don't, strictly speaking. We had to do this for the other two platforms
because there were three of us doing one platform each, and we can't switch over
until all three (well four, including Android, but it's just a stub) have
finished the new versions.

I wanted to do this CL in the same style as the others (see
power_save_blocker_win.cc for instance that also does this), and then do the
cleanup CL next to switch over. I will remove this line in literally my very
next CL.

+
+namespace content {
+
+class PowerSaveBlocker::Delegate
+    : public base::RefCountedThreadSafe<PowerSaveBlocker::Delegate> {
+ public:
+  // Picks an appropriate DBus API to use based on the desktop environment.
+  Delegate(PowerSaveBlockerType type, const std::string& reason);
+
+  void ApplyBlock();
+  void RemoveBlock();

[satorux1, 2012/06/08 14:59:41]

function comments are missing.

[Mike Mammarella, 2012/06/08 18:33:36]

I actually had them in a previous patch set, but I removed them because I
thought they were superfluous. They basically added nothing to the function
names. Is there really any confusion about what these functions do?

[satorux1, 2012/06/08 19:31:50]

From function name alone, it's hard to tell what ApplyBlock() does. It's
probably obvious for the implementer who knows the context, but would be nice to
explain what does the function do.

Besides, this is from the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it. 

[Mike Mammarella, 2012/06/08 19:50:17]

OK, I added comments. (The last patch set had them back, but I added more.)

+
+ private:
+  enum DBusAPI {
+    kNoAPI,           // Disable. No supported API available.
+    kGnomeAPI,        // Use the GNOME API. (Supports more features.)
+    kFreeDesktopAPI,  // Use the FreeDesktop API, for KDE4 and XFCE.
+  };
+
+  // Inhibit flags defined in the org.gnome.SessionManager interface.
+  // Can be OR'd together and passed as argument to the Inhibit() method
+  // to specify which power management features we want to suspend.
+  enum GnomeAPIInhibitFlags {
+    kInhibitLogOut            = 1,
+    kInhibitSwitchUser        = 2,
+    kInhibitSuspendSession    = 4,
+    kInhibitMarkSessionAsIdle = 8
+  };
+
+  static const char kGnomeAPIServiceName[];
+  static const char kGnomeAPIInterfaceName[];
+  static const char kGnomeAPIObjectPath[];
+  static const char kFreeDesktopAPIServiceName[];
+  static const char kFreeDesktopAPIInterfaceName[];
+  static const char kFreeDesktopAPIObjectPath[];

[satorux1, 2012/06/08 14:59:41]

nit: would be simpler to define these constants in anonymous namespace than in
the class.

[Mike Mammarella, 2012/06/08 18:33:36]

Done.

+
+  friend class base::RefCountedThreadSafe<Delegate>;
+  ~Delegate() {}
+
+  // If DPMS is not enabled, then we don't want to try to disable power saving,

[satorux1, 2012/06/08 14:59:41]

what does DPMS stand for? Please add some comment about it.

[Mike Mammarella, 2012/06/08 18:33:36]

Improved the comment. It's not particularly relevant what DPMS actually stands
for (I had to look it up myself just now), but it's worth saying what it is.

+  // since on some desktop environments that may enable DPMS with very poor
+  // default settings (e.g. turning off the display after only 1 second).
+  static bool DPMSEnabled();
+
+  // Returns an appropriate DBus API to use based on the desktop environment.

[satorux1, 2012/06/08 14:59:41]

nit: DBus -> D-Bus

[Mike Mammarella, 2012/06/08 18:33:36]

Done.

+  static DBusAPI SelectAPI();
+
+  const PowerSaveBlockerType type_;
+  const std::string reason_;
+  const DBusAPI api_;
+
+  scoped_refptr<dbus::Bus> bus_;
+
+  // The cookie that identifies our inhibit request,
+  // or 0 if there is no active inhibit request.
+  uint32 inhibit_cookie_;
+
+  DISALLOW_COPY_AND_ASSIGN(Delegate);
+};
+
+const char PowerSaveBlocker::Delegate::kGnomeAPIServiceName[] =
+    "org.gnome.SessionManager";
+const char PowerSaveBlocker::Delegate::kGnomeAPIInterfaceName[] =
+    "org.gnome.SessionManager";
+const char PowerSaveBlocker::Delegate::kGnomeAPIObjectPath[] =
+    "/org/gnome/SessionManager";
+
+const char PowerSaveBlocker::Delegate::kFreeDesktopAPIServiceName[] =
+    "org.freedesktop.PowerManagement";
+const char PowerSaveBlocker::Delegate::kFreeDesktopAPIInterfaceName[] =
+    "org.freedesktop.PowerManagement.Inhibit";
+const char PowerSaveBlocker::Delegate::kFreeDesktopAPIObjectPath[] =
+    "/org/freedesktop/PowerManagement/Inhibit";
+
+PowerSaveBlocker::Delegate::Delegate(PowerSaveBlockerType type,
+                                     const std::string& reason)
+    : type_(type), reason_(reason), api_(SelectAPI()) {
+  // We're on the client's thread here, so we don't allocate the dbus::Bus
+  // object yet. We'll do it below in ApplyBlock(), on the file thread.
+}
+
+void PowerSaveBlocker::Delegate::ApplyBlock() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!bus_.get());  // ApplyBlock() should only be called once.
+  if (api_ == kNoAPI)
+    return;
+
+  dbus::Bus::Options options;
+  options.bus_type = dbus::Bus::SESSION;
+  options.connection_type = dbus::Bus::PRIVATE;
+  bus_ = new dbus::Bus(options);
+
+  scoped_refptr<dbus::ObjectProxy> object_proxy;
+  // We must provide a valid-looking interface. We'll change it below.

[satorux1, 2012/06/08 14:59:41]

This looks lie a hack. Rather than replacing it later, what about doing
something like:

scoped_ptr<dbus::MethodCall> method_call;
...

case kGnomeAPI:
   ...
   method_call.reset(new MethodCall(kGnomeAPIInterfaceName, "Inhibit"));

[Mike Mammarella, 2012/06/08 18:33:36]

I considered that but thought this seemed a little nicer. Since you prefer the
other way I can change it, though how would you feel about adding an if
statement to MethodCall::MethodCall() to only call SetInterface() on a non-empty
string?

[satorux1, 2012/06/08 19:31:50]

I think using a dummy place holder does not look great. I'd prefer the scoped
ptr solution.
Let's not change MethodCall. It takes the two parameters as these are mandatory.

[Mike Mammarella, 2012/06/08 19:50:17]

Done. Personally I'm not a fan of the way this cascades into having to change
message_writer as well. It seems to me that since you can change the interface
with SetInterface() after construction it's not really important that it be
mandatory in the constructor.

+  dbus::MethodCall method_call("org.chromium.Placeholder", "Inhibit");
+  dbus::MessageWriter message_writer(&method_call);
+
+  switch (api_) {
+    case kNoAPI:
+      NOTREACHED();  // We return early above.
+      return;
+    case kGnomeAPI:
+      object_proxy = bus_->GetObjectProxy(
+          kGnomeAPIServiceName,
+          dbus::ObjectPath(kGnomeAPIObjectPath));
+      method_call.SetInterface(kGnomeAPIInterfaceName);
+      // The arguments of the method are:
+      //     app_id:        The application identifier
+      //     toplevel_xid:  The toplevel X window identifier
+      //     reason:        The reason for the inhibit
+      //     flags:         Flags that spefify what should be inhibited
+      message_writer.AppendString(
+          CommandLine::ForCurrentProcess()->GetProgram().value());
+      message_writer.AppendUint32(0);  // should be toplevel_xid
+      message_writer.AppendString(reason_.c_str());

[satorux1, 2012/06/08 14:59:41]

no need to use .c_str()

[Mike Mammarella, 2012/06/08 18:33:36]

Done.

+      {
+        uint32 flags = 0;
+        switch (type_) {
+          case kPowerSaveBlockPreventDisplaySleep:
+            flags |= kInhibitMarkSessionAsIdle;
+            flags |= kInhibitSuspendSession;
+            break;
+          case kPowerSaveBlockPreventAppSuspension:
+            flags |= kInhibitSuspendSession;
+            break;
+        }
+        message_writer.AppendUint32(flags);
+      }
+      break;
+    case kFreeDesktopAPI:
+      object_proxy = bus_->GetObjectProxy(
+          kFreeDesktopAPIServiceName,
+          dbus::ObjectPath(kFreeDesktopAPIObjectPath));
+      method_call.SetInterface(kFreeDesktopAPIInterfaceName);
+      // The arguments of the method are:
+      //     app_id:        The application identifier
+      //     reason:        The reason for the inhibit
+      message_writer.AppendString(
+          CommandLine::ForCurrentProcess()->GetProgram().value());
+      message_writer.AppendString(reason_.c_str());
+      break;
+  }
+
+  // We could do this method call asynchronously, but if we did, we'd need to
+  // handle the case where we want to cancel the block before we get a reply.
+  // We're on the FILE thread so it should be OK to block briefly here.
+  scoped_ptr<dbus::Response> response(object_proxy->CallMethodAndBlock(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+  if (response.get()) {
+    // The method returns an inhibit_cookie, used to uniquely identify
+    // this request. It should be used as an argument to Uninhibit()
+    // in order to remove the request.
+    dbus::MessageReader message_reader(response.get());
+    if (!message_reader.PopUint32(&inhibit_cookie_))
+      LOG(ERROR) << "Invalid Inhibit() response: " << response->ToString();
+  } else {
+    LOG(ERROR) << "No response to Inhibit() request!";
+  }
+}
+
+void PowerSaveBlocker::Delegate::RemoveBlock() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (api_ == kNoAPI)
+    return;
+  DCHECK(bus_.get());  // RemoveBlock() should only be called once.
+
+  scoped_refptr<dbus::ObjectProxy> object_proxy;
+  // We must provide a valid-looking interface. We'll change it below.

[satorux1, 2012/06/08 14:59:41]

see my comment above.

+  dbus::MethodCall method_call("org.chromium.Placeholder", "Uninhibit");
+  dbus::MessageWriter message_writer(&method_call);
+
+  switch (api_) {
+    case kNoAPI:
+      NOTREACHED();  // We return early above.
+      return;
+    case kGnomeAPI:
+      object_proxy = bus_->GetObjectProxy(
+          kGnomeAPIServiceName,
+          dbus::ObjectPath(kGnomeAPIObjectPath));
+      method_call.SetInterface(kGnomeAPIInterfaceName);
+      break;
+    case kFreeDesktopAPI:
+      object_proxy = bus_->GetObjectProxy(
+          kFreeDesktopAPIServiceName,
+          dbus::ObjectPath(kFreeDesktopAPIObjectPath));
+      method_call.SetInterface(kFreeDesktopAPIInterfaceName);
+      break;
+  }
+
+  message_writer.AppendUint32(inhibit_cookie_);
+  scoped_ptr<dbus::Response> response(object_proxy->CallMethodAndBlock(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+  if (!response.get())
+    LOG(ERROR) << "No response to Uninhibit() request!";
+  // We don't care about checking the result. We assume it works; we can't
+  // really do anything about it anyway if it fails.
+  inhibit_cookie_ = 0;
+
+  bus_->ShutdownAndBlock();
+  bus_ = NULL;
+}
+
+// static
+bool PowerSaveBlocker::Delegate::DPMSEnabled() {
+  Display* display = base::MessagePumpForUI::GetDefaultXDisplay();
+  BOOL enabled = false;
+  int dummy;
+  if (DPMSQueryExtension(display, &dummy, &dummy) && DPMSCapable(display)) {
+    CARD16 state;
+    DPMSInfo(display, &state, &enabled);
+  }
+  return enabled;
+}
+
+// static
+PowerSaveBlocker::Delegate::DBusAPI PowerSaveBlocker::Delegate::SelectAPI() {
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  switch (base::nix::GetDesktopEnvironment(env.get())) {
+    case base::nix::DESKTOP_ENVIRONMENT_GNOME:
+      if (DPMSEnabled())
+        return kGnomeAPI;
+      break;
+    case base::nix::DESKTOP_ENVIRONMENT_XFCE:
+    case base::nix::DESKTOP_ENVIRONMENT_KDE4:
+      if (DPMSEnabled())
+        return kFreeDesktopAPI;
+      break;
+    case base::nix::DESKTOP_ENVIRONMENT_KDE3:
+    case base::nix::DESKTOP_ENVIRONMENT_OTHER:
+      // Not supported.
+      break;
+  }
+  return kNoAPI;
+}
+
+PowerSaveBlocker::PowerSaveBlocker(
+    PowerSaveBlockerType type, const std::string& reason)
+    : delegate_(new Delegate(type, reason)) {
+  // We use the FILE thread to service the DBus connection, since we need a
+  // thread that allows I/O operations. The same thread must be used in the
+  // destructor below, but other than that we could use any thread.
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,

[satorux1, 2012/06/08 14:59:41]

FILE thread is now deprecated. Please use BrowserThread::PostBlockingPoolTask()

[Mike Mammarella, 2012/06/08 18:33:36]

Hmm. As the comment indicates, I need the same thread to be used below in the
destructor. If I use PostBlockingPoolSequencedTask() with some random string I
make up, will that happen? It's not clear that the same thread will run the
tasks, only that they'll run in sequence.

[satorux1, 2012/06/08 19:31:50]

Why do they need to run on the same thread? As long as it's run in sequence, I
thought it'd be ok.

[Mike Mammarella, 2012/06/08 19:50:17]

They need to be the same because the D-Bus library asserts it. :)

This thread becomes both the origin and D-Bus thread.

[satorux1, 2012/06/08 20:00:14]

Oh, I that explains. You might want to put it as a comment here, as it was
rather subtle.

[Mike Mammarella, 2012/06/08 20:28:46]

Done.

+                          base::Bind(&Delegate::ApplyBlock, delegate_));
+}
+
+PowerSaveBlocker::~PowerSaveBlocker() {
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&Delegate::RemoveBlock, delegate_));
+}
+
+}  // namespace content