Avoid two targeting phases in aura client-lib and EventProcessor.

Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2681613002
Cr-Original-Commit-Position: refs/heads/master@{#452393}
Committed: https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39544d541e79f2
Review-Url: https://codereview.chromium.org/2681613002
Cr-Commit-Position: refs/heads/master@{#454530}
Committed: https://chromium.googlesource.com/chromium/src/+/762dcf004c4d65c416992d3c6ad4323a9303e9f0

[riajiang, 2017-02-06 23:08:09]

Description was changed from

==========
Send target window for event-dispatch from client-lib to EventProcessor
to avoid converting event location from target window's coordinate
system to root window's coordinate system in client-lib and then convert
it back again when we try to find the target window for that event in
WindowTargeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=covered by tests
==========

to

==========
Send target window for event-dispatch from client-lib to EventProcessor
to avoid converting event location from target window's coordinate
system to root window's coordinate system in client-lib and then convert
it back again when we try to find the target window for that event in
WindowTargeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=covered by tests
     manual (--force-device-scale-factor=2)
==========

[riajiang, 2017-02-06 23:10:07]

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

[riajiang, 2017-02-06 23:10:07]

Hi sadrul@ and sky@, PTAL, thanks!

[sadrul, 2017-02-07 04:04:55]

https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1156: window->GetWindow());
This is somewhat awkward.

Can we set ui::Event::target_ from here instead? And update the code in
EventProcessor so that it doesn't use the targeter if the event already has a
target set?

https://codereview.chromium.org/2681613002/diff/1/ui/events/event_processor.cc
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/1/ui/events/event_processor.c...
ui/events/event_processor.cc:13: delete event_target_;
This shouldn't be taking ownership of the event-target.

[sky, 2017-02-07 04:19:49]

https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1156: window->GetWindow());
On 2017/02/07 04:04:54, sadrul wrote:
> This is somewhat awkward.
> 
> Can we set ui::Event::target_ from here instead? And update the code in
> EventProcessor so that it doesn't use the targeter if the event already has a
> target set?

Extremely!
What's wrong with trying to find the location again?

[sadrul, 2017-02-07 04:26:54]

On 2017/02/07 04:19:49, sky wrote:
>
https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
> File ui/aura/mus/window_tree_client.cc (right):
> 
>
https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
> ui/aura/mus/window_tree_client.cc:1156: window->GetWindow());
> On 2017/02/07 04:04:54, sadrul wrote:
> > This is somewhat awkward.
> > 
> > Can we set ui::Event::target_ from here instead? And update the code in
> > EventProcessor so that it doesn't use the targeter if the event already has
a
> > target set?
> 
> Extremely!

Not sure if you are referring to the whole idea of avoiding the targeting phase
or not :)

> What's wrong with trying to find the location again?

The client does not have all the necessary information to do a proper hit-test
(for example OOPIF with overlapping content from the embedder (think
http://smfr.org/misc/css/stacking/css-escher.html with the iframe being an
OOPIF)). Only the display-compositor in mus-gpu has all the information needed
to the find the right target (and the right location of the event for that
target). So when the client receives the target and the event from mus, it needs
to trust that, and avoid attempting to do its own hit-testing.

[riajiang, 2017-02-07 17:42:40]

https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1156: window->GetWindow());
On 2017/02/07 04:04:54, sadrul wrote:
> This is somewhat awkward.
> 
> Can we set ui::Event::target_ from here instead? And update the code in
> EventProcessor so that it doesn't use the targeter if the event already has a
> target set?

Done! I didn't know we can set the target for an event directly.
But when we create an event from another event (e.g. a mouse event from pointer
event), we don't want to copy over the target right? So I added the target each
time after we do the event conversion...

https://codereview.chromium.org/2681613002/diff/1/ui/events/event_processor.cc
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/1/ui/events/event_processor.c...
ui/events/event_processor.cc:13: delete event_target_;
On 2017/02/07 04:04:54, sadrul wrote:
> This shouldn't be taking ownership of the event-target.

Right! And no longer need this event_target_ anymore.

[riajiang, 2017-02-07 18:41:51]

Description was changed from

==========
Send target window for event-dispatch from client-lib to EventProcessor
to avoid converting event location from target window's coordinate
system to root window's coordinate system in client-lib and then convert
it back again when we try to find the target window for that event in
WindowTargeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=covered by tests
     manual (--force-device-scale-factor=2)
==========

to

==========
Set target window for event dispatching in client-lib
so that EventProcessor can use that target for event dispatching
to avoid converting event location from target window's coordinate
system to root window's coordinate system in client-lib and then convert
it back again when we try to find the target window for that event in
WindowTargeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=covered by tests
     manual (--force-device-scale-factor=2)
==========

[sky, 2017-02-07 20:14:11]

My concern with direct targeting is that there may be valid cases a
client wants some ancestor or other target in the chain to handle the
event. If we force the target to be want mus picked that may not be
honored.

  -Scott

On Mon, Feb 6, 2017 at 8:26 PM,  <sadrul@chromium.org> wrote:
> On 2017/02/07 04:19:49, sky wrote:
>>
>
https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
>> File ui/aura/mus/window_tree_client.cc (right):
>>
>>
>
https://codereview.chromium.org/2681613002/diff/1/ui/aura/mus/window_tree_cli...
>> ui/aura/mus/window_tree_client.cc:1156: window->GetWindow());
>> On 2017/02/07 04:04:54, sadrul wrote:
>> > This is somewhat awkward.
>> >
>> > Can we set ui::Event::target_ from here instead? And update the code in
>> > EventProcessor so that it doesn't use the targeter if the event already
>> > has
> a
>> > target set?
>>
>> Extremely!
>
> Not sure if you are referring to the whole idea of avoiding the targeting
> phase
> or not :)
>
>> What's wrong with trying to find the location again?
>
> The client does not have all the necessary information to do a proper
> hit-test
> (for example OOPIF with overlapping content from the embedder (think
> http://smfr.org/misc/css/stacking/css-escher.html with the iframe being an
> OOPIF)). Only the display-compositor in mus-gpu has all the information
> needed
> to the find the right target (and the right location of the event for that
> target). So when the client receives the target and the event from mus, it
> needs
> to trust that, and avoid attempting to do its own hit-testing.
>
> https://codereview.chromium.org/2681613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2017-02-08 00:19:13]

On 2017/02/07 20:14:11, sky wrote:
> My concern with direct targeting is that there may be valid cases a
> client wants some ancestor or other target in the chain to handle the
> event. If we force the target to be want mus picked that may not be
> honored.

It should still be possible to use pre-target handlers in those cases though.
That should be sufficient?

[sadrul, 2017-02-08 00:24:38]

https://codereview.chromium.org/2681613002/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1178: host->SendEventToProcessor(event.get());
This should go into some helper function, e.g.
WindowTreeClient::DispatchEventToTarget(ui::Event* event, Window* target);

https://codereview.chromium.org/2681613002/diff/20001/ui/events/event_process...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/20001/ui/events/event_process...
ui/events/event_processor.cc:33: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
We should move some code from above into here too. Something like this:

  EventTarget* target = event->target();
  if (!target) {
    EventTarget* root = GetRootTarget();
    EventTargeter* targeter = root->GetEventTargeter();
    target = targeter->FindTargetForEvent(root, event_to_dispatch);
  }

[sky, 2017-02-08 00:33:17]

There are subclasses of WindowTargeter in ash. It would be nice if we
could continue to use them. I haven't investigated to see to what
degree they make sense in mash.

Why don't we convert to DIPs when we get the event? That way
PointerObservers get dips too, which I'm pretty sure they are
expecting. I'm thinking of this:
https://codereview.chromium.org/2680043004 . It needs tests, but the
principal is there.

  -Scott

On Tue, Feb 7, 2017 at 4:24 PM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/2681613002/diff/20001/ui/aura/mus/window_tree...
> File ui/aura/mus/window_tree_client.cc (right):
>
>
https://codereview.chromium.org/2681613002/diff/20001/ui/aura/mus/window_tree...
> ui/aura/mus/window_tree_client.cc:1178:
> host->SendEventToProcessor(event.get());
> This should go into some helper function, e.g.
> WindowTreeClient::DispatchEventToTarget(ui::Event* event, Window*
> target);
>
>
https://codereview.chromium.org/2681613002/diff/20001/ui/events/event_process...
> File ui/events/event_processor.cc (right):
>
>
https://codereview.chromium.org/2681613002/diff/20001/ui/events/event_process...
> ui/events/event_processor.cc:33: target =
> targeter->FindTargetForEvent(root, event_to_dispatch);
> We should move some code from above into here too. Something like this:
>
> EventTarget* target = event->target();
> if (!target) {
> EventTarget* root = GetRootTarget();
> EventTargeter* targeter = root->GetEventTargeter();
> target = targeter->FindTargetForEvent(root, event_to_dispatch);
> }
>
> https://codereview.chromium.org/2681613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2017-02-08 00:55:40]

On 2017/02/08 00:33:17, sky wrote:
> There are subclasses of WindowTargeter in ash. It would be nice if we
> could continue to use them. I haven't investigated to see to what
> degree they make sense in mash.

OK. Let me look at those. Maybe we can make things better there.

> 
> Why don't we convert to DIPs when we get the event? That way
> PointerObservers get dips too, which I'm pretty sure they are
> expecting.

Good catch! Yes, we should be doing the conversion before the observers see the
event.

> I'm thinking of this:
> https://codereview.chromium.org/2680043004 . It needs tests, but the
> principal is there.
> 

Ria had a couple of similar approaches
(https://codereview.chromium.org/2682443002/
https://codereview.chromium.org/2677153003/). But I would prefer to avoid
targeting again in the client most of the time (except for some special cases,
as outlined in Rob's doc ... although we may not need those in aura)

[sky, 2017-02-08 03:05:34]

Ok, fair enough. I'll fix the pointer observer event coordinates.

  -Scott

On Tue, Feb 7, 2017 at 4:55 PM,  <sadrul@chromium.org> wrote:
> On 2017/02/08 00:33:17, sky wrote:
>> There are subclasses of WindowTargeter in ash. It would be nice if we
>> could continue to use them. I haven't investigated to see to what
>> degree they make sense in mash.
>
> OK. Let me look at those. Maybe we can make things better there.
>
>>
>> Why don't we convert to DIPs when we get the event? That way
>> PointerObservers get dips too, which I'm pretty sure they are
>> expecting.
>
> Good catch! Yes, we should be doing the conversion before the observers see
> the
> event.
>
>> I'm thinking of this:
>> https://codereview.chromium.org/2680043004 . It needs tests, but the
>> principal is there.
>>
>
> Ria had a couple of similar approaches
> (https://codereview.chromium.org/2682443002/
> https://codereview.chromium.org/2677153003/). But I would prefer to avoid
> targeting again in the client most of the time (except for some special
> cases,
> as outlined in Rob's doc ... although we may not need those in aura)
>
> https://codereview.chromium.org/2681613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[riajiang, 2017-02-08 19:37:42]

On 2017/02/08 03:05:34, sky wrote:
> Ok, fair enough. I'll fix the pointer observer event coordinates.
> 
>   -Scott
> 
> On Tue, Feb 7, 2017 at 4:55 PM,  <mailto:sadrul@chromium.org> wrote:
> > On 2017/02/08 00:33:17, sky wrote:
> >> There are subclasses of WindowTargeter in ash. It would be nice if we
> >> could continue to use them. I haven't investigated to see to what
> >> degree they make sense in mash.
> >
> > OK. Let me look at those. Maybe we can make things better there.
> >
> >>
> >> Why don't we convert to DIPs when we get the event? That way
> >> PointerObservers get dips too, which I'm pretty sure they are
> >> expecting.
> >
> > Good catch! Yes, we should be doing the conversion before the observers see
> > the
> > event.
> >
> >> I'm thinking of this:
> >> https://codereview.chromium.org/2680043004 . It needs tests, but the
> >> principal is there.
> >>
> >
> > Ria had a couple of similar approaches
> > (https://codereview.chromium.org/2682443002/
> > https://codereview.chromium.org/2677153003/). But I would prefer to avoid
> > targeting again in the client most of the time (except for some special
> > cases,
> > as outlined in Rob's doc ... although we may not need those in aura)
> >
> > https://codereview.chromium.org/2681613002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

As discussed with sadrul@ offline, in order to make sure both target windows set
by mus-ws and ash are honored, one proposal would be: 
1) In the client-lib, after we've received the event and the target window (T) 
from mus-ws, we walk up the tree to find the farthest ancestor (A) that has an 
EventTargeter set. We then convert the event_location from pixels to dip and
from
T's coordinate system to A's coordinate system. Set the event->target_ to be A.
2) In WindowTreeHost, we only install WindowTargeter to the root window if we
are 
not in mus.

[sky, 2017-02-08 20:06:16]

SGTM

On Wed, Feb 8, 2017 at 11:37 AM,  <riajiang@chromium.org> wrote:
> On 2017/02/08 03:05:34, sky wrote:
>> Ok, fair enough. I'll fix the pointer observer event coordinates.
>>
>> -Scott
>>
>> On Tue, Feb 7, 2017 at 4:55 PM, <mailto:sadrul@chromium.org> wrote:
>> > On 2017/02/08 00:33:17, sky wrote:
>> >> There are subclasses of WindowTargeter in ash. It would be nice if we
>> >> could continue to use them. I haven't investigated to see to what
>> >> degree they make sense in mash.
>> >
>> > OK. Let me look at those. Maybe we can make things better there.
>> >
>> >>
>> >> Why don't we convert to DIPs when we get the event? That way
>> >> PointerObservers get dips too, which I'm pretty sure they are
>> >> expecting.
>> >
>> > Good catch! Yes, we should be doing the conversion before the observers
>> > see
>> > the
>> > event.
>> >
>> >> I'm thinking of this:
>> >> https://codereview.chromium.org/2680043004 . It needs tests, but the
>> >> principal is there.
>> >>
>> >
>> > Ria had a couple of similar approaches
>> > (https://codereview.chromium.org/2682443002/
>> > https://codereview.chromium.org/2677153003/). But I would prefer to
>> > avoid
>> > targeting again in the client most of the time (except for some special
>> > cases,
>> > as outlined in Rob's doc ... although we may not need those in aura)
>> >
>> > https://codereview.chromium.org/2681613002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> As discussed with sadrul@ offline, in order to make sure both target windows
> set
> by mus-ws and ash are honored, one proposal would be:
> 1) In the client-lib, after we've received the event and the target window
> (T)
> from mus-ws, we walk up the tree to find the farthest ancestor (A) that has
> an
> EventTargeter set. We then convert the event_location from pixels to dip and
> from
> T's coordinate system to A's coordinate system. Set the event->target_ to be
> A.
> 2) In WindowTreeHost, we only install WindowTargeter to the root window if
> we
> are
> not in mus.
>
> https://codereview.chromium.org/2681613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[riajiang, 2017-02-10 17:08:35]

Patchset #3 (id:40001) has been deleted

[riajiang, 2017-02-11 00:13:11]

Patchset #3 (id:60001) has been deleted

[riajiang, 2017-02-11 00:24:37]

Patchset #3 (id:80001) has been deleted

[riajiang, 2017-02-11 00:40:26]

Description was changed from

==========
Set target window for event dispatching in client-lib
so that EventProcessor can use that target for event dispatching
to avoid converting event location from target window's coordinate
system to root window's coordinate system in client-lib and then convert
it back again when we try to find the target window for that event in
WindowTargeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=covered by tests
     manual (--force-device-scale-factor=2)
==========

to

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)
==========

[riajiang, 2017-02-11 00:43:39]

Hi sadrul@ and sky@, changed to use a different approach and updated the CL
description, could you take another look? Thanks!

https://codereview.chromium.org/2681613002/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1178: host->SendEventToProcessor(event.get());
On 2017/02/08 00:24:38, sadrul wrote:
> This should go into some helper function, e.g.
> WindowTreeClient::DispatchEventToTarget(ui::Event* event, Window* target);

Done.

[sadrul, 2017-02-13 17:05:42]

Looks good. Some nits, and maybe more tests?

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:168: ui::Event::DispatcherApi
dispatch_helper(event);
This can just take the ui::Event, and the WindowMus, as the input. It can find
the corresponding aura::Window and WindowTreeHostMus itself.

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:577: Window child2(&window_delegate);
Can you use a different WindowDelegate here, and update the test expectations
accordingly, to make sure we can test that the right window got the event?

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
ui/events/event_processor.cc:31: targeter = GetDefaultEventTargeter();
Can you add a test in event_processor_unittest.cc that exercises this code?

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
File ui/events/event_processor.h (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
ui/events/event_processor.h:25: virtual EventTargeter* GetDefaultEventTargeter()
= 0;
Document, e.g. "If the root target returned by GetRootForEvent() does not have a
targeter set, then the default targeter is used to find the target."

[riajiang, 2017-02-13 23:30:34]

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:168: ui::Event::DispatcherApi
dispatch_helper(event);
On 2017/02/13 17:05:42, sadrul wrote:
> This can just take the ui::Event, and the WindowMus, as the input. It can find
> the corresponding aura::Window and WindowTreeHostMus itself.

Done.

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:577: Window child2(&window_delegate);
On 2017/02/13 17:05:42, sadrul wrote:
> Can you use a different WindowDelegate here, and update the test expectations
> accordingly, to make sure we can test that the right window got the event?

Done.

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
ui/events/event_processor.cc:31: targeter = GetDefaultEventTargeter();
On 2017/02/13 17:05:42, sadrul wrote:
> Can you add a test in event_processor_unittest.cc that exercises this code?

Added tests for this part of the code in
WindowTreeClientClientTest.InputEventFindTargetAndConversion case 2 and 3. Also
added test for custom WindowTargeter.

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
File ui/events/event_processor.h (right):

https://codereview.chromium.org/2681613002/diff/100001/ui/events/event_proces...
ui/events/event_processor.h:25: virtual EventTargeter* GetDefaultEventTargeter()
= 0;
On 2017/02/13 17:05:42, sadrul wrote:
> Document, e.g. "If the root target returned by GetRootForEvent() does not have
a
> targeter set, then the default targeter is used to find the target."

Done.

[sadrul, 2017-02-14 21:40:10]

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1174: DCHECK(host);
|host| is no longer used I think? Remove.

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:624: // which will send the event to
child2.
Because mus-ws sends the event to client1, and none of its ancestor window has a
targeter, we should actually send the event to client1, not client2.

https://codereview.chromium.org/2681613002/diff/120001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/120001/ui/events/event_proces...
ui/events/event_processor.cc:33: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
Hm. This means we are always using the local targeter. But this is not what we
want, right? When mus gives us a target, and none of the ancestor has a
targeter, we should just use that target.

[riajiang, 2017-02-15 18:09:28]

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1174: DCHECK(host);
On 2017/02/14 21:40:10, sadrul wrote:
> |host| is no longer used I think? Remove.

True. Done.

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/120001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:624: // which will send the event to
child2.
On 2017/02/14 21:40:10, sadrul wrote:
> Because mus-ws sends the event to client1, and none of its ancestor window has
a
> targeter, we should actually send the event to client1, not client2.

Changed the logic in EventProcessor and updated the expected results.

https://codereview.chromium.org/2681613002/diff/120001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/120001/ui/events/event_proces...
ui/events/event_processor.cc:33: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
On 2017/02/14 21:40:10, sadrul wrote:
> Hm. This means we are always using the local targeter. But this is not what we
> want, right? When mus gives us a target, and none of the ancestor has a
> targeter, we should just use that target.

Makes sense. I added one more check to see if there was an event target set,
we'll use that target if there was no targeter but a target?

[sky, 2017-02-15 20:47:18]

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:569:
TEST_F(WindowTreeClientClientTest, InputEventFindTargetAndConversion) {
Please add coverage of a case with a scale factor of 2 as well.

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/test/test_wind...
File ui/aura/test/test_window_targeter.h (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/test/test_wind...
ui/aura/test/test_window_targeter.h:15: class TestWindowTargeter : public
WindowTargeter {
Please add description.

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:429: ui::EventTarget* target =
event->target();
Using ui::EventTarget rather the Window makes this code slightly more confusing.
Given we know the target is a window, can you use Window* here rather than
ui::EventTarget? This means you won't need the casts below.

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:263: if (Env::GetInstance()->mode() ==
Env::Mode::LOCAL) {
Is it possible to remove this code and change Env to always create a
WindowTargeter?

https://codereview.chromium.org/2681613002/diff/140001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/events/event_proces...
ui/events/event_processor.cc:35: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
This duplicates line 33. I think it would be easier if you closed the if started
on line 30 at line 32 and moved the if the starts on line 32 after it, e.g.
if (!targeter)
  targeter = ...
if (!target)
  target = ...

[riajiang, 2017-02-16 00:34:08]

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:569:
TEST_F(WindowTreeClientClientTest, InputEventFindTargetAndConversion) {
On 2017/02/15 20:47:17, sky wrote:
> Please add coverage of a case with a scale factor of 2 as well.

Done.

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/test/test_wind...
File ui/aura/test/test_window_targeter.h (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/test/test_wind...
ui/aura/test/test_window_targeter.h:15: class TestWindowTargeter : public
WindowTargeter {
On 2017/02/15 20:47:17, sky wrote:
> Please add description.

Done.

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:429: ui::EventTarget* target =
event->target();
On 2017/02/15 20:47:17, sky wrote:
> Using ui::EventTarget rather the Window makes this code slightly more
confusing.
> Given we know the target is a window, can you use Window* here rather than
> ui::EventTarget? This means you won't need the casts below.

This function returns ui::EventTarget so I think maybe we can keep using
EventTarget and only cast when it's Window related functions
(Window::ConvertPointToTarget)? We also want to use GetEventTargeter and
GetParentTarget for the EventTarget which would require a cast if it's a Window?

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:263: if (Env::GetInstance()->mode() ==
Env::Mode::LOCAL) {
On 2017/02/15 20:47:17, sky wrote:
> Is it possible to remove this code and change Env to always create a
> WindowTargeter?

We are only creating a targeter for the root window in non-mus mode here because
we wanted to avoid walking all the way up to the root window just to use this
default event targeter when we try to find the farthest ancestor with a targeter
set in WindowEventDispatcher::GetRootForEvent. WDYT?

https://codereview.chromium.org/2681613002/diff/140001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/events/event_proces...
ui/events/event_processor.cc:35: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
On 2017/02/15 20:47:17, sky wrote:
> This duplicates line 33. I think it would be easier if you closed the if
started
> on line 30 at line 32 and moved the if the starts on line 32 after it, e.g.
> if (!targeter)
>   targeter = ...
> if (!target)
>   target = ...

I'm not sure if I understood this. There's a case when we have both target and
targeter (mus-ws sets the target but ash wanted to use that custom targeter), we
do want to update the target using this custom targeter in that case?

[sky, 2017-02-16 01:03:02]

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:429: ui::EventTarget* target =
event->target();
On 2017/02/16 00:34:08, riajiang wrote:
> On 2017/02/15 20:47:17, sky wrote:
> > Using ui::EventTarget rather the Window makes this code slightly more
> confusing.
> > Given we know the target is a window, can you use Window* here rather than
> > ui::EventTarget? This means you won't need the casts below.
> 
> This function returns ui::EventTarget so I think maybe we can keep using
> EventTarget and only cast when it's Window related functions
> (Window::ConvertPointToTarget)? We also want to use GetEventTargeter and
> GetParentTarget for the EventTarget which would require a cast if it's a
Window?

Ugh! This is one of those places made painful by making the implementation of
the interface private. I would find this code easier to read if it were:

for (aura::Window* ancestor = target->parent(); ancestor; ancestor =
ancestor->parent()) {
*  if (ancestor->GetEventTarget()) ...
}

But * fails because the implementation of GetEventTarget is private.

This is made more awkward because you compare ancestor_with_targeter (an
EventTarger) to window() (an aura::Window). ui::EventTarget *has* to be the
aura::Window.

*SIGH*

Ok, leave what you have.

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:435: if (ancestor == window())
Is this check really necessary?

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:263: if (Env::GetInstance()->mode() ==
Env::Mode::LOCAL) {
On 2017/02/16 00:34:08, riajiang wrote:
> On 2017/02/15 20:47:17, sky wrote:
> > Is it possible to remove this code and change Env to always create a
> > WindowTargeter?
> 
> We are only creating a targeter for the root window in non-mus mode here
because
> we wanted to avoid walking all the way up to the root window just to use this
> default event targeter when we try to find the farthest ancestor with a
targeter
> set in WindowEventDispatcher::GetRootForEvent. WDYT?

The walking only happens in the mus case though, and I'm referring to the local
case. Maybe I missing something? What tree walking are you referring to?

Correct me if I'm wrong, but if you remove this code, and you make
WindowEventDispatcher always create the event_targeter_, then isn't that the
only changes that are needed? The local case won't do the tree walking as you
have it now.

https://codereview.chromium.org/2681613002/diff/140001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/events/event_proces...
ui/events/event_processor.cc:35: target = targeter->FindTargetForEvent(root,
event_to_dispatch);
On 2017/02/16 00:34:08, riajiang wrote:
> On 2017/02/15 20:47:17, sky wrote:
> > This duplicates line 33. I think it would be easier if you closed the if
> started
> > on line 30 at line 32 and moved the if the starts on line 32 after it, e.g.
> > if (!targeter)
> >   targeter = ...
> > if (!target)
> >   target = ...
> 
> I'm not sure if I understood this. There's a case when we have both target and
> targeter (mus-ws sets the target but ash wanted to use that custom targeter),
we
> do want to update the target using this custom targeter in that case?

Ah, I see it now. Thanks.

https://codereview.chromium.org/2681613002/diff/160001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/160001/ui/events/event_proces...
ui/events/event_processor.cc:24: EventTarget* target = event->target();
The old code set the target to null here, which matters for the case of the
event already handled. Are you sure we don't need that?

[riajiang, 2017-02-16 19:10:36]

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:435: if (ancestor == window())
On 2017/02/16 01:03:02, sky wrote:
> Is this check really necessary?

Because GetParentTarget() will go all the way up to ash::Shell and aura::Env
(which have NOTREACHED in GetEventTargeter) but we only wanted to walk up to the
root widow?

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2681613002/diff/140001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:263: if (Env::GetInstance()->mode() ==
Env::Mode::LOCAL) {
On 2017/02/16 01:03:02, sky wrote:
> On 2017/02/16 00:34:08, riajiang wrote:
> > On 2017/02/15 20:47:17, sky wrote:
> > > Is it possible to remove this code and change Env to always create a
> > > WindowTargeter?
> > 
> > We are only creating a targeter for the root window in non-mus mode here
> because
> > we wanted to avoid walking all the way up to the root window just to use
this
> > default event targeter when we try to find the farthest ancestor with a
> targeter
> > set in WindowEventDispatcher::GetRootForEvent. WDYT?
> 
> The walking only happens in the mus case though, and I'm referring to the
local
> case. Maybe I missing something? What tree walking are you referring to?
> 
> Correct me if I'm wrong, but if you remove this code, and you make
> WindowEventDispatcher always create the event_targeter_, then isn't that the
> only changes that are needed? The local case won't do the tree walking as you
> have it now.

Oh I see, thanks for clarifying! You mean also getting the default targeter from
WindowEventDispatcher in EventProcessor for local case? I removed the code here
and changed WindowEventDispatcher to always create the targeter; EventProcessor
will then get the default targeter for the local case as well.

https://codereview.chromium.org/2681613002/diff/160001/ui/events/event_proces...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/2681613002/diff/160001/ui/events/event_proces...
ui/events/event_processor.cc:24: EventTarget* target = event->target();
On 2017/02/16 01:03:02, sky wrote:
> The old code set the target to null here, which matters for the case of the
> event already handled. Are you sure we don't need that?

Ah true! I moved it to be inside the case where event hasn't been handled yet.

[sky, 2017-02-16 21:40:44]

LGTM

[riajiang, 2017-02-16 21:46:17]

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

[commit-bot: I haz the power, 2017-02-16 21:47:10]

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, 2017-02-16 22:11:13]

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

[commit-bot: I haz the power, 2017-02-16 22:11:15]

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_...)

[riajiang, 2017-02-21 22:21:27]

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

[commit-bot: I haz the power, 2017-02-21 22:22:10]

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, 2017-02-22 00:25:12]

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

[commit-bot: I haz the power, 2017-02-22 00:25:14]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[sadrul, 2017-02-23 03:20:41]

lgtm

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:613:
EXPECT_FALSE(window_delegate1.was_acked());
Remove the was_acked() parts. I don't think those are relevant here.

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:1832: gfx::ConvertPointToDIP(2.0f,
event_location_in_pixels);
hard-code event_location_in_dip (instead of using ConvertPointToDIP)

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher_unittest.cc:2759: EXPECT_EQ(gfx::Point(0, 0),
Env::GetInstance()->last_mouse_location());
Remove these, since these look unrelated.

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher_unittest.cc:2787: EXPECT_EQ(gfx::Point(0, 0),
Env::GetInstance()->last_mouse_location());
ditto

https://codereview.chromium.org/2681613002/diff/200001/ui/views/touchui/touch...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/200001/ui/views/touchui/touch...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:799:
gfx::NativeWindow root = GetContext();
aura::Window

[riajiang, 2017-02-23 03:46:01]

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:613:
EXPECT_FALSE(window_delegate1.was_acked());
On 2017/02/23 03:20:40, sadrul (OOO) wrote:
> Remove the was_acked() parts. I don't think those are relevant here.

Done.

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client_unittest.cc:1832: gfx::ConvertPointToDIP(2.0f,
event_location_in_pixels);
On 2017/02/23 03:20:40, sadrul (OOO) wrote:
> hard-code event_location_in_dip (instead of using ConvertPointToDIP)

Done.

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher_unittest.cc:2759: EXPECT_EQ(gfx::Point(0, 0),
Env::GetInstance()->last_mouse_location());
On 2017/02/23 03:20:40, sadrul (OOO) wrote:
> Remove these, since these look unrelated.

Done.

https://codereview.chromium.org/2681613002/diff/200001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher_unittest.cc:2787: EXPECT_EQ(gfx::Point(0, 0),
Env::GetInstance()->last_mouse_location());
On 2017/02/23 03:20:40, sadrul (OOO) wrote:
> ditto

Done.

https://codereview.chromium.org/2681613002/diff/200001/ui/views/touchui/touch...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2681613002/diff/200001/ui/views/touchui/touch...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:799:
gfx::NativeWindow root = GetContext();
On 2017/02/23 03:20:40, sadrul (OOO) wrote:
> aura::Window

Done.

[riajiang, 2017-02-23 03:46:11]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2017-02-23 03:46:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2681613002/#ps220001
(title: "test")

[commit-bot: I haz the power, 2017-02-23 03:46:37]

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, 2017-02-23 04:56:38]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487821571541870,
"parent_rev": "73b3f2563cca092c6ddbe6a9f3a87fdbd30e798e", "commit_rev":
"6b3d9805494f966193ad47181d39544d541e79f2"}

[commit-bot: I haz the power, 2017-02-23 04:57:17]

Description was changed from

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)
==========

to

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2681613002
Cr-Commit-Position: refs/heads/master@{#452393}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39...
==========

[commit-bot: I haz the power, 2017-02-23 04:57:19]

Committed patchset #9 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39...

[riajiang, 2017-02-23 23:57:38]

A revert of this CL (patchset #9 id:220001) has been created in
https://codereview.chromium.org/2715743005/ by riajiang@chromium.org.

The reason for reverting is: This patch broke window resizing (specifically when
making windows bigger and when mouse event location is outside the window bounds
for certain distance) in mash. Will fix this before relanding..

[riajiang, 2017-02-24 19:18:42]

Description was changed from

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2681613002
Cr-Commit-Position: refs/heads/master@{#452393}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39...
==========

to

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2681613002
Cr-Commit-Position: refs/heads/master@{#452393}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39...
==========

[riajiang, 2017-02-24 19:44:11]

Resizing was broken because we were skipping
WindowTargeter::FindTargetInRootWindow
(https://cs.chromium.org/chromium/src/ui/aura/window_targeter.cc?type=cs&l=133)
(since we don't look for targets starting from root anymore with this targeting
change) which would have sent the event to the capture window directly. 

With this targeting change CL, mus-ws is still giving us the correct window (in
this case the capture window), however when we use the custom targeter (in this
case ash::wm::ResizeHandleWindowTargeter) to find the target
(https://cs.chromium.org/chromium/src/ui/aura/window_targeter.cc?gsn=FindTarge...),
we don't consider capture window anymore so when the mouse event gets too far
from the window border, eventually WindowTargeter::EventLocationInsideBounds
(https://cs.chromium.org/chromium/src/ui/aura/window_targeter.cc?gsn=FindTarge...)
would fail so we can't send the event to the correct window. 

We could potentially change the check for root window
(https://cs.chromium.org/chromium/src/ui/aura/window_targeter.cc?type=cs&l=24)
to be for topmost window that has a targeter so that
WindowTargeter::FindTargetInRootWindow can still be used but it's going to get
ugly? 

As discussed with sadrul@ offline, right now I just copied the logic in
WindowTargeter::FindTargetInRootWindow to WindowEventDispatcher::GetRootForEvent
to consider capture window first, which solved the resizing problem. WDYT? Could
you take a look at patch 10? Thanks!

[sky, 2017-02-24 21:22:55]

This seems reasonable to me. Did you add test coverage of this case? The changes
from what was landed look solely in aura. If I'm right, Sadrul should review
these changes.

https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:459: // All events should be directed towards
the capture window (if any).
Should this come before the scroll-event/mouse-event check?

[riajiang, 2017-02-27 22:59:30]

Added a test for the capture window case, a TODO and a small change in
EventProcessor to store result of GetRootForEvent in target. PTAL. Thanks!

https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:459: // All events should be directed towards
the capture window (if any).
On 2017/02/24 21:22:54, sky wrote:
> Should this come before the scroll-event/mouse-event check?

If there's a mouse_pressed_handler set then that handler/window would be the
capture window? This is from WindowTargeter::FindTargetInRootWindow (added
TODO).

[sky, 2017-02-27 23:58:11]

One question. How is the capture window handled in aura-local mode?
Why do we have to special case it now?

On Mon, Feb 27, 2017 at 2:59 PM,  <riajiang@chromium.org> wrote:
> Added a test for the capture window case, a TODO and a small change in
> EventProcessor to store result of GetRootForEvent in target. PTAL. Thanks!
>
>
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
> File ui/aura/window_event_dispatcher.cc (right):
>
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
> ui/aura/window_event_dispatcher.cc:459: // All events should be directed
> towards the capture window (if any).
> On 2017/02/24 21:22:54, sky wrote:
>> Should this come before the scroll-event/mouse-event check?
>
> If there's a mouse_pressed_handler set then that handler/window would be
> the capture window? This is from WindowTargeter::FindTargetInRootWindow
> (added TODO).
>
> https://codereview.chromium.org/2681613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[riajiang, 2017-02-28 05:22:17]

On 2017/02/27 23:58:11, sky wrote:
> One question. How is the capture window handled in aura-local mode?
> Why do we have to special case it now?
> 
> On Mon, Feb 27, 2017 at 2:59 PM,  <mailto:riajiang@chromium.org> wrote:
> > Added a test for the capture window case, a TODO and a small change in
> > EventProcessor to store result of GetRootForEvent in target. PTAL. Thanks!
> >
> >
> >
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
> > File ui/aura/window_event_dispatcher.cc (right):
> >
> >
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
> > ui/aura/window_event_dispatcher.cc:459: // All events should be directed
> > towards the capture window (if any).
> > On 2017/02/24 21:22:54, sky wrote:
> >> Should this come before the scroll-event/mouse-event check?
> >
> > If there's a mouse_pressed_handler set then that handler/window would be
> > the capture window? This is from WindowTargeter::FindTargetInRootWindow
> > (added TODO).
> >
> > https://codereview.chromium.org/2681613002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

In aura-local mode, we always look for the target window starting from the root
window which would go through WindowTargeter::FindTargetInRootWindow, in which
we deal with capture window etc. (the logic I'm copying from to
WindowEventDispatcher::GetRootForEvent). But since we don't start from the root
window anymore with this CL, WindowTargeter::FindTargetInRootWindow is not being
called and that's why the logic is moved to
WindowEventDispatcher::GetRootForEvent.

[sky, 2017-02-28 17:22:27]

Got it. Should the logic be moved to the same place for both now?

On Mon, Feb 27, 2017 at 9:22 PM,  <riajiang@chromium.org> wrote:
> On 2017/02/27 23:58:11, sky wrote:
>> One question. How is the capture window handled in aura-local mode?
>> Why do we have to special case it now?
>>
>> On Mon, Feb 27, 2017 at 2:59 PM, <mailto:riajiang@chromium.org> wrote:
>> > Added a test for the capture window case, a TODO and a small change in
>> > EventProcessor to store result of GetRootForEvent in target. PTAL.
>> > Thanks!
>> >
>> >
>> >
>>
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
>> > File ui/aura/window_event_dispatcher.cc (right):
>> >
>> >
>>
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
>> > ui/aura/window_event_dispatcher.cc:459: // All events should be directed
>> > towards the capture window (if any).
>> > On 2017/02/24 21:22:54, sky wrote:
>> >> Should this come before the scroll-event/mouse-event check?
>> >
>> > If there's a mouse_pressed_handler set then that handler/window would be
>> > the capture window? This is from WindowTargeter::FindTargetInRootWindow
>> > (added TODO).
>> >
>> > https://codereview.chromium.org/2681613002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> In aura-local mode, we always look for the target window starting from the
> root
> window which would go through WindowTargeter::FindTargetInRootWindow, in
> which
> we deal with capture window etc. (the logic I'm copying from to
> WindowEventDispatcher::GetRootForEvent). But since we don't start from the
> root
> window anymore with this CL, WindowTargeter::FindTargetInRootWindow is not
> being
> called and that's why the logic is moved to
> WindowEventDispatcher::GetRootForEvent.
>
> https://codereview.chromium.org/2681613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[riajiang, 2017-02-28 22:00:13]

On 2017/02/28 17:22:27, sky wrote:
> Got it. Should the logic be moved to the same place for both now?
> 
> On Mon, Feb 27, 2017 at 9:22 PM,  <mailto:riajiang@chromium.org> wrote:
> > On 2017/02/27 23:58:11, sky wrote:
> >> One question. How is the capture window handled in aura-local mode?
> >> Why do we have to special case it now?
> >>
> >> On Mon, Feb 27, 2017 at 2:59 PM, <mailto:riajiang@chromium.org> wrote:
> >> > Added a test for the capture window case, a TODO and a small change in
> >> > EventProcessor to store result of GetRootForEvent in target. PTAL.
> >> > Thanks!
> >> >
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
> >> > File ui/aura/window_event_dispatcher.cc (right):
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/2681613002/diff/240001/ui/aura/window_event_d...
> >> > ui/aura/window_event_dispatcher.cc:459: // All events should be directed
> >> > towards the capture window (if any).
> >> > On 2017/02/24 21:22:54, sky wrote:
> >> >> Should this come before the scroll-event/mouse-event check?
> >> >
> >> > If there's a mouse_pressed_handler set then that handler/window would be
> >> > the capture window? This is from WindowTargeter::FindTargetInRootWindow
> >> > (added TODO).
> >> >
> >> > https://codereview.chromium.org/2681613002/
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "Chromium-reviews" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> > email
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > In aura-local mode, we always look for the target window starting from the
> > root
> > window which would go through WindowTargeter::FindTargetInRootWindow, in
> > which
> > we deal with capture window etc. (the logic I'm copying from to
> > WindowEventDispatcher::GetRootForEvent). But since we don't start from the
> > root
> > window anymore with this CL, WindowTargeter::FindTargetInRootWindow is not
> > being
> > called and that's why the logic is moved to
> > WindowEventDispatcher::GetRootForEvent.
> >
> > https://codereview.chromium.org/2681613002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Deleted the duplicated logic from WindowEventDispatcher::GetRootForEvent.
Changed it to make WindowTargeter::FindTargetInRootWindow public and call it
through the default window targeter that WindowEventDispatcher owns. Both cases
should go through WindowTargeter::FindTargetInRootWindow now.

[sadrul, 2017-03-01 19:12:36]

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

[commit-bot: I haz the power, 2017-03-01 19:13:13]

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

[riajiang, 2017-03-01 23:15:57]

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

[commit-bot: I haz the power, 2017-03-01 23:16:24]

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, 2017-03-02 00:06:30]

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

[commit-bot: I haz the power, 2017-03-02 00:06:31]

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_...)

[riajiang, 2017-03-02 19:58:28]

Patchset #14 (id:320001) has been deleted

[riajiang, 2017-03-02 19:59:19]

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

[commit-bot: I haz the power, 2017-03-02 19:59:50]

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, 2017-03-02 21:03:11]

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

[commit-bot: I haz the power, 2017-03-02 21:03:12]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-03 02:33:20]

lgtm

https://codereview.chromium.org/2681613002/diff/310028/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/310028/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:66: if (target != event_target &&
event->IsLocatedEvent()) {
early out instead.

[sky, 2017-03-03 04:15:13]

The parts I reviewed SLGTM. I look forward to seeing this landed!

[riajiang, 2017-03-03 04:53:08]

https://codereview.chromium.org/2681613002/diff/310028/ui/aura/window_event_d...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/2681613002/diff/310028/ui/aura/window_event_d...
ui/aura/window_event_dispatcher.cc:66: if (target != event_target &&
event->IsLocatedEvent()) {
On 2017/03/03 02:33:20, sadrul wrote:
> early out instead.

Done.

[riajiang, 2017-03-03 04:53:18]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2017-03-03 04:53:18]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2681613002/#ps350001
(title: "early return")

[commit-bot: I haz the power, 2017-03-03 04:53:29]

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, 2017-03-03 06:24:00]

CQ is committing da patch.

Bot data: {"patchset_id": 350001, "attempt_start_ts": 1488516798172610,
"parent_rev": "afe2c5b5aea22851679d7edd9fdbc850228ca911", "commit_rev":
"762dcf004c4d65c416992d3c6ad4323a9303e9f0"}

[commit-bot: I haz the power, 2017-03-03 06:24:59]

Description was changed from

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2681613002
Cr-Commit-Position: refs/heads/master@{#452393}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39...
==========

to

==========
Avoid two targeting phases in aura client-lib and EventProcessor.

1. Client-lib now skips the step where it converted the event location from
target window's coordinate system to root window's coordinate system; and
sets the target window received from mus-ws to be the target for that event.

2. Added two virtual functions in EventProcessor responsible for getting the
window with the right targeter (either root window or the farthest ancestor
with a targeter set) and for getting the default targeter (WindowTargeter)
respectively.

3. WindowEventDispatcher now is responsible for finding the right target and
doing the conversion between their coordinate systems.

4. WindowTreeHost only sets a targeter for the root window in non-mus mode 
so that we don't end up walking all the way up to the root window just to use
the default event targeter.

This also solves the bug where we were using DIP for conversion in the
client-lib but event location was still in pixels at that time.

BUG=687700
TEST=aura_unittests
     manual (--force-device-scale-factor=2)

Review-Url: https://codereview.chromium.org/2681613002
Cr-Original-Commit-Position: refs/heads/master@{#452393}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b3d9805494f966193ad47181d39...
Review-Url: https://codereview.chromium.org/2681613002
Cr-Commit-Position: refs/heads/master@{#454530}
Committed:
https://chromium.googlesource.com/chromium/src/+/762dcf004c4d65c416992d3c6ad4...
==========

[commit-bot: I haz the power, 2017-03-03 06:25:00]

Committed patchset #15 (id:350001) as
https://chromium.googlesource.com/chromium/src/+/762dcf004c4d65c416992d3c6ad4...