Issue 2323553002: mash: Use the new mus drag and drop API to get drag working in mash.

Elliot Glaysher

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

4 years, 3 months ago (2016-09-07 22:20:06 UTC) #1

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-07 22:20:46 UTC) #2

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-07 22:24:12 UTC) #3

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-07 22:24:13 UTC) #4

Elliot Glaysher

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

4 years, 3 months ago (2016-09-07 23:04:17 UTC) #5

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-07 23:04:56 UTC) #6

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-08 00:47:27 UTC) #7

commit-bot: I haz the power

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_rel_ng/builds/293921)

4 years, 3 months ago (2016-09-08 00:47:28 UTC) #8

Elliot Glaysher

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

4 years, 3 months ago (2016-09-14 22:29:04 UTC) #9

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-14 22:29:30 UTC) #10

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-14 23:59:12 UTC) #11

commit-bot: I haz the power

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_rel_ng/builds/297916)

4 years, 3 months ago (2016-09-14 23:59:13 UTC) #12

Elliot Glaysher

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

4 years, 3 months ago (2016-09-15 21:50:50 UTC) #13

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-15 21:51:31 UTC) #14

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-15 23:12:17 UTC) #15

commit-bot: I haz the power

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_rel_ng/builds/298718)

4 years, 3 months ago (2016-09-15 23:12:18 UTC) #16

Elliot Glaysher

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

4 years, 3 months ago (2016-09-16 23:20:54 UTC) #17

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-16 23:21:16 UTC) #18

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-17 00:35:16 UTC) #19

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-17 00:35:17 UTC) #20

Elliot Glaysher

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

4 years, 3 months ago (2016-09-21 21:55:53 UTC) #21

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-21 21:56:13 UTC) #22

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-21 23:39:20 UTC) #23

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-21 23:39:21 UTC) #24

Elliot Glaysher

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

4 years, 3 months ago (2016-09-22 17:46:11 UTC) #25

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-22 17:46:41 UTC) #26

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-22 18:55:00 UTC) #27

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-22 18:55:01 UTC) #28

Elliot Glaysher

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

4 years, 3 months ago (2016-09-22 20:06:58 UTC) #29

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-22 20:07:33 UTC) #30

Elliot Glaysher

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

4 years, 3 months ago (2016-09-22 20:23:19 UTC) #31

Elliot Glaysher

sadrul: questions about pointer ids sky: review https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode72 ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer ...

4 years, 3 months ago (2016-09-22 20:23:20 UTC) #32

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-22 21:08:40 UTC) #33

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-22 21:08:41 UTC) #34

sky

Mostly just style stuff. https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode20 ui/views/mus/drag_drop_client_mus.cc:20: DragDropClientMus* g_current_dragging_client = nullptr; My ...

4 years, 2 months ago (2016-09-23 16:14:09 UTC) #35

sadrul

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode53 ui/views/mus/drag_drop_client_mus.cc:53: base::MessageLoopForUI* loop = base::MessageLoopForUI::current(); Can this be just MessageLoop? ...

4 years, 2 months ago (2016-09-23 16:22:36 UTC) #36

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
File ui/views/mus/drag_drop_client_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:53: base::MessageLoopForUI* loop =
base::MessageLoopForUI::current();
Can this be just MessageLoop? Because mus apps do not have (more like should not
have) TYPE_UI message loops (because there can be side-effects, e.g. the glib
message loop triggers sandbox traps).

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer =
ui::PointerEvent::kMousePointerId;
On 2016/09/22 20:23:20, Elliot Glaysher wrote:
> This is the one final issue with this patch. At this point, we've been called
by
> either of the two methods in the above comment, and we need a pointer id.
> However, gesture events can have multiple pointer ids, if I'm reading a
comment
> from sadrul correctly.
> 
> The pointer used on the mus server side in drag_controller.cc to only respond
to
> events which have the same pointer id, to ensure if I'm dragging with the
mouse,
> pointer down and up with a finger shouldn't then release the drag.

Does it matter what pointer id you use on the client side for dragging? Can it
be some special pointer id, e.g. kDragPointer? That way, you can continue to
process events from other non-dragging pointers (although that may be a bit
weird to do that?), and the WS will take care of making sure the pointer-id
involved in dragging only takes part in dragging until the release.

Having said that, ui::GestureEvent has a unique_touch_event_id() field that you
*could* use if you really really wanted to
(https://cs.chromium.org/chromium/src/ui/events/event.h?sq=package:chromium&dr...),
but best avoid it, because a gesture event can have more than one contributing
touch point (e.g. two-finger scroll, pinch-zoom etc.)

sky

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode72 ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer = ui::PointerEvent::kMousePointerId; On 2016/09/22 20:23:20, Elliot Glaysher ...

4 years, 2 months ago (2016-09-23 17:56:28 UTC) #37

Elliot Glaysher

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

4 years, 2 months ago (2016-09-23 21:39:41 UTC) #38

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-23 21:40:22 UTC) #39

Elliot Glaysher

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode51 ui/views/mus/drag_drop_client_mus.cc:51: aura_window_->ReleaseCapture(); On 2016/09/23 16:14:08, sky wrote: > Please comment ...

4 years, 2 months ago (2016-09-23 21:40:48 UTC) #40

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
File ui/views/mus/drag_drop_client_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:51: aura_window_->ReleaseCapture();
On 2016/09/23 16:14:08, sky wrote:
> Please comment why this is necessary.

Deleted; it wasn't necessary anymore now that there's an implicit release
capture on the server side.

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:53: base::MessageLoopForUI* loop =
base::MessageLoopForUI::current();
On 2016/09/23 16:22:36, sadrul wrote:
> Can this be just MessageLoop? Because mus apps do not have (more like should
not
> have) TYPE_UI message loops (because there can be side-effects, e.g. the glib
> message loop triggers sandbox traps).

Done.

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer =
ui::PointerEvent::kMousePointerId;
On 2016/09/23 17:56:27, sky wrote:
> On 2016/09/22 20:23:20, Elliot Glaysher wrote:
> > This is the one final issue with this patch. At this point, we've been
called
> by
> > either of the two methods in the above comment, and we need a pointer id.
> > However, gesture events can have multiple pointer ids, if I'm reading a
> comment
> > from sadrul correctly.
> > 
> > The pointer used on the mus server side in drag_controller.cc to only
respond
> to
> > events which have the same pointer id, to ensure if I'm dragging with the
> mouse,
> > pointer down and up with a finger shouldn't then release the drag.
> 
> On the mus side we know we are waiting on a response from the client to a
> particular event. So, we should be able to tie the pointer id together there,
> right?

At this point, mus will no longer have the event which caused this, because the
event dispatch machinery in NativeWidgetMus creates a EventAckHandler, which is
a MessageLoop::NestingObserver which sends handled event acks to mus on a nested
message loop being created, which happens at the top of this method. Therefore,
by the time we even start the drag drop on the mus side,
WindowManagerState::event_awaiting_input_ack_ is null.

(Also, implicit spooky action at a distance sucks.)

I also tried to look at the |pointer_targets_| in the EventDispatcher, but since
there appears to be an explicit capture taken by client during mousedown (not
related to dragging; just normal event dispatch stuff), pointer targets gets
cleared.

Should I add another tracking mechanism in event dispatcher that keeps track of
which pointers are down regardless of capture state?

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drop_targ...
File ui/views/mus/drop_target_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drop_targ...
ui/views/mus/drop_target_mus.cc:40: void DropTargetMus::Translate(uint32_t
key_state,
On 2016/09/23 16:14:08, sky wrote:
> The name translate is rather generic and doesn't really convey what this does.
> Consider something like UpdateTargetWindow or ProcessDrag.

I'd prefer to keep "Translate()" because every other implementation of drag and
drop in chrome has a Translate() method which performs roughly the same
following steps. It's fairly entrenched.

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drop_targ...
ui/views/mus/drop_target_mus.cc:97: mime_data_ = std::move(mime_data);
On 2016/09/23 16:14:08, sky wrote:
> Why do you need to create OsExchangeData on every move? Could you create it
here
> and use that instance?

Created an OSExchangeData here instead.

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drop_targ...
File ui/views/mus/drop_target_mus.h (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drop_targ...
ui/views/mus/drop_target_mus.h:55: std::map<std::string, std::vector<uint8_t>>
mime_data) override;
On 2016/09/23 16:14:08, sky wrote:
> Just noticed this. How come this doesn't take a const map&?

To minimize the number of copies of an arbitrarily large map.

- Incoming mojo map in WindowTreeClient gets move()d to WTC storage.
- The first time the cursor enters a window, its drop target receives an
OnDragDropStart with a std::map. This creates a copy of the map.
- If that map was passed in const&, when it reaches its final storage
assignment, it  needs to be copied a second time due to the constness.
- But if that map is consistently move()d, we won't be building a second copy.

(OK, but then why not make something like linked_ptr<mojo::Map<...>>? Because
the OSExchangeData object is mutable.)

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:715: drop_target_.reset(new
DropTargetMus(content_));
On 2016/09/23 16:14:09, sky wrote:
> MakeUnique

Changed entire file to use MakeUnique.

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:759: if (!params.parent) {
On 2016/09/23 16:14:09, sky wrote:
> I'm unclear as to why you don't create this for every NativeWidgetMus. Every
> NativeWidgetMus contains a hierarchy of aura::Windows with a single root and
has
> a ui::Window. Don't you need to process the drag for every ui::Window?

I'm not sure I believe that this is true.

It appears that menus violate some of those assumptions. Without the `if
(!params.parent)' block around setting the drag drop client,MenuControllerTest.
MenuControllerReplacedDuringDrag times out. The test is set up to create
multiple NativeWidgets (a toplevel one, and one owned by MenuHost, which is a
child), and the implicit assumption made by the test is that replacing the
DragDropClient on the root window of the parent NativeWidget will change the
DragDropClient used by the child NativeWidget.

This check ensures this property. The parent NativeWidgetMus has a
DragDropClientMus, and the child NativeWidgetMus does not, and attempts to drag
something in the child NWMus bubble up to its parent.

(The test might also just be incorrect. Or NativeWidgetMus might not work
correctly with child widgets. Or something else that I'm not thinking of.)

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-23 22:58:44 UTC) #41

commit-bot: I haz the power

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_rel_ng/builds/285379)

4 years, 2 months ago (2016-09-23 22:58:45 UTC) #42

sky

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode72 ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer = ui::PointerEvent::kMousePointerId; On 2016/09/23 21:40:48, Elliot Glaysher ...

4 years, 2 months ago (2016-09-26 02:12:59 UTC) #43

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
File ui/views/mus/drag_drop_client_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer =
ui::PointerEvent::kMousePointerId;
On 2016/09/23 21:40:48, Elliot Glaysher wrote:
> On 2016/09/23 17:56:27, sky wrote:
> > On 2016/09/22 20:23:20, Elliot Glaysher wrote:
> > > This is the one final issue with this patch. At this point, we've been
> called
> > by
> > > either of the two methods in the above comment, and we need a pointer id.
> > > However, gesture events can have multiple pointer ids, if I'm reading a
> > comment
> > > from sadrul correctly.
> > > 
> > > The pointer used on the mus server side in drag_controller.cc to only
> respond
> > to
> > > events which have the same pointer id, to ensure if I'm dragging with the
> > mouse,
> > > pointer down and up with a finger shouldn't then release the drag.
> > 
> > On the mus side we know we are waiting on a response from the client to a
> > particular event. So, we should be able to tie the pointer id together
there,
> > right?
> 
> At this point, mus will no longer have the event which caused this, because
the
> event dispatch machinery in NativeWidgetMus creates a EventAckHandler, which
is
> a MessageLoop::NestingObserver which sends handled event acks to mus on a
nested
> message loop being created, which happens at the top of this method.
Therefore,
> by the time we even start the drag drop on the mus side,
> WindowManagerState::event_awaiting_input_ack_ is null.
> 
> (Also, implicit spooky action at a distance sucks.)
> 
> I also tried to look at the |pointer_targets_| in the EventDispatcher, but
since
> there appears to be an explicit capture taken by client during mousedown (not
> related to dragging; just normal event dispatch stuff), pointer targets gets
> cleared.
> 
> Should I add another tracking mechanism in event dispatcher that keeps track
of
> which pointers are down regardless of capture state?

Can you move crating the RunLoop until after calling PerformDragDrop? It means
you can't supply QuitClosure to the callback, but you should be able to roll
your own.

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:759: if (!params.parent) {
On 2016/09/23 21:40:48, Elliot Glaysher wrote:
> On 2016/09/23 16:14:09, sky wrote:
> > I'm unclear as to why you don't create this for every NativeWidgetMus. Every
> > NativeWidgetMus contains a hierarchy of aura::Windows with a single root and
> has
> > a ui::Window. Don't you need to process the drag for every ui::Window?
> 
> I'm not sure I believe that this is true.

See line 541, which creates the WindowTreeHost which has an Aura::Window.
content_ is addded to it.

> It appears that menus violate some of those assumptions. Without the `if
> (!params.parent)' block around setting the drag drop
client,MenuControllerTest.
> MenuControllerReplacedDuringDrag times out. The test is set up to create
> multiple NativeWidgets (a toplevel one, and one owned by MenuHost, which is a
> child), and the implicit assumption made by the test is that replacing the
> DragDropClient on the root window of the parent NativeWidget will change the
> DragDropClient used by the child NativeWidget.
> 
> This check ensures this property. The parent NativeWidgetMus has a
> DragDropClientMus, and the child NativeWidgetMus does not, and attempts to
drag
> something in the child NWMus bubble up to its parent.
> 
> (The test might also just be incorrect. Or NativeWidgetMus might not work
> correctly with child widgets. Or something else that I'm not thinking of.)

If you do end up sharing a DragDropClients, won't that mean the wrong ui::Window
is used as the source?

Elliot Glaysher

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

4 years, 2 months ago (2016-09-26 19:30:39 UTC) #44

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-26 19:31:17 UTC) #45

Elliot Glaysher

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

4 years, 2 months ago (2016-09-26 20:03:18 UTC) #46

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-26 20:04:04 UTC) #47

Elliot Glaysher

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

4 years, 2 months ago (2016-09-26 20:52:52 UTC) #48

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-26 20:53:45 UTC) #49

Elliot Glaysher

ptal https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc File ui/views/mus/drag_drop_client_mus.cc (right): https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop_client_mus.cc#newcode72 ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer = ui::PointerEvent::kMousePointerId; On 2016/09/26 02:12:59, sky ...

4 years, 2 months ago (2016-09-26 20:57:29 UTC) #50

ptal

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
File ui/views/mus/drag_drop_client_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/drag_drop...
ui/views/mus/drag_drop_client_mus.cc:72: int32_t drag_pointer =
ui::PointerEvent::kMousePointerId;
On 2016/09/26 02:12:59, sky wrote:
> On 2016/09/23 21:40:48, Elliot Glaysher wrote:
> > On 2016/09/23 17:56:27, sky wrote:
> > > On 2016/09/22 20:23:20, Elliot Glaysher wrote:
> > > > This is the one final issue with this patch. At this point, we've been
> > called
> > > by
> > > > either of the two methods in the above comment, and we need a pointer
id.
> > > > However, gesture events can have multiple pointer ids, if I'm reading a
> > > comment
> > > > from sadrul correctly.
> > > > 
> > > > The pointer used on the mus server side in drag_controller.cc to only
> > respond
> > > to
> > > > events which have the same pointer id, to ensure if I'm dragging with
the
> > > mouse,
> > > > pointer down and up with a finger shouldn't then release the drag.
> > > 
> > > On the mus side we know we are waiting on a response from the client to a
> > > particular event. So, we should be able to tie the pointer id together
> there,
> > > right?
> > 
> > At this point, mus will no longer have the event which caused this, because
> the
> > event dispatch machinery in NativeWidgetMus creates a EventAckHandler, which
> is
> > a MessageLoop::NestingObserver which sends handled event acks to mus on a
> nested
> > message loop being created, which happens at the top of this method.
> Therefore,
> > by the time we even start the drag drop on the mus side,
> > WindowManagerState::event_awaiting_input_ack_ is null.
> > 
> > (Also, implicit spooky action at a distance sucks.)
> > 
> > I also tried to look at the |pointer_targets_| in the EventDispatcher, but
> since
> > there appears to be an explicit capture taken by client during mousedown
(not
> > related to dragging; just normal event dispatch stuff), pointer targets gets
> > cleared.
> > 
> > Should I add another tracking mechanism in event dispatcher that keeps track
> of
> > which pointers are down regardless of capture state?
> 
> Can you move crating the RunLoop until after calling PerformDragDrop? It means
> you can't supply QuitClosure to the callback, but you should be able to roll
> your own.

Done, with a few additional changes. (For example, we weren't always keeping
track of the event that we were waiting on in the server side.)

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2323553002/diff/140001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:759: if (!params.parent) {
On 2016/09/26 02:12:59, sky wrote:
> On 2016/09/23 21:40:48, Elliot Glaysher wrote:
> > On 2016/09/23 16:14:09, sky wrote:
> > > I'm unclear as to why you don't create this for every NativeWidgetMus.
Every
> > > NativeWidgetMus contains a hierarchy of aura::Windows with a single root
and
> > has
> > > a ui::Window. Don't you need to process the drag for every ui::Window?
> > 
> > I'm not sure I believe that this is true.
> 
> See line 541, which creates the WindowTreeHost which has an Aura::Window.
> content_ is addded to it.
> 
> > It appears that menus violate some of those assumptions. Without the `if
> > (!params.parent)' block around setting the drag drop
> client,MenuControllerTest.
> > MenuControllerReplacedDuringDrag times out. The test is set up to create
> > multiple NativeWidgets (a toplevel one, and one owned by MenuHost, which is
a
> > child), and the implicit assumption made by the test is that replacing the
> > DragDropClient on the root window of the parent NativeWidget will change the
> > DragDropClient used by the child NativeWidget.
> > 
> > This check ensures this property. The parent NativeWidgetMus has a
> > DragDropClientMus, and the child NativeWidgetMus does not, and attempts to
> drag
> > something in the child NWMus bubble up to its parent.
> > 
> > (The test might also just be incorrect. Or NativeWidgetMus might not work
> > correctly with child widgets. Or something else that I'm not thinking of.)
> 
> If you do end up sharing a DragDropClients, won't that mean the wrong
ui::Window
> is used as the source?

Disabled the test as discussed in person.

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
services/ui/ws/window_manager_state.cc:170: drag_pointer =
event_awaiting_input_ack_->AsPointerEvent()->pointer_id();
In the server, do I have to handle things other than pointer events here? There
are no GestureEvents in the ws/ directory and the one place in
platform_display.cc which translates MouseEvents and TouchEvents to
PointerEvents.

sky

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_manager_state.cc File services/ui/ws/window_manager_state.cc (left): https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_manager_state.cc#oldcode419 services/ui/ws/window_manager_state.cc:419: event_awaiting_input_ack_ = ui::Event::Clone(event); Why do you need to do ...

4 years, 2 months ago (2016-09-26 21:44:41 UTC) #51

Elliot Glaysher

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

4 years, 2 months ago (2016-09-26 22:19:14 UTC) #52

Elliot Glaysher

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_manager_state.cc File services/ui/ws/window_manager_state.cc (left): https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_manager_state.cc#oldcode419 services/ui/ws/window_manager_state.cc:419: event_awaiting_input_ack_ = ui::Event::Clone(event); On 2016/09/26 21:44:41, sky wrote: > ...

4 years, 2 months ago (2016-09-26 22:20:02 UTC) #53

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
File services/ui/ws/window_manager_state.cc (left):

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
services/ui/ws/window_manager_state.cc:419: event_awaiting_input_ack_ =
ui::Event::Clone(event);
On 2016/09/26 21:44:41, sky wrote:
> Why do you need to do this change?

Because we need to keep track of the event awaiting input, and this isn't doing
that.

In this previous code, we're only keeping track of the event which triggered an
input if we had an accelerator associated with the action; accelerator can be
null any other time. (ie DispatchInputEventToWindow()).

This change actually brings the code into line with the comment on
event_awaiting_input_ack_ in the header.

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
services/ui/ws/window_manager_state.cc:170: drag_pointer =
event_awaiting_input_ack_->AsPointerEvent()->pointer_id();
On 2016/09/26 21:44:41, sky wrote:
> On 2016/09/26 20:57:29, Elliot Glaysher wrote:
> > In the server, do I have to handle things other than pointer events here?
> There
> > are no GestureEvents in the ws/ directory and the one place in
> > platform_display.cc which translates MouseEvents and TouchEvents to
> > PointerEvents.
> 
> You should at least check that event_awaiting_input_ack_ is a pointer event
> (IsPointerEvent()).
> Once 571688 then gestures will be possible.

Misunderstood interface; thought it returned null if AsPointer() would fail.

https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_...
services/ui/ws/window_manager_state.cc:172: NOTIMPLEMENTED() << "Set drag drop
set up during something other than a "
On 2016/09/26 21:44:41, sky wrote:
> Can the NOTIMPLEMENTED case fail? I'm ok if you want to fix up separately.

Done.

https://codereview.chromium.org/2323553002/diff/220001/ui/views/mus/drop_targ...
File ui/views/mus/drop_target_mus.h (right):

https://codereview.chromium.org/2323553002/diff/220001/ui/views/mus/drop_targ...
ui/views/mus/drop_target_mus.h:44: void Translate(uint32_t key_state,
On 2016/09/26 21:44:41, sky wrote:
> See earlier comment on this name being confusing. Can it be renamed?

Added comment as discussed in person.

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-26 22:20:03 UTC) #54

sky

LGTM https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_manager_state.cc File services/ui/ws/window_manager_state.cc (left): https://codereview.chromium.org/2323553002/diff/220001/services/ui/ws/window_manager_state.cc#oldcode419 services/ui/ws/window_manager_state.cc:419: event_awaiting_input_ack_ = ui::Event::Clone(event); On 2016/09/26 22:20:02, Elliot Glaysher ...

4 years, 2 months ago (2016-09-26 23:06:05 UTC) #55

Elliot Glaysher

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

4 years, 2 months ago (2016-09-26 23:07:56 UTC) #56

Elliot Glaysher

tsepez: mojom review which just removes one argument

4 years, 2 months ago (2016-09-26 23:07:58 UTC) #57

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-26 23:33:22 UTC) #58

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-26 23:33:23 UTC) #59

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-27 17:00:02 UTC) #62

commit-bot: I haz the power

Description was changed from ========== mash: Use the new mus drag and drop API to ...

4 years, 2 months ago (2016-09-27 17:41:54 UTC) #64

commit-bot: I haz the power

4 years, 2 months ago (2016-09-27 17:41:55 UTC) #65

Message was sent while issue was closed.

Patchset 13 (id:??) landed as
https://crrev.com/de431fe87ced556b5fa4411da711ee33453de9a2
Cr-Commit-Position: refs/heads/master@{#421254}

Issue 2323553002: mash: Use the new mus drag and drop API to get drag working in mash. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Rebase to ToT #

Patch Set 3 : Merge with parent patch. #

Patch Set 4 : Rebase to tot (maybe final merge?) #

Patch Set 5 : Selectively create DragDropClientMus to fix MenuControllerTest.MenuControllerReplacedDuringDrag #

Patch Set 6 : Rebase to ToT #

Patch Set 7 : Rebase to ToT now that the cancel patch landed #

Patch Set 8 : Patch cleanup #

Patch Set 9 : comments #

Patch Set 10 : Remove drag_pointer from the interface and look it up on the server side. #

Patch Set 11 : Disable MenuControllerTest.MenuControllerReplacedDuringDrag in mus and remove hack that made it pas… #

Patch Set 12 : another make unique #

Patch Set 13 : comments #

Messages