Linux clipboard support for Chromoting host.

Submission for readability review.

Original CLs:
http://codereview.chromium.org/10909133
http://codereview.chromium.org/11035049


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

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

3 files changed, 108 insertions, 47 deletions

diff --git a/remoting/host/clipboard_linux.cc b/remoting/host/clipboard_linux.cc
index 1f74689..30b5391 100644
--- a/remoting/host/clipboard_linux.cc
+++ b/remoting/host/clipboard_linux.cc
@@ -40,9 +40,14 @@ class ClipboardLinux : public Clipboard,
 
   scoped_ptr<protocol::ClipboardStub> client_clipboard_;
 
+  // Underlying X11 clipboard implementation.
   XServerClipboard x_server_clipboard_;
+
+  // Connection to the X server, used by |x_server_clipboard_|. This is created
+  // and owned by this class.
   Display* display_;
 
+  // Watcher used to handle X11 events from |display_|.
   MessageLoopForIO::FileDescriptorWatcher x_connection_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(ClipboardLinux);
diff --git a/remoting/host/linux/x_server_clipboard.cc b/remoting/host/linux/x_server_clipboard.cc
index b81f7b2..e63e5a0 100644
--- a/remoting/host/linux/x_server_clipboard.cc
+++ b/remoting/host/linux/x_server_clipboard.cc
@@ -6,6 +6,7 @@
 
 #include <X11/extensions/Xfixes.h>
 
+#include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "remoting/base/constants.h"
@@ -16,7 +17,6 @@ XServerClipboard::XServerClipboard()
     : display_(NULL),
       clipboard_window_(BadValue),
       xfixes_event_base_(-1),
-      xfixes_error_base_(-1),
       clipboard_atom_(None),
       large_selection_atom_(None),
       selection_string_atom_(None),
@@ -42,8 +42,9 @@ void XServerClipboard::Init(Display* display,
   // TODO(lambroslambrou): Consider using ScopedXErrorHandler here, or consider
   // placing responsibility for handling X Errors outside this class, since
   // X Error handlers are global to all X connections.
+  int xfixes_error_base;
   if (!XFixesQueryExtension(display_, &xfixes_event_base_,
-                            &xfixes_error_base_)) {
+                            &xfixes_error_base)) {
     LOG(INFO) << "X server does not support XFixes.";
     return;
   }
@@ -60,23 +61,26 @@ void XServerClipboard::Init(Display* display,
 
   // TODO(lambroslambrou): Use ui::X11AtomCache for this, either by adding a
   // dependency on ui/ or by moving X11AtomCache to base/.
-  const int kNumAtoms = 6;
-  const char* names[kNumAtoms] = {
+  static const char* const kAtomNames[] = {
     "CLIPBOARD",
     "INCR",
     "SELECTION_STRING",
     "TARGETS",
     "TIMESTAMP",
-    "UTF8_STRING" };
-  Atom atoms[kNumAtoms];
-  if (XInternAtoms(display_, const_cast<char**>(names), kNumAtoms, False,
-                   atoms)) {
+    "UTF8_STRING"
+  };
+  static const int kNumAtomNames = arraysize(kAtomNames);
+
+  Atom atoms[kNumAtomNames];
+  if (XInternAtoms(display_, const_cast<char**>(kAtomNames), kNumAtomNames,
+                   False, atoms)) {
     clipboard_atom_ = atoms[0];
     large_selection_atom_ = atoms[1];
     selection_string_atom_ = atoms[2];
     targets_atom_ = atoms[3];
     timestamp_atom_ = atoms[4];
     utf8_string_atom_ = atoms[5];
+    COMPILE_ASSERT(kNumAtomNames >= 6, kAtomNames_too_small);
   } else {
     LOG(ERROR) << "XInternAtoms failed";
   }
@@ -126,7 +130,7 @@ void XServerClipboard::ProcessXEvent(XEvent* event) {
 
   if (event->type == xfixes_event_base_ + XFixesSetSelectionOwnerNotify) {
     XFixesSelectionNotifyEvent* notify_event =
-      reinterpret_cast<XFixesSelectionNotifyEvent*>(event);
+        reinterpret_cast<XFixesSelectionNotifyEvent*>(event);
     OnSetSelectionOwnerNotify(notify_event->selection,
                               notify_event->selection_timestamp);
   }
@@ -140,7 +144,7 @@ void XServerClipboard::OnSetSelectionOwnerNotify(Atom selection,
   // quickly to our requests.
   if (!get_selections_time_.is_null() &&
       (base::TimeTicks::Now() - get_selections_time_) <
-      base::TimeDelta::FromSeconds(5)) {
+          base::TimeDelta::FromSeconds(5)) {
     // TODO(lambroslambrou): Instead of ignoring this notification, cancel any
     // pending request operations and ignore the resulting events, before
     // dispatching new requests here.
@@ -231,40 +235,14 @@ void XServerClipboard::OnSelectionRequest(XEvent* event) {
   } else {
     selection_event.property = event->xselectionrequest.property;
     if (selection_event.target == targets_atom_) {
-      // Respond advertising XA_STRING, UTF8_STRING and TIMESTAMP data for the
-      // selection.
-      Atom targets[3];
-      targets[0] = timestamp_atom_;
-      targets[1] = utf8_string_atom_;
-      targets[2] = XA_STRING;
-      XChangeProperty(display_, selection_event.requestor,
-                      selection_event.property, XA_ATOM, 32, PropModeReplace,
-                      reinterpret_cast<unsigned char*>(targets), 3);
+      SendTargetsResponse(selection_event.requestor, selection_event.property);
     } else if (selection_event.target == timestamp_atom_) {
-      // Respond with the timestamp of our selection; we always return
-      // CurrentTime since our selections are set by remote clients, so there
-      // is no associated local X event.
-
-      // TODO(lambroslambrou): Should use a proper timestamp here instead of
-      // CurrentTime.  ICCCM recommends doing a zero-length property append,
-      // and getting a timestamp from the subsequent PropertyNotify event.
-      Time time = CurrentTime;
-      XChangeProperty(display_, selection_event.requestor,
-                      selection_event.property, XA_INTEGER, 32,
-                      PropModeReplace, reinterpret_cast<unsigned char*>(&time),
-                      1);
+      SendTimestampResponse(selection_event.requestor,
+                            selection_event.property);
     } else if (selection_event.target == utf8_string_atom_ ||
                selection_event.target == XA_STRING) {
-      if (!data_.empty()) {
-        // Return the actual string data; we always return UTF8, regardless of
-        // the configured locale.
-        XChangeProperty(display_, selection_event.requestor,
-                        selection_event.property, selection_event.target, 8,
-                        PropModeReplace,
-                        reinterpret_cast<unsigned char*>(
-                            const_cast<char*>(data_.data())),
-                        data_.size());
-      }
+      SendStringResponse(selection_event.requestor, selection_event.property,
+                         selection_event.target);
     }
   }
   XSendEvent(display_, selection_event.requestor, False, 0,
@@ -275,6 +253,42 @@ void XServerClipboard::OnSelectionClear(XEvent* event) {
   selections_owned_.erase(event->xselectionclear.selection);
 }
 
+void XServerClipboard::SendTargetsResponse(Window requestor, Atom property) {
+  // Respond advertising XA_STRING, UTF8_STRING and TIMESTAMP data for the
+  // selection.
+  Atom targets[3];
+  targets[0] = timestamp_atom_;
+  targets[1] = utf8_string_atom_;
+  targets[2] = XA_STRING;
+  XChangeProperty(display_, requestor, property, XA_ATOM, 32, PropModeReplace,
+                  reinterpret_cast<unsigned char*>(targets), 3);
+}
+
+void XServerClipboard::SendTimestampResponse(Window requestor, Atom property) {
+  // Respond with the timestamp of our selection; we always return
+  // CurrentTime since our selections are set by remote clients, so there
+  // is no associated local X event.
+
+  // TODO(lambroslambrou): Should use a proper timestamp here instead of
+  // CurrentTime.  ICCCM recommends doing a zero-length property append,
+  // and getting a timestamp from the subsequent PropertyNotify event.
+  Time time = CurrentTime;
+  XChangeProperty(display_, requestor, property, XA_INTEGER, 32,
+                  PropModeReplace, reinterpret_cast<unsigned char*>(&time), 1);
+}
+
+void XServerClipboard::SendStringResponse(Window requestor, Atom property,
+                                          Atom target) {
+  if (!data_.empty()) {
+    // Return the actual string data; we always return UTF8, regardless of
+    // the configured locale.
+    XChangeProperty(display_, requestor, property, target, 8, PropModeReplace,
+                    reinterpret_cast<unsigned char*>(
+                        const_cast<char*>(data_.data())),
+                    data_.size());
+  }
+}
+
 void XServerClipboard::HandleSelectionNotify(XSelectionEvent* event,
                                              Atom type,
                                              int format,
diff --git a/remoting/host/linux/x_server_clipboard.h b/remoting/host/linux/x_server_clipboard.h
index 7bf2359..fb2d026 100644
--- a/remoting/host/linux/x_server_clipboard.h
+++ b/remoting/host/linux/x_server_clipboard.h
@@ -24,9 +24,15 @@ namespace remoting {
 // application's main event-processing thread.
 class XServerClipboard {
  public:
+  // Called when new clipboard data has been received from the owner of the X
+  // selection (primary or clipboard).
+  // |mime_type| is the MIME type associated with the data. This will be one of
+  // the types listed in remoting/base/constants.h.
+  // |data| is the clipboard data from the associated X event, encoded with the
+  // specified MIME-type.
   typedef base::Callback<void(const std::string& mime_type,
                               const std::string& data)>
-    ClipboardChangedCallback;
+      ClipboardChangedCallback;
 
   XServerClipboard();
   ~XServerClipboard();
@@ -45,15 +51,26 @@ class XServerClipboard {
   void ProcessXEvent(XEvent* event);
 
  private:
-  // Handlers for X selection events.
+  // Handlers called by ProcessXEvent() for each event type.
   void OnSetSelectionOwnerNotify(Atom selection, Time timestamp);
   void OnPropertyNotify(XEvent* event);
   void OnSelectionNotify(XEvent* event);
   void OnSelectionRequest(XEvent* event);
   void OnSelectionClear(XEvent* event);
 
-  // Called when the selection owner has replied to a request for information
-  // about a selection.
+  // Used by OnSelectionRequest() to respond to requests for details of our
+  // clipboard content. This is done by changing the property |property| of the
+  // |requestor| window (these values come from the XSelectionRequestEvent).
+  // |target| must be a string type (STRING or UTF8_STRING).
+  void SendTargetsResponse(Window requestor, Atom property);
+  void SendTimestampResponse(Window requestor, Atom property);
+  void SendStringResponse(Window requestor, Atom property, Atom target);
+
+  // Called by OnSelectionNotify() when the selection owner has replied to a
+  // request for information about a selection.
+  // |event| is the raw X event from the notification.
+  // |type|, |format| etc are the results from XGetWindowProperty(), or 0 if
+  // there is no associated data.
   void HandleSelectionNotify(XSelectionEvent* event,
                              Atom type,
                              int format,
@@ -61,7 +78,8 @@ class XServerClipboard {
                              void* data);
 
   // These methods return true if selection processing is complete, false
-  // otherwise.
+  // otherwise. They are called from HandleSelectionNotify(), and take the same
+  // arguments.
   bool HandleSelectionTargetsEvent(XSelectionEvent* event,
                                    int format,
                                    int item_count,
@@ -83,20 +101,44 @@ class XServerClipboard {
   void AssertSelectionOwnership(Atom selection);
   bool IsSelectionOwner(Atom selection);
 
+  // Stores the Display* supplied to Init().
   Display* display_;
+
+  // Window through which clipboard events are received, or BadValue if the
+  // window could not be created.
   Window clipboard_window_;
+
+  // The event base returned by XFixesQueryExtension(). If XFixes is
+  // unavailable, the clipboard window will not be created, and no
+  // event-processing will take place.
   int xfixes_event_base_;
-  int xfixes_error_base_;
+
+  // Cached atoms for various strings, initialized during Init().
   Atom clipboard_atom_;
   Atom large_selection_atom_;
   Atom selection_string_atom_;
   Atom targets_atom_;
   Atom timestamp_atom_;
   Atom utf8_string_atom_;
+
+  // The set of X selections owned by |clipboard_window_| (can be Primary or
+  // Clipboard or both).
   std::set<Atom> selections_owned_;
+
+  // Clipboard content to return to other applications when |clipboard_window_|
+  // owns a selection.
   std::string data_;
+
+  // Stores the property to use for large transfers, or None if a large
+  // transfer is not currently in-progress.
   Atom large_selection_property_;
+
+  // Remembers the start time of selection processing, and is set to null when
+  // processing is complete. This is used to decide whether to begin processing
+  // a new selection or continue with the current selection.
   base::TimeTicks get_selections_time_;
+
+  // |callback| argument supplied to Init().
   ClipboardChangedCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(XServerClipboard);