Find display root ServerWindow for located events in mus.

Find display root ServerWindow for located events in mus.

The EventDispatcher currently just grabs the root ServerWindow for the
first display. This doesn't work with multiple displays, so use the
root_location to find the correct Display.

BUG=625876
Committed: https://crrev.com/375bfd94aa008c34400a35c7f13fc47523c66875
Cr-Commit-Position: refs/heads/master@{#411728}

[kylechar, 2016-08-11 18:12:49]

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

[sky, 2016-08-11 20:58:04]

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/event_dispat...
File services/ui/ws/event_dispatcher_delegate.h (right):

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/event_dispat...
services/ui/ws/event_dispatcher_delegate.h:67: virtual ServerWindow*
GetRootWindowContaining(gfx::Point* location) = 0;
Update description about location. In particular that it may be updated in
place.

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:537: // with X11. These locations crop up
when windows overlap or when dragging.
I believe this will also happen on some devices that have sensors outside the
display bounds. You'll likely need a better way to pit the target_display in
this case. I'm OK with a TODO if you want to deal later.

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:539: LOG(ERROR) << "Invalid event
location " << location->ToString();
Make this a DVLOG(1) given it can happen in normal practice.

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:551: gfx::Point origin =
target_display->root_window()->bounds().origin();
The location of the root is always 0, 0. Don't you want to use the origin from
the PlatformDisplay?

[kylechar, 2016-08-12 16:09:11]

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/event_dispat...
File services/ui/ws/event_dispatcher_delegate.h (right):

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/event_dispat...
services/ui/ws/event_dispatcher_delegate.h:67: virtual ServerWindow*
GetRootWindowContaining(gfx::Point* location) = 0;
On 2016/08/11 20:58:04, sky wrote:
> Update description about location. In particular that it may be updated in
> place.

Done.

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:537: // with X11. These locations crop up
when windows overlap or when dragging.
On 2016/08/11 20:58:04, sky wrote:
> I believe this will also happen on some devices that have sensors outside the
> display bounds. You'll likely need a better way to pit the target_display in
> this case. I'm OK with a TODO if you want to deal later.

Yep, it definitely needs more work. This will avoid it crashing for now and

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:539: LOG(ERROR) << "Invalid event
location " << location->ToString();
On 2016/08/11 20:58:04, sky wrote:
> Make this a DVLOG(1) given it can happen in normal practice.

Done.

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:551: gfx::Point origin =
target_display->root_window()->bounds().origin();
On 2016/08/11 20:58:04, sky wrote:
> The location of the root is always 0, 0. Don't you want to use the origin from
> the PlatformDisplay?

I'll change it to use the PlatformDisplay bounds be consistent with what I'm
checking above.

Although, I definitely misunderstood this before and changed the Display root
ServerWindow origin in a previous CL. My reasoning was that since the Display
root ServerWindow doesn't have a parent, the origin should be in screen
coordinates and made it the same as the PlatformDisplay origin.

I'm happy to change it back so that Display root ServerWindows are all at (0,
0). Although, changing it back breaks a bunch of drawing and event handling code
with regards to the second display. With ToT this morning + this patch, drawing
on the second display is working. I spent the last hour trying to figure out
where exactly it breaks but haven't found it yet.

[sky, 2016-08-12 16:47:46]

This LGTM, please make sure root origins are 0,0 soonish though.

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2237003002/diff/1/services/ui/ws/window_manag...
services/ui/ws/window_manager_state.cc:551: gfx::Point origin =
target_display->root_window()->bounds().origin();
On 2016/08/12 16:09:11, kylechar wrote:
> On 2016/08/11 20:58:04, sky wrote:
> > The location of the root is always 0, 0. Don't you want to use the origin
from
> > the PlatformDisplay?
> 
> I'll change it to use the PlatformDisplay bounds be consistent with what I'm
> checking above.
> 
> Although, I definitely misunderstood this before and changed the Display root
> ServerWindow origin in a previous CL. My reasoning was that since the Display
> root ServerWindow doesn't have a parent, the origin should be in screen
> coordinates and made it the same as the PlatformDisplay origin.
> 
> I'm happy to change it back so that Display root ServerWindows are all at (0,
> 0). Although, changing it back breaks a bunch of drawing and event handling
code
> with regards to the second display. With ToT this morning + this patch,
drawing
> on the second display is working. I spent the last hour trying to figure out
> where exactly it breaks but haven't found it yet.

I think we need to put them at 0, 0, otherwise I worry that we're going to have
inconsistencies with what all the client code is expecting. By that I mean
existing ash code (and other places) convert to screen bounds using something
like:
convert window bounds to root
add display bounds

If the root includes the display bounds then we're going to get the wrong thing.

https://codereview.chromium.org/2237003002/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2237003002/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:552: location->Offset(-origin.x(),
-origin.y());
Typically we do something like:
*location -= origin.OffsetFromOrigin().

[kylechar, 2016-08-12 17:25:12]

Thanks sky. I'm trying to figure out what needs to change to move the origin
back to (0, 0) now.

https://codereview.chromium.org/2237003002/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2237003002/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:552: location->Offset(-origin.x(),
-origin.y());
On 2016/08/12 16:47:46, sky wrote:
> Typically we do something like:
> *location -= origin.OffsetFromOrigin().

Done.

[kylechar, 2016-08-12 17:25:19]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-08-12 17:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2237003002/#ps40001
(title: "Fix for comment.")

[commit-bot: I haz the power, 2016-08-12 17:25:36]

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

[commit-bot: I haz the power, 2016-08-12 18:18:34]

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

[commit-bot: I haz the power, 2016-08-12 18:18:35]

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

[kylechar, 2016-08-12 18:23:51]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-08-12 18:23:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2237003002/#ps60001
(title: "Fix tests.")

[commit-bot: I haz the power, 2016-08-12 18:24:18]

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

[commit-bot: I haz the power, 2016-08-12 19:22:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-12 19:24:40]

Description was changed from

==========
Find display root ServerWindow for located events in mus.

The EventDispatcher currently just grabs the root ServerWindow for the
first display. This doesn't work with multiple displays, so use the
root_location to find the correct Display.

BUG=625876
==========

to

==========
Find display root ServerWindow for located events in mus.

The EventDispatcher currently just grabs the root ServerWindow for the
first display. This doesn't work with multiple displays, so use the
root_location to find the correct Display.

BUG=625876

Committed: https://crrev.com/375bfd94aa008c34400a35c7f13fc47523c66875
Cr-Commit-Position: refs/heads/master@{#411728}
==========

[commit-bot: I haz the power, 2016-08-12 19:24:41]

Patchset 4 (id:??) landed as
https://crrev.com/375bfd94aa008c34400a35c7f13fc47523c66875
Cr-Commit-Position: refs/heads/master@{#411728}