Process SelectionRequestor::PerformBlockingConvertSelection() requests one at a

time. In particular, do not call XConvertSelection() till SelectionNotify is
received for the current request (or the current request has timed out).

This is part #1 of implementing support for receiving selection data in multiple
chunks via the INCR property. In particular, when data is transmitted via the
INCR property, the transmission is not complete when the SelectionNotify event
is received.

BUG=367549
TEST=None

Review URL: https://codereview.chromium.org/391593002

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

ui/base/clipboard/clipboard_aurax11.cc

@@ -241,9 +241,6 @@ class Clipboard::AuraX11Details : public PlatformEventDispatcher {
   // CLIPBOARD_TYPE_COPY_PASTE.
   ::Atom GetCopyPasteSelection() const;
 
-  // Returns the object which is responsible for communication on |type|.
-  SelectionRequestor* GetSelectionRequestorForClipboardType(ClipboardType type);
-
   // Finds the SelectionFormatMap for the incoming selection atom.
   const SelectionFormatMap& LookupStorageForAtom(::Atom atom);
 
@@ -305,10 +302,8 @@ class Clipboard::AuraX11Details : public PlatformEventDispatcher {
 
   X11AtomCache atom_cache_;
 
-  // Objects which request and receive selection data.
-  SelectionRequestor clipboard_requestor_;
-  SelectionRequestor primary_requestor_;
-  SelectionRequestor clipboard_manager_requestor_;
+  // Object which requests and receives selection data.
+  SelectionRequestor selection_requestor_;
 
   // Temporary target map that we write to during DispatchObects.
   SelectionFormatMap clipboard_data_;
@@ -333,12 +328,7 @@ Clipboard::AuraX11Details::AuraX11Details()
           0,
           NULL)),
       atom_cache_(x_display_, kAtomsToCache),
-      clipboard_requestor_(x_display_, x_window_,
-                           atom_cache_.GetAtom(kClipboard), this),
-      primary_requestor_(x_display_, x_window_, XA_PRIMARY, this),
-      clipboard_manager_requestor_(x_display_, x_window_,
-                                   atom_cache_.GetAtom(kClipboardManager),
-                                   this),
+      selection_requestor_(x_display_, x_window_, this),
       clipboard_owner_(x_display_, x_window_, atom_cache_.GetAtom(kClipboard)),
       primary_owner_(x_display_, x_window_, XA_PRIMARY) {
   // We don't know all possible MIME types at compile time.
@@ -379,15 +369,6 @@ const SelectionFormatMap& Clipboard::AuraX11Details::LookupStorageForAtom(
   return clipboard_owner_.selection_format_map();
 }
 
-ui::SelectionRequestor*
-Clipboard::AuraX11Details::GetSelectionRequestorForClipboardType(
-    ClipboardType type) {
-  if (type == CLIPBOARD_TYPE_COPY_PASTE)
-    return &clipboard_requestor_;
-  else
-    return &primary_requestor_;
-}
-
 void Clipboard::AuraX11Details::CreateNewClipboardData() {
   clipboard_data_ = SelectionFormatMap();
 }
@@ -423,11 +404,12 @@ SelectionData Clipboard::AuraX11Details::RequestAndWaitForTypes(
     }
   } else {
     TargetList targets = WaitAndGetTargetsList(type);
-    SelectionRequestor* receiver = GetSelectionRequestorForClipboardType(type);
 
+    ::Atom selection_name = LookupSelectionForClipboardType(type);
     std::vector< ::Atom> intersection;
     ui::GetAtomIntersection(types, targets.target_list(), &intersection);
-    return receiver->RequestAndWaitForTypes(intersection);
+    return selection_requestor_.RequestAndWaitForTypes(selection_name,
+                                                       intersection);
   }
 
   return SelectionData();
@@ -450,11 +432,12 @@ TargetList Clipboard::AuraX11Details::WaitAndGetTargetsList(
     size_t out_data_items = 0;
     ::Atom out_type = None;
 
-    SelectionRequestor* receiver = GetSelectionRequestorForClipboardType(type);
-    if (receiver->PerformBlockingConvertSelection(atom_cache_.GetAtom(kTargets),
-                                                  &data,
-                                                  &out_data_items,
-                                                  &out_type)) {
+    if (selection_requestor_.PerformBlockingConvertSelection(
+            selection_name,
+            atom_cache_.GetAtom(kTargets),
+            &data,
+            &out_data_items,
+            &out_type)) {
       // Some apps return an |out_type| of "TARGETS". (crbug.com/377893)
       if (out_type == XA_ATOM || out_type == atom_cache_.GetAtom(kTargets)) {
         const ::Atom* atom_array =
@@ -473,10 +456,11 @@ TargetList Clipboard::AuraX11Details::WaitAndGetTargetsList(
       for (std::vector< ::Atom>::const_iterator it = types.begin();
            it != types.end(); ++it) {
         ::Atom type = None;
-        if (receiver->PerformBlockingConvertSelection(*it,
-                                                      NULL,
-                                                      NULL,
-                                                      &type) &&
+        if (selection_requestor_.PerformBlockingConvertSelection(selection_name,
+                                                                 *it,
+                                                                 NULL,
+                                                                 NULL,
+                                                                 &type) &&
             type == *it) {
           out.push_back(*it);
         }
@@ -520,8 +504,10 @@ void Clipboard::AuraX11Details::StoreCopyPasteDataAndWait() {
   std::vector<Atom> targets = format_map.GetTypes();
 
   base::TimeTicks start = base::TimeTicks::Now();
-  clipboard_manager_requestor_.PerformBlockingConvertSelectionWithParameter(
-      atom_cache_.GetAtom(kSaveTargets), targets);
+  selection_requestor_.PerformBlockingConvertSelectionWithParameter(
+      atom_cache_.GetAtom(kClipboardManager),
+      atom_cache_.GetAtom(kSaveTargets),
+      targets);
   UMA_HISTOGRAM_TIMES("Clipboard.X11StoreCopyPasteDuration",
                       base::TimeTicks::Now() - start);
 }
@@ -544,13 +530,7 @@ uint32_t Clipboard::AuraX11Details::DispatchEvent(const PlatformEvent& xev) {
       break;
     }
     case SelectionNotify: {
-      ::Atom selection = xev->xselection.selection;
-      if (selection == XA_PRIMARY)
-        primary_requestor_.OnSelectionNotify(*xev);
-      else if (selection == GetCopyPasteSelection())
-        clipboard_requestor_.OnSelectionNotify(*xev);
-      else if (selection == atom_cache_.GetAtom(kClipboardManager))
-        clipboard_manager_requestor_.OnSelectionNotify(*xev);
+      selection_requestor_.OnSelectionNotify(*xev);
       break;
     }
     case SelectionClear: {

ui/base/x/selection_requestor.h

@@ -5,18 +5,14 @@
 #ifndef UI_BASE_X_SELECTION_REQUESTOR_H_
 #define UI_BASE_X_SELECTION_REQUESTOR_H_
 
-#include <X11/Xlib.h>
-
-// Get rid of a macro from Xlib.h that conflicts with Aura's RootWindow class.
-#undef RootWindow
-
-#include <list>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/event_types.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "ui/base/ui_base_export.h"
 #include "ui/gfx/x/x11_atom_cache.h"
 #include "ui/gfx/x/x11_types.h"
@@ -29,74 +25,98 @@ class SelectionData;
 // system.
 //
 // X11 uses a system called "selections" to implement clipboards and drag and
-// drop. This class interprets messages from the statefull selection request
+// drop. This class interprets messages from the stateful selection request
 // API. SelectionRequestor should only deal with the X11 details; it does not
 // implement per-component fast-paths.
 class UI_BASE_EXPORT SelectionRequestor {
  public:
   SelectionRequestor(XDisplay* xdisplay,
                      XID xwindow,
-                     XAtom selection_name,
                      PlatformEventDispatcher* dispatcher);
   ~SelectionRequestor();
 
-  // Does the work of requesting |target| from the selection we handle,
-  // spinning up the nested message loop, and reading the resulting data
-  // back. The result is stored in |out_data|.
+  // Does the work of requesting |target| from |selection|, spinning up the
+  // nested message loop, and reading the resulting data back. The result is
+  // stored in |out_data|.
   // |out_data_items| is the length of |out_data| in |out_type| items.
   bool PerformBlockingConvertSelection(
+      XAtom selection,
       XAtom target,
       scoped_refptr<base::RefCountedMemory>* out_data,
       size_t* out_data_items,
       XAtom* out_type);
 
-  // Requests |target| from the selection that we handle, passing |parameter|
-  // as a parameter to XConvertSelection().
+  // Requests |target| from |selection|, passing |parameter| as a parameter to
+  // XConvertSelection().
   void PerformBlockingConvertSelectionWithParameter(
+      XAtom selection,
       XAtom target,
       const std::vector<XAtom>& parameter);
 
-  // Returns the first of |types| offered by the current selection holder, or
-  // returns NULL if none of those types are available.
-  SelectionData RequestAndWaitForTypes(const std::vector<XAtom>& types);
+  // Returns the first of |types| offered by the current owner of |selection|.
+  // Returns an empty SelectionData object if none of |types| are available.
+  SelectionData RequestAndWaitForTypes(XAtom selection,
+                                       const std::vector<XAtom>& types);
 
   // It is our owner's responsibility to plumb X11 SelectionNotify events on
   // |xwindow_| to us.
   void OnSelectionNotify(const XEvent& event);
 
  private:
-  // A request that has been issued and we are waiting for a response to.
-  struct PendingRequest {
-    explicit PendingRequest(XAtom target);
-    ~PendingRequest();
+  friend class SelectionRequestorTest;
 
-    // Data to the current XConvertSelection request. Used for error detection;
-    // we verify it on the return message.
+  // A request that has been issued.
+  struct Request {
+    Request(XAtom selection, XAtom target, base::TimeTicks timeout);
+    ~Request();
+
+    // The target and selection requested in the XConvertSelection() request.
+    // Used for error detection.
+    XAtom selection;
     XAtom target;
 
+    // The result data for the XConvertSelection() request.
+    scoped_refptr<base::RefCountedMemory> out_data;
+    size_t out_data_items;
+    XAtom out_type;
+
+    // Whether the XConvertSelection() request was successful.
+    bool success;
+
+    // The time when the request should be aborted.
+    base::TimeTicks timeout;
+
     // Called to terminate the nested message loop.
     base::Closure quit_closure;
 
-    // The property in the returning SelectNotify message is used to signal
-    // success. If None, our request failed somehow. If equal to the property
-    // atom that we sent in the XConvertSelection call, we can read that
-    // property on |x_window_| for the requested data.
-    XAtom returned_property;
-
-    // Set to true when return_property is populated.
-    bool returned;
+    // True if the request is complete.
+    bool completed;
   };
 
+  // Aborts requests which have timed out.
+  void AbortStaleRequests();
+
+  // Mark |request| as completed. If the current request is completed, converts
+  // the selection for the next request.
+  void CompleteRequest(size_t index);
+
+  // Converts the selection for the request at |current_request_index_|.
+  void ConvertSelectionForCurrentRequest();
+
   // Blocks till SelectionNotify is received for the target specified in
   // |request|.
-  void BlockTillSelectionNotifyForRequest(PendingRequest* request);
+  void BlockTillSelectionNotifyForRequest(Request* request);
+
+  // Returns the request at |current_request_index_| or NULL if there isn't any.
+  Request* GetCurrentRequest();
 
   // Our X11 state.
   XDisplay* x_display_;
   XID x_window_;
 
-  // The X11 selection that this instance communicates on.
-  XAtom selection_name_;
+  // The property on |x_window_| set by the selection owner with the value of
+  // the selection.
+  XAtom x_property_;
 
   // Dispatcher which handles SelectionNotify and SelectionRequest for
   // |selection_name_|. PerformBlockingConvertSelection() calls the
@@ -105,8 +125,18 @@ class UI_BASE_EXPORT SelectionRequestor {
   // Not owned.
   PlatformEventDispatcher* dispatcher_;
 
-  // A list of requests for which we are waiting for responses.
-  std::list<PendingRequest*> pending_requests_;
+  // In progress requests. Requests are added to the list at the start of
+  // PerformBlockingConvertSelection() and are removed and destroyed right
+  // before the method terminates.
+  std::vector<Request*> requests_;
+
+  // The index of the currently active request in |requests_|. The active
+  // request is the request for which XConvertSelection() has been
+  // called and for which we are waiting for a SelectionNotify response.
+  size_t current_request_index_;
+
+  // Used to abort requests if the selection owner takes too long to respond.
+  base::RepeatingTimer<SelectionRequestor> abort_timer_;
 
   X11AtomCache atom_cache_;
 

ui/base/x/selection_requestor_unittest.cc

+// Copyright 2014 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 "ui/base/x/selection_requestor.h"
+
+#include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/x/selection_utils.h"
+#include "ui/base/x/x11_util.h"
+#include "ui/events/platform/platform_event_source.h"
+#include "ui/gfx/x/x11_atom_cache.h"
+#include "ui/gfx/x/x11_types.h"
+
+#include <X11/Xlib.h>
+
+namespace ui {
+
+namespace {
+
+const char* kAtomsToCache[] = {
+    "STRING",
+    NULL
+};
+
+}  // namespace
+
+class SelectionRequestorTest : public testing::Test {
+ public:
+  SelectionRequestorTest()
+      : x_display_(gfx::GetXDisplay()),
+        x_window_(None),
+        atom_cache_(gfx::GetXDisplay(), kAtomsToCache) {
+    atom_cache_.allow_uncached_atoms();
+  }
+
+  virtual ~SelectionRequestorTest() {
+  }
+
+  // Responds to the SelectionRequestor's XConvertSelection() request by
+  // - Setting the property passed into the XConvertSelection() request to
+  //   |value|.
+  // - Sending a SelectionNotify event.
+  void SendSelectionNotify(XAtom selection,
+                           XAtom target,
+                           const std::string& value) {
+    ui::SetStringProperty(x_window_,
+                          requestor_->x_property_,
+                          atom_cache_.GetAtom("STRING"),
+                          value);
+
+    XEvent xev;
+    xev.type = SelectionNotify;
+    xev.xselection.serial = 0u;
+    xev.xselection.display = x_display_;
+    xev.xselection.requestor = x_window_;
+    xev.xselection.selection = selection;
+    xev.xselection.target = target;
+    xev.xselection.property = requestor_->x_property_;
+    xev.xselection.time = CurrentTime;
+    xev.xselection.type = SelectionNotify;
+    requestor_->OnSelectionNotify(xev);
+  }
+
+ protected:
+  virtual void SetUp() OVERRIDE {
+    // Make X11 synchronous for our display connection.
+    XSynchronize(x_display_, True);
+
+    // Create a window for the selection requestor to use.
+    x_window_ = XCreateWindow(x_display_,
+                              DefaultRootWindow(x_display_),
+                              0, 0, 10, 10,    // x, y, width, height
+                              0,               // border width
+                              CopyFromParent,  // depth
+                              InputOnly,
+                              CopyFromParent,  // visual
+                              0,
+                              NULL);
+
+    event_source_ = ui::PlatformEventSource::CreateDefault();
+    CHECK(ui::PlatformEventSource::GetInstance());
+    requestor_.reset(new SelectionRequestor(x_display_, x_window_, NULL));
+  }
+
+  virtual void TearDown() OVERRIDE {
+    requestor_.reset();
+    event_source_.reset();
+    XDestroyWindow(x_display_, x_window_);
+    XSynchronize(x_display_, False);
+  }
+
+  Display* x_display_;
+
+  // |requestor_|'s window.
+  XID x_window_;
+
+  scoped_ptr<ui::PlatformEventSource> event_source_;
+  scoped_ptr<SelectionRequestor> requestor_;
+
+  base::MessageLoopForUI message_loop_;
+  X11AtomCache atom_cache_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SelectionRequestorTest);
+};
+
+namespace {
+
+// Converts |selection| to |target| and checks the returned values.
+void PerformBlockingConvertSelection(SelectionRequestor* requestor,
+                                     X11AtomCache* atom_cache,
+                                     XAtom selection,
+                                     XAtom target,
+                                     const std::string& expected_data) {
+  scoped_refptr<base::RefCountedMemory> out_data;
+  size_t out_data_items = 0u;
+  XAtom out_type = None;
+  EXPECT_TRUE(requestor->PerformBlockingConvertSelection(
+      selection, target, &out_data, &out_data_items, &out_type));
+  EXPECT_EQ(expected_data, ui::RefCountedMemoryToString(out_data));
+  EXPECT_EQ(expected_data.size(), out_data_items);
+  EXPECT_EQ(atom_cache->GetAtom("STRING"), out_type);
+}
+
+}  // namespace
+
+// Test that SelectionRequestor correctly handles receiving a request while it
+// is processing another request.
+TEST_F(SelectionRequestorTest, NestedRequests) {
+  // Assume that |selection| will have no owner. If there is an owner, the owner
+  // will set the property passed into the XConvertSelection() request which is
+  // undesirable.
+  XAtom selection = atom_cache_.GetAtom("FAKE_SELECTION");
+
+  XAtom target1 = atom_cache_.GetAtom("TARGET1");
+  XAtom target2 = atom_cache_.GetAtom("TARGET2");
+
+  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+  loop->PostTask(FROM_HERE,
+                 base::Bind(&PerformBlockingConvertSelection,
+                            base::Unretained(requestor_.get()),
+                            base::Unretained(&atom_cache_),
+                            selection,
+                            target2,
+                            "Data2"));
+  loop->PostTask(FROM_HERE,
+                 base::Bind(&SelectionRequestorTest::SendSelectionNotify,
+                            base::Unretained(this),
+                            selection,
+                            target1,
+                            "Data1"));
+  loop->PostTask(FROM_HERE,
+                 base::Bind(&SelectionRequestorTest::SendSelectionNotify,
+                            base::Unretained(this),
+                            selection,
+                            target2,
+                            "Data2"));
+  PerformBlockingConvertSelection(
+      requestor_.get(), &atom_cache_, selection, target1, "Data1");
+}
+
+}  // namespace ui

ui/ui_unittests.gyp

@@ -74,6 +74,7 @@
         'base/text/bytes_formatting_unittest.cc',
         'base/view_prop_unittest.cc',
         'base/webui/web_ui_util_unittest.cc',
+        'base/x/selection_requestor_unittest.cc',
         'gfx/canvas_unittest_mac.mm',
         'gfx/platform_font_mac_unittest.mm',
       ],
@@ -216,6 +217,7 @@
           ],
           'sources!': [
             'base/dragdrop/os_exchange_data_provider_aurax11_unittest.cc',
+            'base/x/selection_requestor_unittest.cc',
           ],
         }],
         ['chromeos==0 or use_x11==0', {