Process SelectionRequestor::PerformBlockingConvertSelection() requests one at a time

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285729

[pkotwicz, 2014-07-13 23:08:48]

Elliot, can you please take a look?

I cannot think of a way which would process one request at a time and block
other requests till the currently running request finishes.

The INCR property is described here.
http://tronche.com/gui/x/icccm/sec-2.html#s-2.7.2

[erg, 2014-07-14 17:30:58]

lgtm

[pkotwicz, 2014-07-14 23:34:25]

derat@ for OWNERS

[Daniel Erat, 2014-07-15 01:04:09]

https://codereview.chromium.org/391593002/diff/20001/ui/base/x/selection_requ...
File ui/base/x/selection_requestor.cc (right):

https://codereview.chromium.org/391593002/diff/20001/ui/base/x/selection_requ...
ui/base/x/selection_requestor.cc:66: property_pool_.pop_back();
with the disclaimer that i don't know much about selections, and i didn't know
anything about INCR until just now: what's the reason for having multiple atoms
here? it looks like you pop the back one off, use it synchronously, and then put
it back at the end of the list. if i'm not misreading the code, won't this
result in both this method and PerformBlockingConvertSelectionWithParameter()
always only using the last atom? if so, what's the reason for having multiple
atoms?

[pkotwicz, 2014-07-15 02:07:24]

https://codereview.chromium.org/391593002/diff/20001/ui/base/x/selection_requ...
File ui/base/x/selection_requestor.cc (right):

https://codereview.chromium.org/391593002/diff/20001/ui/base/x/selection_requ...
ui/base/x/selection_requestor.cc:66: property_pool_.pop_back();
The problem is that there are callers of clipboard functions from posted tasks.
So a second PerformBlockingConvertSelection() call can be made before the first
one completes.
This is the case with chrome.bookmarkManagerPrivate.canPaste

[Daniel Erat, 2014-07-15 03:32:01]

lgtm

https://codereview.chromium.org/391593002/diff/20001/ui/base/x/selection_requ...
File ui/base/x/selection_requestor.cc (right):

https://codereview.chromium.org/391593002/diff/20001/ui/base/x/selection_requ...
ui/base/x/selection_requestor.cc:66: property_pool_.pop_back();
On 2014/07/15 02:07:24, pkotwicz wrote:
> The problem is that there are callers of clipboard functions from posted
tasks.
> So a second PerformBlockingConvertSelection() call can be made before the
first
> one completes.
> This is the case with chrome.bookmarkManagerPrivate.canPaste

oh. is the nested message loop in BlockTillSelectionNotifyForRequest() where the
second call can get run?

[pkotwicz, 2014-07-16 03:29:45]

derat@, can you please take another look? I rewrote the CL completely.

I rewrote the CL because I realized that it was possible to have nested loops
nested arbitrarily deep (i.e. it was possible to have nested loops 5 (or more)
deep). The code I had previously seemed like it was begging for someone to break
the 5 deep assumption.
I managed to get nested loops which were pretty deep by having several
extensions repeatedly call document.execCommand("paste") on their background
page.

I have added a test for the nested loop behavior. It's a start. In an ideal
world, this class would have more tests

[Daniel Erat, 2014-07-16 21:23:12]

there's going to be another version of this after
https://codereview.chromium.org/392153002/ goes in, right?

[pkotwicz, 2014-07-16 21:59:48]

I have rebased this CL against https://codereview.chromium.org/392153002/

[Daniel Erat, 2014-07-16 22:17:01]

generally lgtm with the disclaimer that erg@ should do another review since i'm
not very familiar with this code :-P

https://codereview.chromium.org/391593002/diff/200001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/391593002/diff/200001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_aurax11.cc:408: ::Atom selection_name =
LookupSelectionForClipboardType(type);
doesn't need to happen in this change, but these can all be changed to XAtom
now, right?

https://codereview.chromium.org/391593002/diff/200001/ui/base/x/selection_req...
File ui/base/x/selection_requestor.h (right):

https://codereview.chromium.org/391593002/diff/200001/ui/base/x/selection_req...
ui/base/x/selection_requestor.h:130: // which initiated the request has not yet
returned.
could you add a comment describing the ownership of the Request objects?

https://codereview.chromium.org/391593002/diff/200001/ui/base/x/selection_req...
File ui/base/x/selection_requestor_unittest.cc (right):

https://codereview.chromium.org/391593002/diff/200001/ui/base/x/selection_req...
ui/base/x/selection_requestor_unittest.cc:32: SelectionRequestorTest() :
x_display_(gfx::GetXDisplay()),
nit: ':' of constructor list should be on the next line and indented four spaces

https://codereview.chromium.org/391593002/diff/200001/ui/base/x/selection_req...
ui/base/x/selection_requestor_unittest.cc:66: SelectionRequestor* requestor() {
nit: make the member protected so you don't need an accessor?

https://codereview.chromium.org/391593002/diff/200001/ui/base/x/selection_req...
ui/base/x/selection_requestor_unittest.cc:70: X11AtomCache* atom_cache() {
nit: make the member protected so you don't need an accessor?

[erg, 2014-07-16 22:39:27]

lgtm

(I think I've convinced myself that your timing for
ConvertSelectionForCurrentRequest() queuing is correct.)

[pkotwicz, 2014-07-17 19:38:34]

dcheng@ for ui/base/clipboard OWNERS

https://codereview.chromium.org/391593002/diff/200001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/391593002/diff/200001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_aurax11.cc:408: ::Atom selection_name =
LookupSelectionForClipboardType(type);
On 2014/07/16 22:17:00, Daniel Erat wrote:
> doesn't need to happen in this change, but these can all be changed to XAtom
> now, right?

Acknowledged.

[pkotwicz, 2014-07-20 21:14:55]

dcheng@ PING?

[dcheng, 2014-07-25 18:05:59]

lgtm. just one small comment.

https://codereview.chromium.org/391593002/diff/240001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/391593002/diff/240001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_aurax11.cc:532: case SelectionNotify:
Nit: { } for consistency with other case blocks

[pkotwicz, 2014-07-25 21:39:48]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-25 21:41:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/391593002/280001

[commit-bot: I haz the power, 2014-07-25 23:41:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

[commit-bot: I haz the power, 2014-07-25 23:59:46]

Change committed as 285729