mus: Implement interwindow drag and drop

mash: Implements basic drag and drop IPC.

This implements a new mus IPC protocol to allow inter-process drag and
drop in mus. It handles the entire lifecycle of a drag between two mus
Windows, possibly in different processes. This patch does not implement
changing the mouse cursor or showing a drag representation, which is
queued work.

This patch is mus side only; it does not implement the views sided of
this protocol, which is next. (With it, you can drag text from the
chrome omnibox to the quick launcher and vice versa. You can drag text
from the quick launcher to the content area. Etc.)

BUG=614037
Committed: https://crrev.com/0873818c95cc607d8afe5a16b345fafe4109a735
Cr-Commit-Position: refs/heads/master@{#419005}

[Elliot Glaysher, 2016-08-22 22:42:51]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 22:43:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 23:09:44]

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

[commit-bot: I haz the power, 2016-08-22 23:09:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Elliot Glaysher, 2016-08-22 23:16:58]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:17:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-08-22 23:53:38]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:54:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 00:58:03]

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

[commit-bot: I haz the power, 2016-08-23 00:58:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-08-23 22:10:28]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 22:11:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-08-23 22:31:07]

Description was changed from

==========
[xxx]: Patch backup.

This implements most of drag and drop on mus.

[xxx]: Still doesn't deal with web content. I'm not tickling some blink
stuff correctly.

BUG=
==========

to

==========
mash: Implements basic drag and drop IPC.

This implements enough of a new mus IPC protocol to allow mash chrome to drag
text from the omnibox to the titlebar or to content area.

(This does not implement changing the mouse cursor or showing a drag
representation which look hard. I want to keep this patch comparatively small to
if I implemented everything at once.)

BUG=614037
==========

[Elliot Glaysher, 2016-08-23 22:31:07]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2016-08-23 22:31:57]

This patch isn't really ready for review (the patch works, but it needs
significantly more unit testing before the "real" send out), but I have an
aesthetic question, sky, since that will interact with how I test this. (When
this does get sent out for real, all the ui/views code will go in a separate
patch.)

Right now, incoming mojo calls to a WindowTree get routed through the WMS to
the event dispatcher, which creates and owns a CurrentDragOperation which
intercepts individual pointer events. I believe that I'm currently doing a few
layer violation: I'm passing the WindowTree* all the way down to the
CurrentDragOperation (I'm doing that so that I can fire the OnChangeCompleted()
signal at the end of the operation), and I'm passing the WindowServer* all the
way down, too (I'm doing that so that I can GetTreeWithRoot() of arbitrary
ServerWindows*s so that I can dispatch the next step of the drag state
machine).

What should I do here?

  1) Should I move these things to delegate interfaces? This could be a
  solution for the WindowServer* usage but it doesn't really help with the
  WindowTree stuff.
  
  2) I suppose I could also pass a callback on WindowTree::OnCompletedCallback
  instead, but then I'd have to make WindowTree have a weak factory.

  3) Should I do nothing and leave it? But if so, how do I track whether the
  WindowTree* is going away?

[commit-bot: I haz the power, 2016-08-23 23:25:46]

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

[commit-bot: I haz the power, 2016-08-23 23:25:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2016-08-24 00:08:57]

Good call on asking about the layering violation. I've tried to keep the
dependencies of EventDispatcher at a minimum.

https://codereview.chromium.org/2266603002/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.h (right):

https://codereview.chromium.org/2266603002/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.h:83: void SetDragDropSourceWindow(
I recommend having this take an interface rather than WindowTree. WindowTree
either implements the interface, or creates an object that implements the
interface (probably the former).
As far as lifetime issues, WindowTree will know it's installed itself as the
drag drop source, right? It can cancel dnd from the destructor (or sooner) as
necessary.

[Elliot Glaysher, 2016-08-24 21:54:47]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 21:55:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-08-24 23:02:00]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 23:03:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 00:58:10]

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

[commit-bot: I haz the power, 2016-08-25 00:58:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-08-26 23:07:09]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 23:08:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 00:42:49]

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

[commit-bot: I haz the power, 2016-08-27 00:42:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-08-31 23:29:23]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 23:29:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 00:56:48]

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

[commit-bot: I haz the power, 2016-09-01 00:56:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Elliot Glaysher, 2016-09-01 21:08:16]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 21:08:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 22:48:42]

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

[commit-bot: I haz the power, 2016-09-01 22:48:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-09-02 21:24:10]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 21:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 22:41:41]

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

[commit-bot: I haz the power, 2016-09-02 22:41:44]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Elliot Glaysher, 2016-09-02 23:44:45]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 23:45:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 00:37:48]

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

[commit-bot: I haz the power, 2016-09-03 00:37:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-09-06 17:43:48]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 17:44:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-06 18:00:30]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 18:01:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-06 18:20:48]

Description was changed from

==========
mash: Implements basic drag and drop IPC.

This implements enough of a new mus IPC protocol to allow mash chrome to drag
text from the omnibox to the titlebar or to content area.

(This does not implement changing the mouse cursor or showing a drag
representation which look hard. I want to keep this patch comparatively small to
if I implemented everything at once.)

BUG=614037
==========

to

==========
mash: Implements basic drag and drop IPC.

This implements a new mus IPC protocol to allow inter-process drag and
drop in mus. It handles the entire lifecycle of a drag between two mus
Windows, possibly in different processes. This patch does not implement
changing the mouse cursor or showing a drag representation, which is
queued work.

This patch is mus side only; it does not implement the views sided of
this protocol, which is next. (With it, you can drag text from the
chrome omnibox to the quick launcher and vice versa. You can drag text
from the quick launcher to the content area. Etc.)

BUG=614037
==========

[commit-bot: I haz the power, 2016-09-06 19:04:33]

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

[commit-bot: I haz the power, 2016-09-06 19:04:34]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-09-06 19:11:38]

OK, this is ready for review.

This patch isn't as large as it looks; about a third of its weight is new unit
tests and testing related stuff.

[sky, 2016-09-06 21:11:27]

I just looked at the changes to the window server side. Here's some initial
feedback.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:116: // Called by clients that
want to accept drags. Windows default
Similar comment about drops.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:283: PerformDragDrop(uint32
change_id,
Don't you need an image and offset into that image for the drag operation?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:453: // OnDragFirstEnter()
message. This signals that a client can forget the
OnDragStart?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/BUILD.gn
File services/ui/ws/BUILD.gn (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/BUILD.g...
services/ui/ws/BUILD.gn:22: "current_drag_operation.h",
current_drag_operation_source.h= and drag_target_connection.h?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
File services/ui/ws/current_drag_operation.cc (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.cc:19: gfx::Point position;
Document what this is relative to. Screen?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.cc:50: // If we're waiting on a target
window to respond to the final drag drop
If you early return here, does it mean the event is dropped on the floor?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.cc:58: current_target = nullptr;
Shouldn't this bubble

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.cc:62: event.type() != ET_POINTER_UP) {
Shouldn't you only care about POINTER_UP of the pointer that started the drag
and if there are no other touch points down for the pointer type?
Also, it seems like this block only cares about a move the pointer type that
started the drag.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
File services/ui/ws/current_drag_operation.h (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.h:28: class CurrentDragOperation : public
ServerWindowObserver {
Is 'current' really helpful in this context? Is just DragOperation or
DragController better?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.h:37: void DispatchCancel();
I would just call this Cancel().

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.h:45: enum DragEventType { TYPE_ENTER,
TYPE_OVER, TYPE_LEAVE, TYPE_DROP };
enum class, and nuke TYPE_?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.h:52: size_t
GetSizeOfQueueForWindow(ServerWindow* window);
Description.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.h:72: uint32_t drag_operations_;
const?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation.h:86: std::map<ServerWindow*,
std::deque<Operation>> current_window_state_;
window_operations_?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
File services/ui/ws/current_drag_operation_source.h (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation_source.h:22: virtual void OnDragOver(bool
success) = 0;
On initial look I read this as 'drag moved ontop of a window'. But that isn't
the case. How about DragCompleted or DragFinished? And document what success
means.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/current...
services/ui/ws/current_drag_operation_source.h:26: virtual DragTargetConnection*
GetDragTargetWithRoot(
Document what this should do. I assume find deepest, it's good to document that.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/drag_ta...
File services/ui/ws/drag_target_connection.h (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/drag_ta...
services/ui/ws/drag_target_connection.h:54: virtual void
PerformOnDragFinish(const ServerWindow* window) = 0;
Document when this is sent. It isn't clear how it relates to PerformOnDRagLeave
and PerformOnDragDrop. What would actually help is class level doc that
describes the cycle.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:189: current_drag_drop_operation_.reset(new
CurrentDragOperation(
MakeUnique is the new hotness.

Should this implicitly change capture? What do we do in the aura side now?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:352: const PointerTarget target =
PointerTargetForEvent(event);
Won't an early return potentially leave pointer_targets_ in the wrong state?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/server_...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/server_...
services/ui/ws/server_window.h:249: bool accepts_drags_ = false;
Isn't this accepts_drops_?

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/window_...
File services/ui/ws/window_manager_access_policy.cc (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/window_...
services/ui/ws/window_manager_access_policy.cc:143: // Only the client
associated with a window may set this.
Are you sure you don't want WasCreatedByThisClient(window)? Otherwise it means
any windows created by the window manager can't accept drags.

[Elliot Glaysher, 2016-09-07 21:10:26]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 21:11:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-07 21:42:23]

https://codereview.chromium.org/2266603002/diff/260001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:283: PerformDragDrop(uint32
change_id,
On 2016/09/06 21:11:26, sky wrote:
> Don't you need an image and offset into that image for the drag operation?

I removed all management of drag representations from this patch to keep things
reasonable; they'll be added in another patch. (Unless you feel strongly that
they should be added now and remain unused on the mus side.)

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:189: current_drag_drop_operation_.reset(new
CurrentDragOperation(
On 2016/09/06 21:11:27, sky wrote:
> MakeUnique is the new hotness.
> 
> Should this implicitly change capture? What do we do in the aura side now?

The normal ash drag controller implicitly takes capture on the beginning of a
drag.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:352: const PointerTarget target =
PointerTargetForEvent(event);
On 2016/09/06 21:11:27, sky wrote:
> Won't an early return potentially leave pointer_targets_ in the wrong state?

Maybe I'm misunderstanding this, but why is this a problem when sending a mouse
event to an explicitly captured when isn't?

[commit-bot: I haz the power, 2016-09-07 22:44:33]

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

[commit-bot: I haz the power, 2016-09-07 22:44:34]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-09-07 22:54:02]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 22:54:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-09-07 23:25:29]

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:189: current_drag_drop_operation_.reset(new
CurrentDragOperation(
On 2016/09/07 21:42:23, Elliot Glaysher wrote:
> On 2016/09/06 21:11:27, sky wrote:
> > MakeUnique is the new hotness.
> > 
> > Should this implicitly change capture? What do we do in the aura side now?
> 
> The normal ash drag controller implicitly takes capture on the beginning of a
> drag.

Relying on ash to have done before calling this is error prone and racy. If we
need to ensure capture is set we should either fail if capture isn't set, or
implicitly grab capture if not already grabbed.

https://codereview.chromium.org/2266603002/diff/260001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:352: const PointerTarget target =
PointerTargetForEvent(event);
On 2016/09/07 21:42:23, Elliot Glaysher wrote:
> On 2016/09/06 21:11:27, sky wrote:
> > Won't an early return potentially leave pointer_targets_ in the wrong state?
> 
> Maybe I'm misunderstanding this, but why is this a problem when sending a
mouse
> event to an explicitly captured when isn't?

As long as you ensure capture is present, then I agree this early return is ok.
That said, what happens if capture changes part way through the drag? Should the
drag be canceled? If not, things are in a bad state.

[commit-bot: I haz the power, 2016-09-08 00:32:15]

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

[commit-bot: I haz the power, 2016-09-08 00:32:16]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-09-08 20:44:49]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 20:45:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-08 21:18:27]

Reset the implicit capture on start and rejected capture requests during drag as
discussed in person.

[commit-bot: I haz the power, 2016-09-08 22:34:21]

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

[commit-bot: I haz the power, 2016-09-08 22:34:23]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-08 23:37:21]

I looked through more of this. Feel free to stop by if you have questions on
anything.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
File services/ui/public/cpp/window_drop_target.h (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_drop_target.h:16: // pass to their mus::Window.
This is the client side equivalent to the
mus->ui

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_drop_target.h:17: // messages defined in
ui::ws::DragTargetConnection.
ui->mojom

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_drop_target.h:24: // mus::Window receives this
immediately through the client library instead
mus->ui (to a global search/replace as I'm going to stop mentioning it).

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1108: if (!target) {
combine with if on 1102?

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1129: if (!target) {
Combine with if on 1123?

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:422: OnDragStart(uint32 window,
Based on the name I thought this was a callback from initiating a drag. But that
isn't the case. This function is about providing the mime types. Maybe
OnDragMimeTypes(...)?

Is there a reason the window is needed here? It seems like it would be better to
call this function once per connection, rather than per once per window in a
connection.

Also, did you consider grouping these functions in an associated interface?
Placing them in a separate interface means you have better lifetime (you
wouldn't need OnDragFinish() for one). I'm thinking of something like
GetDropTarget(mime_types, associated DropTarget& drop_target).

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:448: OnDragDrop(uint32 window,
The 'Drag' in the name here is mildly confusing. How about OnDrop or
OnPerformDrop to better indicate this should process the actual drop.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/default...
File services/ui/ws/default_access_policy.cc (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/default...
services/ui/ws/default_access_policy.cc:153: return
delegate_->HasRootForAccessPolicy(window);
This only accepts drops on the root. You also want
WasCreatedByThisClient(window) here, otherwise windows created by this client
can't accept drops. I think you'll also want to forbid the drop if there is
another client embedded in the window. In other words I think you want the same
thing as is in DefaultAccessPolicy::CanDescendIntoWindowForWindowTree(). I think
you want the same in the WindowManagerAccessPolicy.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:44: // |this| may be invalid now.
invalid->deleted?

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:49: uint32_t key_state = event.flags();
Naming this key_state, when it's event_flags is mildly confusing. Could you
rename to event_flags every where? Seems like you should also mask out only the
parts that matter, e.g. shift down, control down but not button state.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:239: if (current_target_window_ == window)
You never add an observer for current_target_window_. Don't you need to cleanup
if it is destroyed?

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:246: source_window_ = nullptr;
RemoveObserver?

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:32: int32_t darg_pointer,
darg->drag

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:60: void QueueEvent(ServerWindow* window,
It seems like this duplicates event queuing logic that is also in
WindowManagerState. It shouldn't be that hard to have WindowManagerState queue
events while waiting for a response from a client, which should simplify this
class. You'll also want to leverage the ack timer in WindowManagerState to know
if a client is considered dead and not block all events.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:88: ServerWindow* source_window_ = nullptr;
You set this in the member initialize, so don't set here too.

[Elliot Glaysher, 2016-09-12 23:35:06]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 23:35:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-13 00:14:20]

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:422: OnDragStart(uint32 window,
On 2016/09/08 23:37:20, sky wrote:
> Based on the name I thought this was a callback from initiating a drag. But
that
> isn't the case. This function is about providing the mime types. Maybe
> OnDragMimeTypes(...)?
> 
> Is there a reason the window is needed here? It seems like it would be better
to
> call this function once per connection, rather than per once per window in a
> connection.

I renamed this and also changed it to transfer once per connection instead of
once per window.
 
> Also, did you consider grouping these functions in an associated interface?
> Placing them in a separate interface means you have better lifetime (you
> wouldn't need OnDragFinish() for one). I'm thinking of something like
> GetDropTarget(mime_types, associated DropTarget& drop_target).

I experimented with this, but this turns a bunch of explicit
OnDragMimeTypes/OnDragFinish() calls into some implicit calls around the
lifetime of a wrapper around DropTarget. (Ownership becomes an issue to worry
about since WindowTree would have to offer the same DropTarget for multiple
windows; DragController asks DragSource for the DropTargetConnection (or
DropTarget in this world) per window, and WindowTree can pass a different
WindowTree back.)

https://codereview.chromium.org/2266603002/diff/320001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:448: OnDragDrop(uint32 window,
On 2016/09/08 23:37:20, sky wrote:
> The 'Drag' in the name here is mildly confusing. How about OnDrop or
> OnPerformDrop to better indicate this should process the actual drop.

Renamed to OnCompleteDrop (I'm already using the word perform in another
context).

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:246: source_window_ = nullptr;
On 2016/09/08 23:37:20, sky wrote:
> RemoveObserver?

Did this and also made sure we were unsubscribing from everything in the dtor.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:60: void QueueEvent(ServerWindow* window,
On 2016/09/08 23:37:21, sky wrote:
> It seems like this duplicates event queuing logic that is also in
> WindowManagerState. It shouldn't be that hard to have WindowManagerState queue
> events while waiting for a response from a client, which should simplify this
> class.

This isn't clear to me.

The event queueing logic in WindowManagerState works on ui::Events, which go
through the whole input dispatch layer, which we don't want here. This method
adds an DragController::Operation to this window's window_operations_ queue. To
make this clearer, I renamed this from QueueEvent (and others) to
QueueOperation.

> You'll also want to leverage the ack timer in WindowManagerState to know
> if a client is considered dead and not block all events.

A dead client won't block all events. Each queue in |window_operations_|
operates independently. I also could have sworn I wrote a unit test to exercise
this specific case, but it looks like I didn't, so I just wrote one.

(That said, perhaps there should be a timer on OnCompleteDrop(); that's the only
part where a dead window could hold up the process and I'm not what a reasonable
timeout would be since the window dropped on can do a lot of computation on this
event.)

[commit-bot: I haz the power, 2016-09-13 00:41:45]

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

[commit-bot: I haz the power, 2016-09-13 00:41:47]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-13 18:15:32]

I made it through the whole thing this time. Yay! It's almost there, the
following are mostly minor name/description changes.

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2266603002/diff/320001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:60: void QueueEvent(ServerWindow* window,
On 2016/09/13 00:14:19, Elliot Glaysher wrote:
> On 2016/09/08 23:37:21, sky wrote:
> > It seems like this duplicates event queuing logic that is also in
> > WindowManagerState. It shouldn't be that hard to have WindowManagerState
queue
> > events while waiting for a response from a client, which should simplify
this
> > class.
> 
> This isn't clear to me.
> 
> The event queueing logic in WindowManagerState works on ui::Events, which go
> through the whole input dispatch layer, which we don't want here. This method
> adds an DragController::Operation to this window's window_operations_ queue.
To
> make this clearer, I renamed this from QueueEvent (and others) to
> QueueOperation.

I was hoping to share logic, in so far as waiting for a response before
continuing. But I think you're right that they are different use cases.

> > You'll also want to leverage the ack timer in WindowManagerState to know
> > if a client is considered dead and not block all events.
> 
> A dead client won't block all events. Each queue in |window_operations_|
> operates independently. I also could have sworn I wrote a unit test to
exercise
> this specific case, but it looks like I didn't, so I just wrote one.
> 
> (That said, perhaps there should be a timer on OnCompleteDrop(); that's the
only
> part where a dead window could hold up the process and I'm not what a
reasonable
> timeout would be since the window dropped on can do a lot of computation on
this
> event.)

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
File services/ui/public/cpp/in_flight_change.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
services/ui/public/cpp/in_flight_change.cc:45: : InFlightChange(window,
ChangeType::MOVE_LOOP) {}
You're using this for both move and drag now. Pass in the change type?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
services/ui/public/cpp/in_flight_change.cc:50: // Reverting bounds happens in
the window server in the error case; it cannot
remove stale comment.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
services/ui/public/cpp/window.h:216: WindowDropTarget* drop_target() const {
return drop_target_; }
Either return const here, or remove const from signature.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
File services/ui/public/cpp/window_drop_target.h (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
services/ui/public/cpp/window_drop_target.h:20: virtual ~WindowDropTarget() {}
move to protected section?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.h:454: uint32_t current_drag_change_ =
0u;
Move current_drag_change_ and on_current_drag_finished_ into a structure so that
it's clear they relate to PerformDragDrop. This gives better lifetime management
too.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.h:463: std::set<Id> entered_windows_;
Add description.
optional: include drag in the name to better indicate for drag.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:422: OnDragMimeTypes(map<string,
array<uint8>> drag_data);
optional: Please give this a better name as it isn't just the mime types, but
the data too. OnDragData? Maybe a name that better indicates it correlates with
OnDragFinish: OnDragDropStart and OnDragDropDone?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:46: for (auto& w : window_operations_)
optional: 'w' is mildly misleading as I would tend to think it means a window,
but it's really a pair. Maybe pair or window_operation_pair?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:198: // This is the last event in queue; stop
watching the queue.
If you do this, and the window is destroyed OnWindowDestroying won't be called.
Don't you want to have an observer attached any time the entry is added to
window_operations_?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:109: std::map<ServerWindow*,
std::deque<Operation>> window_operations_;
Did you consider going with a structure other than a deque? It seems like all
you care about here is:
waiting_for_response_from_enter_over;
Operation queued_operation;

A deque certainly works for this, but it makes the code harder to understand (it
took me a while to figure the above out). I also suspect the above will make it
easier to handle cursor updating too.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller_unittest.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller_unittest.cc:5: #include
"services/ui/ws/drag_controller.h"
newline between 5/6.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:562: if (window && target == window)
I don't think you need the 'window' conditional.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_server.cc:504: uint32_t
WindowServer::GetCurrentDragLoopChangeId() {
Why does this information need to be held by the WindowServer? AFAICT it's only
needed by the WindowTree that initiates the drag. Shouldn't it be held there?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1467: uint32_t source_window_id,
uint32_t -> Id

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1472: bool success = window &&
access_policy_->CanInitiateMoveLoop(window);
Please add a new function, CanInitiateDragLoop?

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1476: OnChangeCompleted(change_id, false);
Please DVLOG the failures.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1502: wms->SetDragDropSourceWindow(this, window,
this, drag_pointer,
optional: it's mildly weird that WindowServer::StartDropLoop doesn't call
wms->SetDragDropSourceWindow. In other words, it's weird the StartDragLoop
doesn't start anything.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1758: return
window_server_->GetTreeWithRoot(window);
This returns the WindowTree that has |window| as one if it's root. That's fine
for the embed point, but not for windows that aren't embedded. If
GetTreeWithRoot returns null you should fallback to the tree that created the
window.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_tree.h (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_tree.h:435: uint32_t source_window_id,
uint32_t -> Id

[Elliot Glaysher, 2016-09-14 21:25:31]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 21:26:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-14 22:19:16]

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:198: // This is the last event in queue; stop
watching the queue.
On 2016/09/13 18:15:31, sky wrote:
> If you do this, and the window is destroyed OnWindowDestroying won't be
called.
> Don't you want to have an observer attached any time the entry is added to
> window_operations_?

I'm confused.

If we're here, we're removing the last operation from the queue and the window
isn't the source or current window. After the following pop_front(), we won't
hold any reference to window unless I am very mistaken.

(I've rewritten the comment since maybe what was written was unclear/deceptive?)

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:562: if (window && target == window)
On 2016/09/13 18:15:32, sky wrote:
> I don't think you need the 'window' conditional.

|window| can be null (called at line 173).

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_server.cc:504: uint32_t
WindowServer::GetCurrentDragLoopChangeId() {
On 2016/09/13 18:15:32, sky wrote:
> Why does this information need to be held by the WindowServer? AFAICT it's
only
> needed by the WindowTree that initiates the drag. Shouldn't it be held there?

Parallelism with storing the move loop for the safety checks. If we have one
connection currently performing a drag, and a second tries to start a drag on a
different root in a different process, we should immediately return unsuccessful
the second one.

(Along with erroring on trying to start a move loop during a drag and vice
versa.)

[commit-bot: I haz the power, 2016-09-14 22:26:49]

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

[commit-bot: I haz the power, 2016-09-14 22:26:51]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-09-14 22:29:25]

erg@chromium.org changed reviewers:
+ tsepez@chromium.org

[Elliot Glaysher, 2016-09-14 22:29:26]

+tsepez for mojom security review

[sky, 2016-09-14 22:52:02]

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:198: // This is the last event in queue; stop
watching the queue.
On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> On 2016/09/13 18:15:31, sky wrote:
> > If you do this, and the window is destroyed OnWindowDestroying won't be
> called.
> > Don't you want to have an observer attached any time the entry is added to
> > window_operations_?
> 
> I'm confused.
> 
> If we're here, we're removing the last operation from the queue and the window
> isn't the source or current window. After the following pop_front(), we won't
> hold any reference to window unless I am very mistaken.

Isn't the reference still held in the map? e.g. window_operations_[window]?

> 
> (I've rewritten the comment since maybe what was written was
unclear/deceptive?)

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:562: if (window && target == window)
On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> On 2016/09/13 18:15:32, sky wrote:
> > I don't think you need the 'window' conditional.
> 
> |window| can be null (called at line 173).

Sure, but the !target on line 559 means you know target is not-null at this
point, so the only way target == window is true is if window is non-null,
meaning the if (window) part is unnecessary.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_server.cc:504: uint32_t
WindowServer::GetCurrentDragLoopChangeId() {
On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> On 2016/09/13 18:15:32, sky wrote:
> > Why does this information need to be held by the WindowServer? AFAICT it's
> only
> > needed by the WindowTree that initiates the drag. Shouldn't it be held
there?
> 
> Parallelism with storing the move loop for the safety checks. If we have one
> connection currently performing a drag, and a second tries to start a drag on
a
> different root in a different process, we should immediately return
unsuccessful
> the second one.
> 
> (Along with erroring on trying to start a move loop during a drag and vice
> versa.)

Isn't this information seldomly used? If so, can you iterate over all the
WindowTrees to see if one has a drag loop going on?

[Tom Sepez, 2016-09-15 17:07:20]

Mojom LGTM.

[Elliot Glaysher, 2016-09-15 17:12:32]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[Elliot Glaysher, 2016-09-15 17:12:40]

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:198: // This is the last event in queue; stop
watching the queue.
On 2016/09/14 22:52:02, sky wrote:
> On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> > On 2016/09/13 18:15:31, sky wrote:
> > > If you do this, and the window is destroyed OnWindowDestroying won't be
> > called.
> > > Don't you want to have an observer attached any time the entry is added to
> > > window_operations_?
> > 
> > I'm confused.
> > 
> > If we're here, we're removing the last operation from the queue and the
window
> > isn't the source or current window. After the following pop_front(), we
won't
> > hold any reference to window unless I am very mistaken.
> 
> Isn't the reference still held in the map? e.g. window_operations_[window]?
> 
> > 
> > (I've rewritten the comment since maybe what was written was
> unclear/deceptive?)
> 

This (and related) chunks have been removed. This method of tracking observers
is probably just too complex to track. Now everything that's tracked is in a
unified  window_state_ map.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/event_d...
services/ui/ws/event_dispatcher.cc:562: if (window && target == window)
On 2016/09/14 22:52:02, sky wrote:
> On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> > On 2016/09/13 18:15:32, sky wrote:
> > > I don't think you need the 'window' conditional.
> > 
> > |window| can be null (called at line 173).
> 
> Sure, but the !target on line 559 means you know target is not-null at this
> point, so the only way target == window is true is if window is non-null,
> meaning the if (window) part is unnecessary.

Done.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_server.cc:504: uint32_t
WindowServer::GetCurrentDragLoopChangeId() {
On 2016/09/14 22:52:02, sky wrote:
> On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> > On 2016/09/13 18:15:32, sky wrote:
> > > Why does this information need to be held by the WindowServer? AFAICT it's
> > only
> > > needed by the WindowTree that initiates the drag. Shouldn't it be held
> there?
> > 
> > Parallelism with storing the move loop for the safety checks. If we have one
> > connection currently performing a drag, and a second tries to start a drag
on
> a
> > different root in a different process, we should immediately return
> unsuccessful
> > the second one.
> > 
> > (Along with erroring on trying to start a move loop during a drag and vice
> > versa.)
> 
> Isn't this information seldomly used? If so, can you iterate over all the
> WindowTrees to see if one has a drag loop going on?

WindowServer doesn't publicly expose the WindowTreeMap. Adding a method on
WindowServer to expose it or write a method on WindowServer that searches for it
feels worse than this.

[commit-bot: I haz the power, 2016-09-15 17:12:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-09-15 18:10:47]

Almost there, mostly minor stuff now.

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2266603002/diff/340001/services/ui/ws/window_...
services/ui/ws/window_server.cc:504: uint32_t
WindowServer::GetCurrentDragLoopChangeId() {
On 2016/09/15 17:12:40, Elliot Glaysher wrote:
> On 2016/09/14 22:52:02, sky wrote:
> > On 2016/09/14 22:19:16, Elliot Glaysher wrote:
> > > On 2016/09/13 18:15:32, sky wrote:
> > > > Why does this information need to be held by the WindowServer? AFAICT
it's
> > > only
> > > > needed by the WindowTree that initiates the drag. Shouldn't it be held
> > there?
> > > 
> > > Parallelism with storing the move loop for the safety checks. If we have
one
> > > connection currently performing a drag, and a second tries to start a drag
> on
> > a
> > > different root in a different process, we should immediately return
> > unsuccessful
> > > the second one.
> > > 
> > > (Along with erroring on trying to start a move loop during a drag and vice
> > > versa.)
> > 
> > Isn't this information seldomly used? If so, can you iterate over all the
> > WindowTrees to see if one has a drag loop going on?
> 
> WindowServer doesn't publicly expose the WindowTreeMap. Adding a method on
> WindowServer to expose it or write a method on WindowServer that searches for
it
> feels worse than this.

Fair enough.

https://codereview.chromium.org/2266603002/diff/380001/services/ui/public/cpp...
File services/ui/public/cpp/in_flight_change.h (right):

https://codereview.chromium.org/2266603002/diff/380001/services/ui/public/cpp...
services/ui/public/cpp/in_flight_change.h:150: explicit
InFlightDragChange(Window* window, ChangeType type);
remove explicit

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/default...
File services/ui/ws/default_access_policy.cc (right):

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/default...
services/ui/ws/default_access_policy.cc:171: return
delegate_->HasRootForAccessPolicy(window);
Dragging should also be allowed on windows created by the client (e.g. same as
CanSetCursorProperties above).

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:22: struct DragController::WindowState {
Much easier to understand, and better documented. Thanks!

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:23: // Set to true once we've observed
Did you forget to finish the sentence?

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:140: size_t
DragController::GetSizeOfQueueForWindow(ServerWindow* window) {
It doesn't seem as though you are using this anymore?

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:195: case OperationType::NONE: {
It is possible to get here with an operation of NONE? Assuming it is, please
document why as it isn't readily obvious how you end up here with a type of
NONE.

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/window_...
File services/ui/ws/window_manager_access_policy.cc (right):

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/window_...
services/ui/ws/window_manager_access_policy.cc:160: return false;
If you return here it means the windowmanager can't initiated drags. We
definitely want the WM to be able to drag. Go with the policy of
CanSetCursorProperties here too.

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1477: DVLOG(1) << "PerformDragDrop failed (access
denied).";
Thanks for adding the logs. I think they're helpful for debugging failures.

[Elliot Glaysher, 2016-09-15 19:13:41]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 19:14:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-15 19:14:33]

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:140: size_t
DragController::GetSizeOfQueueForWindow(ServerWindow* window) {
On 2016/09/15 18:10:47, sky wrote:
> It doesn't seem as though you are using this anymore?

It's used in the unit tests via the private testing api.

https://codereview.chromium.org/2266603002/diff/380001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.cc:195: case OperationType::NONE: {
On 2016/09/15 18:10:47, sky wrote:
> It is possible to get here with an operation of NONE? Assuming it is, please
> document why as it isn't readily obvious how you end up here with a type of
> NONE.

Added a NOTREACHED() to emphasize that this is to silence the compiler.

[commit-bot: I haz the power, 2016-09-15 20:10:54]

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

[commit-bot: I haz the power, 2016-09-15 20:10:56]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-15 22:00:23]

LGTM

[Elliot Glaysher, 2016-09-15 22:01:31]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-09-15 22:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2266603002/#ps400001
(title: "more comments")

[commit-bot: I haz the power, 2016-09-15 22:02:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 22:08:59]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-09-15 22:10:51]

Description was changed from

==========
mash: Implements basic drag and drop IPC.

This implements a new mus IPC protocol to allow inter-process drag and
drop in mus. It handles the entire lifecycle of a drag between two mus
Windows, possibly in different processes. This patch does not implement
changing the mouse cursor or showing a drag representation, which is
queued work.

This patch is mus side only; it does not implement the views sided of
this protocol, which is next. (With it, you can drag text from the
chrome omnibox to the quick launcher and vice versa. You can drag text
from the quick launcher to the content area. Etc.)

BUG=614037
==========

to

==========
mash: Implements basic drag and drop IPC.

This implements a new mus IPC protocol to allow inter-process drag and
drop in mus. It handles the entire lifecycle of a drag between two mus
Windows, possibly in different processes. This patch does not implement
changing the mouse cursor or showing a drag representation, which is
queued work.

This patch is mus side only; it does not implement the views sided of
this protocol, which is next. (With it, you can drag text from the
chrome omnibox to the quick launcher and vice versa. You can drag text
from the quick launcher to the content area. Etc.)

BUG=614037

Committed: https://crrev.com/0873818c95cc607d8afe5a16b345fafe4109a735
Cr-Commit-Position: refs/heads/master@{#419005}
==========

[commit-bot: I haz the power, 2016-09-15 22:10:54]

Patchset 21 (id:??) landed as
https://crrev.com/0873818c95cc607d8afe5a16b345fafe4109a735
Cr-Commit-Position: refs/heads/master@{#419005}