Enable copying long text to Chrome and pasting long text from Chrome

Enable copying long text to Chrome and pasting long text from Chrome

This CL adds support for the INCR property
(http://tronche.com/gui/x/icccm/sec-2.html#s-2.7.2) to SelectionRequestor and
SelectionOwner

BUG=367549
TEST=Manual, see bug
R=derat,dcheng,erg
TBR=erg for ui/base/BUILD.gn

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

[pkotwicz, 2014-07-17 15:51:48]

Elliot, can you please take a look?

[Elliot Glaysher, 2014-07-17 22:32:46]

lgtm

https://codereview.chromium.org/397223002/diff/80001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_aurax11.cc:517: return true;
I don't like that we're moving away from a mapping of one object to one X11
window, but I can't think of any other way to accomplish what you're doing
here...

[pkotwicz, 2014-07-25 18:33:13]

derat@ for ui/base/x OWNERS
dcheng@ for ui/base/clipboard OWNERS

[Daniel Erat, 2014-07-25 19:38:44]

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.cc:37: const int kIncrTimerPeriodMs = 1000;
s/Incr/Incremental/ here and in other constants

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.cc:49: (extended_max_size ? extended_max_size :
XMaxRequestSize(display)) - 100);
nit: check for negative wraparound

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owner.h
File ui/base/x/selection_owner.h (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:60: struct IncrTransfer {
nit: rename to IncrementalTransfer

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:98: void ProcessIncr(IncrTransfer* transfer);
nit: ProcessIncrementalTransfer

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:101: void AbortStaleIncrTransfers();
AbortStaleIncrementalTransfers

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:104: void
CompleteIncrTransfer(std::vector<IncrTransfer>::iterator it);
CompleteIncrementalTransfer

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:108: std::vector<IncrTransfer>::iterator
FindIncrTransferForEvent(
etc.

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:124: std::vector<IncrTransfer> incrs_;
incremental_transfers_

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.h:127: base::RepeatingTimer<SelectionOwner>
incr_timer_;
incremental_timer_

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

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_requ...
ui/base/x/selection_requestor.cc:31: // kRequestTimeoutMs.
add a compile-time assert of this constraint?

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
File ui/base/x/x11_foreign_window_manager.cc (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
ui/base/x/x11_foreign_window_manager.cc:55: if (it == request_map_.end())
i think there's a bug here. when removing the last request for an xid,
CancelRequest() deletes the xid's entry from |request_map_| before calling this
method. this method will then bail out here without unselecting the events,
won't it?

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
ui/base/x/x11_foreign_window_manager.cc:63: XSelectInput(gfx::GetXDisplay(),
xid, event_mask);
do we crash in response to X errors? i'm wondering if you need to trap errors
generated by this call in case the window is destroyed before your request makes
it to the X server.

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
File ui/base/x/x11_foreign_window_manager.h (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
ui/base/x/x11_foreign_window_manager.h:60: typedef std::vector<Request>
RequestList;
nit: RequestVector or just Requests so it doesn't seem like it's an std::list

[dcheng, 2014-07-25 23:48:30]

ui/base/clipboard/* LGTM with one nit.

https://codereview.chromium.org/397223002/diff/80001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_aurax11.cc:554: case PropertyNotify:
{}s to be consistent =)

[pkotwicz, 2014-07-26 20:09:54]

derat@, can you please take another look?
I think I have addressed all of your concerns. I have also fixed a couple of
bugs that I found.

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_owne...
ui/base/x/selection_owner.cc:49: (extended_max_size ? extended_max_size :
XMaxRequestSize(display)) - 100);
On 2014/07/25 19:38:43, Daniel Erat wrote:
> nit: check for negative wraparound

Done.

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

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/selection_requ...
ui/base/x/selection_requestor.cc:31: // kRequestTimeoutMs.
On 2014/07/25 19:38:44, Daniel Erat wrote:
> add a compile-time assert of this constraint?

Done.

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
File ui/base/x/x11_foreign_window_manager.cc (right):

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
ui/base/x/x11_foreign_window_manager.cc:55: if (it == request_map_.end())
You're right. Thanks for catching the bug!

https://codereview.chromium.org/397223002/diff/80001/ui/base/x/x11_foreign_wi...
ui/base/x/x11_foreign_window_manager.cc:63: XSelectInput(gfx::GetXDisplay(),
xid, event_mask);
We do not crash. We just log errors via ui::LogErrorEventDescription().

This is a scenario I did not consider, thanks for pointing it out!
I am not adding a gfx::X11ErrorTracker to surpress the logging here. There are a
number of other places where we generate X errors if the X window is deleted.
The call to XChangeProperty() in ui::SelectionOwner::ProcessTarget() is an
example

https://codereview.chromium.org/397223002/diff/190001/ui/base/x/selection_req...
File ui/base/x/selection_requestor.cc (right):

https://codereview.chromium.org/397223002/diff/190001/ui/base/x/selection_req...
ui/base/x/selection_requestor.cc:170: if (request->out_type ==
atom_cache_.GetAtom(kIncr)) {
I now clear request->out_data and request->out_data_items here

https://codereview.chromium.org/397223002/diff/190001/ui/base/x/selection_req...
ui/base/x/selection_requestor.cc:190: if (!request ||
!request->data_sent_incrementally)
I now check request->data_sent_incrementally
This causes us to early return if we have not yet received the SelectionNotify
event.

https://codereview.chromium.org/397223002/diff/190001/ui/base/x/selection_req...
ui/base/x/selection_requestor.cc:244: while (GetCurrentRequest() &&
GetCurrentRequest()->completed)
It is now possible for the timeouts not to be in increasing order from the
beginning to the end of the array because we update the timeout in
SelectionRequestor::OnPropertyEvent()

I now correctly handle aborting a request which is not the current request.

Being able to abort a request which is not the current request is confusing.
However, setting the timeout upon calling
SelectionRequestor::PerformBlockingConvertSelection() puts an upper bound on the
amount of time that the UI thread can be blocked due to queued clipboard
requests.

[Daniel Erat, 2014-07-28 14:33:20]

lgtm

[pkotwicz, 2014-07-31 14:58:13]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-31 14:59:30]

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

[pkotwicz, 2014-07-31 18:16:31]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:18:40]

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

[pkotwicz, 2014-07-31 18:40:17]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:41:20]

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

[commit-bot: I haz the power, 2014-07-31 22:50:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-31 23:31:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-31 23:31:20]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2014-08-04 16:36:03]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-04 16:36:47]

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

[pkotwicz, 2014-08-04 16:47:34]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-08-04 16:48:07]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-04 16:48:34]

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

[commit-bot: I haz the power, 2014-08-04 23:46:33]

Change committed as 287404