Use mojo for cursor control in ozone drm

Use mojo for cursor control in ozone drm

Use mojo to control the cursor in ozone drm in mus. First in a series of patches to
convert Ozone IPC to mojo.

BUG=620927
Committed: https://crrev.com/19f44a11dcc5866c52e86bee2eab07149d06ddd0
Cr-Commit-Position: refs/heads/master@{#408840}

[rjkroege, 2016-07-19 00:35:19]

rjkroege@chromium.org changed reviewers:
+ rockot@chromium.org, sievers@chromium.org, sky@chromium.org,
spang@chromium.org, tsepez@chromium.org

[rjkroege, 2016-07-19 00:35:20]

PTAL

rockot@ general mojom review and ui/ozone/platform/drm/DEPS
tsepez@ mojom security

spang@: ui/ozone/*

sky@: services/ui/*

sievers@: ui/gl/test

[sky, 2016-07-19 13:21:48]

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
File ui/ozone/public/interfaces/cursor.mojom (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/cursor.mojom:14: interface Cursor {
Can this be named CursorOzone? I realize that is implied by the namespace, but
it makes for less confusion when the same code ends up using two types of
Cursors.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/cursor.mojom:15: // Sets the cursor |bitmaps| on
|window| at |point| with |frame_delay_ms|.
nit: 2 spaces? I try to format .mojom files as I would C++ files.

[spang, 2016-07-19 14:59:37]

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/cursor_proxy_mojo.cc (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.cc:31:
evdev_cursor_ptr_->MoveCursor(widget, location);
Why is it necessary to have two connections (since we already synchronize access
to it? Is this a mojo restriction?)

It seems really subtle to decide which connection to use based on which function
gets called in this class.. And I suspect it may be broken, because explicit
warps and injected mouse events (remoting) start on the UI thread and may send a
move event from that thread, land here, and call through evdev_cursor_ptr_, not
main_cursor_ptr_.

[Tom Sepez, 2016-07-19 16:14:38]

lgtm

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/cursor_proxy_mojo.h (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.h:14: class SingleThreadTaskRunner;
nit: is this required?  I didn't see it used in this file.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.h:26: // it's the client) and sends
mount pointer control messages to a less
nit: mouse (sp)

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.h:30: CursorProxyMojo(shell::Connector*
connector);
nit: explicit

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mus_thread_proxy.h (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mus_thread_proxy.h:35: CursorProxyThread(MusThreadProxy*
mus_thread_proxy);
nit: explicit

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mus_thread_proxy.h:46: MusThreadProxy* mus_thread_proxy_; 
// Not owned.
nit: maybe blank line between methods and members. Also, can this be  
   MusThreadProxy* const mus_thread_proxy_;
?

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
File ui/ozone/public/interfaces/cursor.mojom (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/cursor.mojom:12: // functionality equivalent to the
DrmCursorProxy is exposed by a dedicated thread
nit: 80 cols, also line 15

[rjkroege, 2016-07-21 01:10:42]

ptal

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/cursor_proxy_mojo.cc (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.cc:31:
evdev_cursor_ptr_->MoveCursor(widget, location);
On 2016/07/19 14:59:37, spang wrote:
> Why is it necessary to have two connections (since we already synchronize
access
> to it? Is this a mojo restriction?)

Yes. Connections are thread-to-thread.

> 
> It seems really subtle to decide which connection to use based on which
function
> gets called in this class.. And I suspect it may be broken, because explicit
> warps and injected mouse events (remoting) start on the UI thread and may send
a
> move event from that thread, land here, and call through evdev_cursor_ptr_,
not
> main_cursor_ptr_.

Excellent point. I misread the code in drm_cursor.cc and have adjusted to use
thread-specific entry points in the DrmCursorProxy based on the the threading
conventions documented in CursorDelegateEvdev.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/cursor_proxy_mojo.h (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.h:14: class SingleThreadTaskRunner;
On 2016/07/19 16:14:38, Tom Sepez wrote:
> nit: is this required?  I didn't see it used in this file.

Done.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.h:26: // it's the client) and sends
mount pointer control messages to a less
On 2016/07/19 16:14:38, Tom Sepez wrote:
> nit: mouse (sp)

Done.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.h:30: CursorProxyMojo(shell::Connector*
connector);
On 2016/07/19 16:14:38, Tom Sepez wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/m...
File ui/ozone/platform/drm/mus_thread_proxy.h (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mus_thread_proxy.h:35: CursorProxyThread(MusThreadProxy*
mus_thread_proxy);
On 2016/07/19 16:14:38, Tom Sepez wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/m...
ui/ozone/platform/drm/mus_thread_proxy.h:46: MusThreadProxy* mus_thread_proxy_; 
// Not owned.
On 2016/07/19 16:14:38, Tom Sepez wrote:
> nit: maybe blank line between methods and members. Also, can this be  
>    MusThreadProxy* const mus_thread_proxy_;
> ?

Done.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
File ui/ozone/public/interfaces/cursor.mojom (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/cursor.mojom:12: // functionality equivalent to the
DrmCursorProxy is exposed by a dedicated thread
On 2016/07/19 16:14:38, Tom Sepez wrote:
> nit: 80 cols, also line 15

Done. I naïvely assumed presubmit would check .mojom. Filed a bug.

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/cursor.mojom:14: interface Cursor {
On 2016/07/19 13:21:48, sky wrote:
> Can this be named CursorOzone? I realize that is implied by the namespace, but
> it makes for less confusion when the same code ends up using two types of
> Cursors.

Having wished for the same thing for bunch of other classes (e.g. Surface,
Service, Display, DisplayMode), I'm sympathetic. But, it will also be ugly for
an interface that is intended to only be used within ozone code to have the
extra Ozone in the name. I renamed so at least it's unique. WDYT?

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/cursor.mojom:15: // Sets the cursor |bitmaps| on
|window| at |point| with |frame_delay_ms|.
On 2016/07/19 13:21:48, sky wrote:
> nit: 2 spaces? I try to format .mojom files as I would C++ files.

Done.

[sky, 2016-07-21 15:27:26]

I would have preferred DeviceCursorOzone, but DeviceCursor is better than
Cursor, so  LGTM

[Ken Rockot(use gerrit already), 2016-07-21 19:12:30]

lgtm

https://codereview.chromium.org/2156093004/diff/40001/ui/ozone/public/interfa...
File ui/ozone/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2156093004/diff/40001/ui/ozone/public/interfa...
ui/ozone/public/interfaces/BUILD.gn:13: get_path_info("../../../..", "abspath"),
... Is this really necessary?

[spang, 2016-07-21 19:21:58]

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
File ui/ozone/platform/drm/cursor_proxy_mojo.cc (right):

https://codereview.chromium.org/2156093004/diff/20001/ui/ozone/platform/drm/c...
ui/ozone/platform/drm/cursor_proxy_mojo.cc:31:
evdev_cursor_ptr_->MoveCursor(widget, location);
On 2016/07/21 01:10:41, rjkroege wrote:
> On 2016/07/19 14:59:37, spang wrote:
> > Why is it necessary to have two connections (since we already synchronize
> access
> > to it? Is this a mojo restriction?)
> 
> Yes. Connections are thread-to-thread.
> 
> > 
> > It seems really subtle to decide which connection to use based on which
> function
> > gets called in this class.. And I suspect it may be broken, because explicit
> > warps and injected mouse events (remoting) start on the UI thread and may
send
> a
> > move event from that thread, land here, and call through evdev_cursor_ptr_,
> not
> > main_cursor_ptr_.
> 
> Excellent point. I misread the code in drm_cursor.cc and have adjusted to use
> thread-specific entry points in the DrmCursorProxy based on the the threading
> conventions documented in CursorDelegateEvdev.

Those docs aren't quite right :( For example tablets don't use dx/dy

[spang, 2016-07-22 19:52:54]

https://codereview.chromium.org/2156093004/diff/40001/ui/ozone/platform/drm/h...
File ui/ozone/platform/drm/host/drm_cursor.cc (right):

https://codereview.chromium.org/2156093004/diff/40001/ui/ozone/platform/drm/h...
ui/ozone/platform/drm/host/drm_cursor.cc:181: SendCursorMoveLocked(UI);
Assuming MoveCursorTo() is called from UI is broken, the exception is for tablet
devices.

[rjkroege, 2016-07-27 01:39:17]

On 2016/07/22 19:52:54, spang wrote:
>
https://codereview.chromium.org/2156093004/diff/40001/ui/ozone/platform/drm/h...
> File ui/ozone/platform/drm/host/drm_cursor.cc (right):
> 
>
https://codereview.chromium.org/2156093004/diff/40001/ui/ozone/platform/drm/h...
> ui/ozone/platform/drm/host/drm_cursor.cc:181: SendCursorMoveLocked(UI);
> Assuming MoveCursorTo() is called from UI is broken, the exception is for
tablet
> devices.

I addressed this. And rebased. PTAL.

[spang, 2016-07-27 17:00:59]

lgtm

https://codereview.chromium.org/2156093004/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2156093004/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:93: uint32_t frame_delay_ms);
Should it really be unsigned? I'd say int is the correct thing to use - per the
C++ style guide, unsigned integers should be avoided (with the exception of
bitmasks) because the language's type conversion rules are poisonous.

[rjkroege, 2016-07-27 20:55:14]

https://codereview.chromium.org/2156093004/diff/60001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2156093004/diff/60001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:93: uint32_t frame_delay_ms);
On 2016/07/27 17:00:59, spang wrote:
> Should it really be unsigned? I'd say int is the correct thing to use - per
the
> C++ style guide, unsigned integers should be avoided (with the exception of
> bitmasks) because the language's type conversion rules are poisonous.

But negative time into the future? That makes me sad. It happens to be
completely safe in this case but prudently done anyway.

[rjkroege, 2016-07-27 21:17:25]

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

[commit-bot: I haz the power, 2016-07-27 21:17:42]

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

[commit-bot: I haz the power, 2016-07-27 21:19:32]

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

[commit-bot: I haz the power, 2016-07-27 21:19:33]

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

[no sievers, 2016-07-27 21:32:44]

ui/gl lgtm

[rjkroege, 2016-07-27 21:41:07]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2016-07-27 21:41:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org, sky@chromium.org, spang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2156093004/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2016-07-27 21:41:33]

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-07-27 22:00:45]

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

[commit-bot: I haz the power, 2016-07-27 22:00:47]

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

[rjkroege, 2016-07-30 00:49:07]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2016-07-30 00:49:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org, sky@chromium.org, sievers@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/2156093004/#ps140001
(title: "corrected small code oversight")

[commit-bot: I haz the power, 2016-07-30 00:49:27]

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-07-30 01:59:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-30 02:01:19]

Description was changed from

==========
Use mojo for cursor control in ozone drm

Use mojo to control the cursor in ozone drm in mus. First in a series of patches
to
convert Ozone IPC to mojo.

BUG=620927
==========

to

==========
Use mojo for cursor control in ozone drm

Use mojo to control the cursor in ozone drm in mus. First in a series of patches
to
convert Ozone IPC to mojo.

BUG=620927

Committed: https://crrev.com/19f44a11dcc5866c52e86bee2eab07149d06ddd0
Cr-Commit-Position: refs/heads/master@{#408840}
==========

[commit-bot: I haz the power, 2016-07-30 02:01:20]

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