ozone/platform/wayland: Implement pointer handling

ozone/platform/wayland: Implement pointer handling

Only version 4 of wl_pointer is handled currently for simplicity. Version 5 was released only recently in wayland-1.10, and compositors should support both versions.

BUG=578890
Committed: https://crrev.com/1329450837ca9818a2da0210f681b4cb037f3e08
Cr-Commit-Position: refs/heads/master@{#380516}

[Michael Forney, 2016-02-23 00:46:35]

Description was changed from

==========
ozone/platform/wayland: Implement pointer handling

BUG=578890
==========

to

==========
ozone/platform/wayland: Implement pointer handling

Only version 4 of wl_pointer is handled currently for simplicity. Version 5 was
released only recently in wayland-1.10, and compositors should support both
versions.

BUG=578890
==========

[Michael Forney, 2016-02-23 00:48:23]

forney@google.com changed reviewers:
+ rjkroege@chromium.org, spang@chromium.org

[spang, 2016-02-24 16:42:21]

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer.cc:61:
base::TimeDelta::FromMilliseconds(time), pointer->flags_, 0);
Which clock does wayland use for events?

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer.cc:129:
offset.set_x(wl_fixed_to_double(value) / kAxisValueScale *
Why under "else"?

Surely |axis| is set for horizontal scrolling too?

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer_unittest.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer_unittest.cc:78:
wl_pointer_send_button(pointer->resource(), 3, 1003, BTN_LEFT,
How can you get a button when the pointer isn't over the window?

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_window_unittest.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_window_unittest.cc:119: EF_LEFT_MOUSE_BUTTON);
It should go on the class.

[Michael Forney, 2016-02-24 19:32:21]

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer.cc:61:
base::TimeDelta::FromMilliseconds(time), pointer->flags_, 0);
On 2016/02/24 16:42:21, spang wrote:
> Which clock does wayland use for events?

The spec says

"The time argument is a timestamp with millisecond granularity, with an
undefined base."

In practice, for compositors that use libinput with evdev, this comes from
CLOCK_MONOTONIC.

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer.cc:129:
offset.set_x(wl_fixed_to_double(value) / kAxisValueScale *
On 2016/02/24 16:42:21, spang wrote:
> Why under "else"?
> 
> Surely |axis| is set for horizontal scrolling too?

Done.

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer_unittest.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer_unittest.cc:78:
wl_pointer_send_button(pointer->resource(), 3, 1003, BTN_LEFT,
On 2016/02/24 16:42:21, spang wrote:
> How can you get a button when the pointer isn't over the window?

If there are multiple platform windows created, some other window might have the
focus. The point of the test is to make sure that WaylandPointer unsets the
pointer focus of a window when it receives a "leave" event for that window.

I could add create another WaylandWindow for this test, and send an enter event
for it, but this would not affect any of the code being tested (it would just
make the scenario more realistic).

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_window_unittest.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_window_unittest.cc:119: EF_LEFT_MOUSE_BUTTON);
On 2016/02/24 16:42:21, spang wrote:
> It should go on the class.

Done.

[spang, 2016-02-25 23:14:32]

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer.cc:61:
base::TimeDelta::FromMilliseconds(time), pointer->flags_, 0);
On 2016/02/24 19:32:21, Michael Forney wrote:
> On 2016/02/24 16:42:21, spang wrote:
> > Which clock does wayland use for events?
> 
> The spec says
> 
> "The time argument is a timestamp with millisecond granularity, with an
> undefined base."
> 
> In practice, for compositors that use libinput with evdev, this comes from
> CLOCK_MONOTONIC.

Ok good, we require CLOCK_MONOTONIC. Compositors that don't use it will break
chrome.

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer_unittest.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer_unittest.cc:78:
wl_pointer_send_button(pointer->resource(), 3, 1003, BTN_LEFT,
On 2016/02/24 19:32:21, Michael Forney wrote:
> On 2016/02/24 16:42:21, spang wrote:
> > How can you get a button when the pointer isn't over the window?
> 
> If there are multiple platform windows created, some other window might have
the
> focus. The point of the test is to make sure that WaylandPointer unsets the
> pointer focus of a window when it receives a "leave" event for that window.
> 
> I could add create another WaylandWindow for this test, and send an enter
event
> for it, but this would not affect any of the code being tested (it would just
> make the scenario more realistic).

Ok, let's do the 2 window test then so it's clear what the test is for.

[Michael Forney, 2016-02-26 01:25:53]

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_pointer_unittest.cc (right):

https://codereview.chromium.org/1712103002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_pointer_unittest.cc:78:
wl_pointer_send_button(pointer->resource(), 3, 1003, BTN_LEFT,
On 2016/02/25 23:14:32, spang wrote:
> On 2016/02/24 19:32:21, Michael Forney wrote:
> > On 2016/02/24 16:42:21, spang wrote:
> > > How can you get a button when the pointer isn't over the window?
> > 
> > If there are multiple platform windows created, some other window might have
> the
> > focus. The point of the test is to make sure that WaylandPointer unsets the
> > pointer focus of a window when it receives a "leave" event for that window.
> > 
> > I could add create another WaylandWindow for this test, and send an enter
> event
> > for it, but this would not affect any of the code being tested (it would
just
> > make the scenario more realistic).
> 
> Ok, let's do the 2 window test then so it's clear what the test is for.

Done.

This also helped me find a bug where we might try to look up the WaylandWindow
for a NULL wl_surface. Even though the event does not have "allow-null" for the
surface parameter, it could be NULL if the client destroys it before it
processes the event. Internally, libwayland-client replaces the object with a
global "zombie" object, and then replaces zombies with NULL when dispatching
events.

[spang, 2016-02-26 01:41:11]

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_display.cc (left):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_display.cc:86:
DCHECK(base::MessageLoopForUI::IsCurrent());
I hope this cannot get called off the UI thread.. so why did it get relaxed from
a DCHECK to a conditional?

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_display.cc (right):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_display.cc:179: display->pointer_.reset();
How could the capability get revoked?

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_pointer.cc (right):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_pointer.cc:40: if (surface)
When is is null?

[Michael Forney, 2016-02-26 01:47:34]

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_display.cc (left):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_display.cc:86:
DCHECK(base::MessageLoopForUI::IsCurrent());
On 2016/02/26 01:41:11, spang wrote:
> I hope this cannot get called off the UI thread.. so why did it get relaxed
from
> a DCHECK to a conditional?

I changed it to a conditional because this gets called from the tests when a new
WaylandWindow is created.

Maybe there is a better way to deal with this?

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_display.cc (right):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_display.cc:179: display->pointer_.reset();
On 2016/02/26 01:41:11, spang wrote:
> How could the capability get revoked?

The user unplugs their mouse, and seat no longer has a pointer.

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_pointer.cc (right):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_pointer.cc:40: if (surface)
On 2016/02/26 01:41:11, spang wrote:
> When is is null?

See my comment in the previous email.

[spang, 2016-02-26 03:51:45]

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_display.cc (left):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_display.cc:86:
DCHECK(base::MessageLoopForUI::IsCurrent());
On 2016/02/26 01:47:34, Michael Forney wrote:
> On 2016/02/26 01:41:11, spang wrote:
> > I hope this cannot get called off the UI thread.. so why did it get relaxed
> from
> > a DCHECK to a conditional?
> 
> I changed it to a conditional because this gets called from the tests when a
new
> WaylandWindow is created.
> 
> Maybe there is a better way to deal with this?

Sure, create a UI message loop from the test before creating the window.

Ideally the test would actually use the message loop, too, so that it fully
tests the IO handling. You used wl_display_roundtrip() instead but it is not
completely realistic.

Anyway, simply creating the loop would avoid having special test-only handling
here.

[Michael Forney, 2016-02-27 00:01:37]

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
File ui/ozone/platform/wayland/wayland_display.cc (left):

https://codereview.chromium.org/1712103002/diff/100001/ui/ozone/platform/wayl...
ui/ozone/platform/wayland/wayland_display.cc:86:
DCHECK(base::MessageLoopForUI::IsCurrent());
On 2016/02/26 03:51:45, spang wrote:
> On 2016/02/26 01:47:34, Michael Forney wrote:
> > On 2016/02/26 01:41:11, spang wrote:
> > > I hope this cannot get called off the UI thread.. so why did it get
relaxed
> > from
> > > a DCHECK to a conditional?
> > 
> > I changed it to a conditional because this gets called from the tests when a
> new
> > WaylandWindow is created.
> > 
> > Maybe there is a better way to deal with this?
> 
> Sure, create a UI message loop from the test before creating the window.
> 
> Ideally the test would actually use the message loop, too, so that it fully
> tests the IO handling. You used wl_display_roundtrip() instead but it is not
> completely realistic.
> 
> Anyway, simply creating the loop would avoid having special test-only handling
> here.

Cool. Looks like just creating a MessageLoopForUI and using
RunLoop().RunUntilIdle() does the job.

I've rebased this CL on a patch series that does this.

[spang, 2016-03-02 04:24:11]

lgtm

[Michael Forney, 2016-03-10 23:56:40]

The CQ bit was checked by forney@google.com

[Michael Forney, 2016-03-10 23:56:41]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1712103002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-10 23:57:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712103002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712103002/140001

[commit-bot: I haz the power, 2016-03-11 00:36:07]

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

[commit-bot: I haz the power, 2016-03-11 00:36:08]

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

[Michael Forney, 2016-03-11 02:43:34]

The CQ bit was checked by forney@google.com

[commit-bot: I haz the power, 2016-03-11 02:44:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712103002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712103002/140001

[commit-bot: I haz the power, 2016-03-11 05:16:18]

Description was changed from

==========
ozone/platform/wayland: Implement pointer handling

Only version 4 of wl_pointer is handled currently for simplicity. Version 5 was
released only recently in wayland-1.10, and compositors should support both
versions.

BUG=578890
==========

to

==========
ozone/platform/wayland: Implement pointer handling

Only version 4 of wl_pointer is handled currently for simplicity. Version 5 was
released only recently in wayland-1.10, and compositors should support both
versions.

BUG=578890
==========

[commit-bot: I haz the power, 2016-03-11 05:16:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-11 05:18:08]

Description was changed from

==========
ozone/platform/wayland: Implement pointer handling

Only version 4 of wl_pointer is handled currently for simplicity. Version 5 was
released only recently in wayland-1.10, and compositors should support both
versions.

BUG=578890
==========

to

==========
ozone/platform/wayland: Implement pointer handling

Only version 4 of wl_pointer is handled currently for simplicity. Version 5 was
released only recently in wayland-1.10, and compositors should support both
versions.

BUG=578890

Committed: https://crrev.com/1329450837ca9818a2da0210f681b4cb037f3e08
Cr-Commit-Position: refs/heads/master@{#380516}
==========

[commit-bot: I haz the power, 2016-03-11 05:18:10]

Patchset 8 (id:??) landed as
https://crrev.com/1329450837ca9818a2da0210f681b4cb037f3e08
Cr-Commit-Position: refs/heads/master@{#380516}