Drag on linux

linux_aura: Make chrome windows drag sources on X11.

This implements the source side of the Xdnd protocol. It also includes some small changes to which object owns the drag selection so that we can do that part entirely asynchronously without spinning up a nested message loop.

This patch only handles text drags and doesn't change the cursor or show a drag image under the cursor. Both of these require some refactoring and this patch is big enough as is.

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

[Elliot Glaysher, 2013-06-12 23:11:28]

OK, we can round trip. Can't drag within chrome yet.

[Elliot Glaysher, 2013-06-14 20:12:05]

Speculative reenabling of omnibox unit tests

[Elliot Glaysher, 2013-06-17 18:20:20]

derat: review

[Daniel Erat, 2013-06-17 22:09:57]

LGTM after some comments and questions are addressed

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
File ui/base/dragdrop/os_exchange_data_provider_aurax11.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
ui/base/dragdrop/os_exchange_data_provider_aurax11.cc:303: case
SelectionRequest: {
nit: don't need curly brackets here

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
ui/base/dragdrop/os_exchange_data_provider_aurax11.cc:307: default: {
or here

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
File ui/base/dragdrop/os_exchange_data_provider_aurax11.h (right):

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
ui/base/dragdrop/os_exchange_data_provider_aurax11.h:35: // |x_window| is the
window the cursor is over, and |targets| are the MIME
nit: update this comment to remove stale mention of |targets|

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
ui/base/dragdrop/os_exchange_data_provider_aurax11.h:47: // this to
nit: fix this comment

https://codereview.chromium.org/16271006/diff/72001/ui/base/dragdrop/os_excha...
ui/base/dragdrop/os_exchange_data_provider_aurax11.h:120: // process, or build
up through a sequence of Set*() calls. It can be passed
nit: s/build/built/

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/selection_owner.cc
File ui/base/x/selection_owner.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/selection_owner...
ui/base/x/selection_owner.cc:41: void
SelectionOwner::RetrieveTargets(std::vector<Atom>* targets) {
nit: either clear |targets| first here or document in the header that this
appends to it

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/selection_utils.cc
File ui/base/x/selection_utils.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/selection_utils...
ui/base/x/selection_utils.cc:121: SelectionFormatMap*
SelectionFormatMap::Clone() const {
return a scoped_ptr instead?

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/x11_util.cc#new...
ui/base/x/x11_util.cc:777: window,
nit: move window and property to the end of the first line

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/x11_util.cc#new...
ui/base/x/x11_util.cc:994: XSync(ui::GetXDisplay(), False);
document that this makes a round trip?

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

https://codereview.chromium.org/16271006/diff/72001/ui/base/x/x11_util.h#newc...
ui/base/x/x11_util.h:167: // Returns the raw bytes from a property with minimal
interpretation.
document ownership of the memory pointed at by |out_data| and how the caller
should free it

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:34: const char
kChromiumDragReciever[] = "CHROMIUM_DRAG_RECEIVER";
nit: prefix vendor-specific atoms with underscores per
http://tronche.com/gui/x/icccm/sec-1.html#s-1.2.3

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:58: class
DragTargetWindowFinder : public ui::EnumerateWindowsDelegate {
please add a comment describing what this class does (e.g. its criteria,
dictated by the icccm i think, for selecting a window)

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:109:
NOTIMPLEMENTED();
this NOTIMPLEMENTED() seems weird to me -- what do you plan to implement
instead?

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:110:
*dest_window = None;
how about clearing dest_window at the beginning of this method so you don't need
to do it in multiple places later?

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:194: // Where
the cursor is on screen
nit: add trailing period

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:239: // The
window doesn't have a DesktopDragDropClientAuraX11, that means its
nit: s/its/it's/

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:252: // for the
data its offering.
nit: s/its/it's/

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:264: //
Unsubscribe to message events.
nit: s/to/from/

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:283: // At this
point, we need to start
fix this comment

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:391:
DCHECK(g_live_client_map.find(xwindow) == g_live_client_map.end());
nit: can you use DCHECK_EQ() here?

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:470: //
TODO(erg): event.data.[2,3] specify a rectangle. It is a request by the
is this really a TODO, or just a note? sounds like you don't plan to do anything
more here

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:737:
drag_operation = ui::DragDropTypes::DRAG_COPY;
nit: remove drag_operation and just return from each of these clauses, with a
final "return DRAG_NONE" at the end?

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:766:
DragDropDelegate* delegate;
nit: initialize to NULL

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:783:
xev.xclient.data.l[1] = (drag_operation != 0) ? (2 | 1) : 0;
nit: document what this means in more detail

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:48:
delegate_->OnMouseReleased();
is it intentional that this is also called for button presses (presumably
referring to other buttons)?

[Elliot Glaysher, 2013-06-18 00:06:39]

sky: owners review

Comments for derat:

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:109:
NOTIMPLEMENTED();
On 2013/06/17 22:09:57, Daniel Erat wrote:
> this NOTIMPLEMENTED() seems weird to me -- what do you plan to implement
> instead?

I wanted a nonfatal log. Deleted.

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:391:
DCHECK(g_live_client_map.find(xwindow) == g_live_client_map.end());
On 2013/06/17 22:09:57, Daniel Erat wrote:
> nit: can you use DCHECK_EQ() here?

You can't use DCHECK_EQ with iterators.

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/16271006/diff/72001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:48:
delegate_->OnMouseReleased();
On 2013/06/17 22:09:57, Daniel Erat wrote:
> is it intentional that this is also called for button presses (presumably
> referring to other buttons)?

Removed ButtonPress. (The historical reasons here have to do with what should
happen when another button is pressed while dragging a tab; I've filled
crbug.com/251040)

[sky, 2013-06-18 14:09:45]

It looks like Dan looked through all of this. Is there any particular part you
needed me to review?

https://codereview.chromium.org/16271006/diff/98001/ui/base/dragdrop/os_excha...
File ui/base/dragdrop/os_exchange_data_provider_aurax11.cc (right):

https://codereview.chromium.org/16271006/diff/98001/ui/base/dragdrop/os_excha...
ui/base/dragdrop/os_exchange_data_provider_aurax11.cc:109: size_t text_len =
utf8.size();
Document ownership.

https://codereview.chromium.org/16271006/diff/98001/ui/base/x/selection_utils.h
File ui/base/x/selection_utils.h (right):

https://codereview.chromium.org/16271006/diff/98001/ui/base/x/selection_utils...
ui/base/x/selection_utils.h:53: // Adds the selection in the format |atom|.
Ownership of |data| is passed to
The semantics you're adding here are confusing and error prone (Insert can take
the same data multiple times). If there a reason you aren't using a const
std::string&?

[Elliot Glaysher, 2013-06-18 20:28:17]

sky: Just asking for an owners stamp

https://codereview.chromium.org/16271006/diff/98001/ui/base/x/selection_utils.h
File ui/base/x/selection_utils.h (right):

https://codereview.chromium.org/16271006/diff/98001/ui/base/x/selection_utils...
ui/base/x/selection_utils.h:53: // Adds the selection in the format |atom|.
Ownership of |data| is passed to
On 2013/06/18 14:09:45, sky wrote:
> The semantics you're adding here are confusing and error prone (Insert can
take
> the same data multiple times). If there a reason you aren't using a const
> std::string&?

These semantics date back in some way or another to when we started supporting
clipboards in GTK (r4719). SelectionFormatMap is a fairly straightforward copy
of clipboard_gtk.cc's TargetMap.

AFAIU, the reasoning behind why things work like this is that |data| can be huge
and can be served multiple times, especially when you consider that image data
can go on the clipboard under multiple MIME types.

I think I could make this backed by a map of shared_ptr<std::string> instead,
but any refactoring here is going to require major overhauls of the clipboard
system, which we share this code with. Would you mind if I do that in a followup
patch?

[sky, 2013-06-18 21:35:26]

SGTM and LGTM

[commit-bot: I haz the power, 2013-06-18 21:47:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/16271006/115002

[commit-bot: I haz the power, 2013-06-19 13:09:58]

Change committed as 207219