Implement clipboard for Chromoting Linux hosts.

remoting/host/linux/x_server_clipboard.h

+// 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.
+
+// Don't include this file in any .h files because it pulls in some X headers.

[Wez, 2012/09/17 22:13:29]

nit: in -> from?

[Lambros, 2012/09/19 23:58:19]

Done.

+
+#ifndef REMOTING_HOST_LINUX_X_SERVER_CLIPBOARD_H_
+#define REMOTING_HOST_LINUX_X_SERVER_CLIPBOARD_H_
+
+#include <X11/Xatom.h>
+#include <X11/Xlib.h>
+
+#include <map>
+#include <set>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback_forward.h"
+#include "base/timer.h"
+
+namespace remoting {
+
+// A class to allow manipulation of the X clipboard, using low-level X

[Wez, 2012/09/17 22:13:29]

nit: low-level -> only

[Lambros, 2012/09/19 23:58:19]

Done.

+// API calls.
+class XServerClipboard {
+ public:
+  typedef base::Callback<void(const std::string& mime_type,
+                              const std::string& data)>
+    ClipboardChangedCallback;
+
+  XServerClipboard();
+  ~XServerClipboard();
+
+  // The caller should ensure |display| remains valid as long as any other

[Wez, 2012/09/17 22:13:29]

nit: should -> must?

[Lambros, 2012/09/19 23:58:19]

Done.

+  // methods are called on this object.

[Wez, 2012/09/17 22:13:29]

nit: Suggest reword: ... |display| is still valid whenever any of this object's
APIs are invoked.

Or alternatively just require that Display* out-live this object?

[Lambros, 2012/09/19 23:58:19]

Done.

+  void Init(Display* display, const ClipboardChangedCallback& callback);
+
+  void SetClipboard(const std::string& mime_type, const std::string& data);

[Wez, 2012/09/17 22:13:29]

nit: Add a sentence to describe this API.

[Lambros, 2012/09/19 23:58:19]

Done.

+
+  // This method allows this object to be hooked up to the program's main event
+  // loop.  It should be called when the event loop receives an X event, and
+  // all methods of this object should be called on this thread.
+  void ProcessXEvent(XEvent* event);

[Wez, 2012/09/17 22:13:29]

nit: Reword: Invoked by the caller for each X11 event.

I don't understand the "and all methods..." bit; do you mean that the call needs
to be made on the same thread the event was received on?

[Lambros, 2012/09/19 23:58:19]

Not necessarily. But then the caller needs to do synchronization if it's going
to call XPending/XNextEvent on a different thread from the thread that calls
methods of this class, which also call into Xlib.  I've changed the wording,
ptal.

+
+ private:
+  // This checks the state of the X selections.  If there is new text selected

[Wez, 2012/09/17 22:13:29]

nit: Suggest reword:

"Checks the state of the X PRIMARY and CLIPBOARD selections and queues a
notification to the registered ClipboardChangedCallback if the content has
changed since the previous call."

I don't understand why PRIMARY is tried first, if SECONDARY is used if it's
newer; doesn't that just mean you use whichever is newer?

Why filter out duplicates? We already have echo filtering elsewhere in the
clipboard pipeline, and filtering here will lead to un-intuitive behaviour.

+  // by another application, this will eventually trigger the registered
+  // ClipboardChangedCallback.  Of the two selections (PRIMARY and CLIPBOARD),
+  // the PRIMARY selection is tried first, and the CLIPBOARD selection is only
+  // used if it is newer than PRIMARY.  If there is no new text since the
+  // previous call, then no callback will be made.  Duplicate text is filtered
+  // out, so that the same text being selected again will not trigger the
+  // callback.
+  // The Init() method calls this to get the initial state of the X selections;
+  // no callback will be triggered in this case.

[Wez, 2012/09/17 22:13:29]

nit: I don't think you need to document this here; mention it at the call-site
instead.

+  // If |selection| is CLIPBOARD or PRIMARY, it is checked.  If |selection| is
+  // None, both CLIPBOARD and PRIMARY are checked.  Other values of |selection|
+  // are ignored.

[Wez, 2012/09/17 22:13:29]

nit: Why ignore other values rather than treating them as invalid?

[Wez, 2012/09/17 22:13:29]

nit: Suggest reword "|selection| specifies the selection to be checked, or None
to check both the CLIPBOARD and PRIMARY selections."

[Lambros, 2012/09/19 23:58:19]

We ignore them because we might conceivably get them from XFixes, if an app were
to claim ownership of a new X clipboard type. I suppose we could remote the new
clipboard, but probably best not to, for now at least :)

+  // If |timestamp| is non-zero, it is recorded as the "current selection time",
+  // and is treated as new.

[Wez, 2012/09/17 22:13:29]

nit: This doesn't really explain what |timestamp| means; is it used to indicate
when we most recently checked the clipboard, for example? Will it ever become
zero after having been non-zero?

[Lambros, 2012/09/19 23:58:19]

I've expanded the wording a bit.

+  void GetSelections(Atom selection, Time timestamp);

[Wez, 2012/09/17 22:13:29]

Looks like this is the handler for XFixes' SetSelectionOwner events, so name it
OnSetSelectionOwner?

[Lambros, 2012/09/19 23:58:19]

It's also called during init (although arguably it shouldn't be, as we shouldn't
be getting the initial clipboard selection for Chromoting - I'll take a look at
this).

[Wez, 2012/09/20 23:37:46]

OK, but can we re-name it to OnSetSelectionOwner, or similar, since it's really
the XFixes event handler, or ProcessSelection, say, since it processes the
specified selection?

+
+  void FinishGetSelections();

[Wez, 2012/09/17 22:13:29]

nit: Add a comment explaining what this is for.

[Lambros, 2012/09/19 23:58:19]

Done.

+
+  void OnPropertyNotify(XEvent* event);
+  void OnSelectionNotify(XEvent* event);
+  void OnSelectionRequest(XEvent* event);
+
+  // This is called when the selection owner has replied to a request for
+  // information about a selection.

[Wez, 2012/09/17 22:13:29]

nit: Reword: "Called when the ..."

[Lambros, 2012/09/19 23:58:19]

Done.

+  void DoSelectionNotify(XSelectionEvent* event,
+                         Atom type,
+                         int format,
+                         int item_count,
+                         void* data);
+
+  bool GotSelectionTimestamp(XSelectionEvent* event,
+                             int format,
+                             int item_count,
+                             void* data);
+  bool GotSelectionTargets(XSelectionEvent* event,
+                           int format,
+                           int item_count,
+                           void* data);
+  bool GotSelectionString(XSelectionEvent* event,
+                          int format,
+                          int item_count,
+                          void* data);
+  void GotCutTextUtf8(const std::string& text);
+  bool IsSelectionOwner(Atom selection);
+  void GetSelectionTimestamp(Atom selection, Time time);
+  void GetSelectionTargets(Atom selection);
+  void GetSelectionString(Atom selection, Atom target);
+
+  // Request that the window owns the given selection from the given time (the

[Wez, 2012/09/17 22:13:29]

nit: Which window?

[Lambros, 2012/09/19 23:58:19]

Done.

+  // time should be taken from an X event).

[Wez, 2012/09/17 22:13:29]

nit: Suggest reword: "Assert ownership of the specified |selection| at the
specified |time|."

Why should time be taken from an X event?  What X event would it be taken from? 
Shouldn't time be "now", or thereabouts?

[Lambros, 2012/09/19 23:58:19]

Given that we only ever pass CurrentTime in, I've removed this parameter.

+  void OwnSelection(Atom selection, Time time);
+
+  Display* display_;
+  Window clipboard_window_;
+  bool have_xfixes_;
+  int xfixes_event_base_;
+  int xfixes_error_base_;
+  Atom clipboard_atom_;
+  Atom large_selection_atom_;
+  Atom selection_string_atom_;
+  Atom targets_atom_;
+  Atom timestamp_atom_;
+  Atom utf8_string_atom_;
+  std::set<Atom> selections_owned_;
+  std::map<Atom, Time> selection_own_time_;
+  std::string data_;
+  Atom large_selection_property_;
+  Time current_selection_time_;
+  Atom new_selection_;
+  bool getting_initial_selection_;
+  bool new_cut_text_;
+  bool had_valid_timestamp_;
+  base::TimeTicks get_selections_time_;
+  ClipboardChangedCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(XServerClipboard);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_LINUX_X_SERVER_CLIPBOARD_H_