Refactor ozone drm cursor code for mojo

Refactor ozone drm cursor code for mojo

Adjust code in ozone drm platform to simplify the introduction of mojo IPC for
cursor control.

BUG=620927
Committed: https://crrev.com/6b5aaf6974e27f524ed8e6f87cfe511ff32c5c4e
Cr-Commit-Position: refs/heads/master@{#401265}

[rjkroege, 2016-06-20 21:57:40]

rjkroege@chromium.org changed reviewers:
+ dnicoara@chromium.org

[rjkroege, 2016-06-20 21:57:41]

ptal

[dnicoara, 2016-06-21 14:14:16]

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_thread.cc (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_thread.cc:178: void DrmThread::MoveCursor(const
gfx::AcceleratedWidget& widget,
Same here.

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_thread.h:85: void MoveCursor(const
gfx::AcceleratedWidget& widget,
nit: Why const& for AcceleratedWidget? It is supposed to be lightweight.

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
File ui/ozone/platform/drm/host/drm_cursor.cc (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
ui/ozone/platform/drm/host/drm_cursor.cc:216:
evdev_thread_checker_.DetachFromThread();
Unless this is not called on the evdev thread you shouldn't detach. (In fact you
should DCHECK it since that makes sure |evdev_tread_checker_| is attached to the
correct thread)

[rjkroege, 2016-06-21 17:20:52]

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_thread.cc (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_thread.cc:178: void DrmThread::MoveCursor(const
gfx::AcceleratedWidget& widget,
On 2016/06/21 14:14:16, dnicoara wrote:
> Same here.

same comment.

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_thread.h:85: void MoveCursor(const
gfx::AcceleratedWidget& widget,
On 2016/06/21 14:14:16, dnicoara wrote:
> nit: Why const& for AcceleratedWidget? It is supposed to be lightweight.

To permit the mojo struct traits implementation to be zero-copy, it has to be
const&. You will be seeing more of this for the same reason in subsequent CLs.

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
File ui/ozone/platform/drm/host/drm_cursor.cc (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
ui/ozone/platform/drm/host/drm_cursor.cc:216:
evdev_thread_checker_.DetachFromThread();
On 2016/06/21 14:14:16, dnicoara wrote:
> Unless this is not called on the evdev thread you shouldn't detach. (In fact
you
> should DCHECK it since that makes sure |evdev_tread_checker_| is attached to
the
> correct thread)

The evdev_thread_checker_ is initialized on the main thread in the constructor.
So, I need to call DetachFromThread here to force the thread checker to be
attached to the evdev thread instead yes? This is true empirically: the thread
where this class is constructed is most definitely not the same thread as where
InitializeOnEvdev is called.

[dnicoara, 2016-06-21 17:38:18]

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_thread.h:85: void MoveCursor(const
gfx::AcceleratedWidget& widget,
On 2016/06/21 17:20:52, rjkroege wrote:
> On 2016/06/21 14:14:16, dnicoara wrote:
> > nit: Why const& for AcceleratedWidget? It is supposed to be lightweight.
> 
> To permit the mojo struct traits implementation to be zero-copy, it has to be
> const&. You will be seeing more of this for the same reason in subsequent CLs.

Ah, good to know, thanks for the explanation. Out of curiosity is there a mojo
traits dos/don'ts guide?

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
File ui/ozone/platform/drm/host/drm_cursor.cc (right):

https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
ui/ozone/platform/drm/host/drm_cursor.cc:216:
evdev_thread_checker_.DetachFromThread();
On 2016/06/21 17:20:52, rjkroege wrote:
> On 2016/06/21 14:14:16, dnicoara wrote:
> > Unless this is not called on the evdev thread you shouldn't detach. (In fact
> you
> > should DCHECK it since that makes sure |evdev_tread_checker_| is attached to
> the
> > correct thread)
> 
> The evdev_thread_checker_ is initialized on the main thread in the
constructor.
> So, I need to call DetachFromThread here to force the thread checker to be
> attached to the evdev thread instead yes? This is true empirically: the thread
> where this class is constructed is most definitely not the same thread as
where
> InitializeOnEvdev is called.

Ah, I see your reasoning. My understanding is that you should then call
DetachFromThread() when |evdev_thread_checker_| is created (in DrmCursor
constructor) and call CalledOnValidThread() in here. That way all call sites
that need to be done on the Evdev thread are checked.

[rjkroege, 2016-06-21 19:09:01]

On 2016/06/21 17:38:18, dnicoara wrote:
>
https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
> File ui/ozone/platform/drm/gpu/drm_thread.h (right):
> 
>
https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/gpu/d...
> ui/ozone/platform/drm/gpu/drm_thread.h:85: void MoveCursor(const
> gfx::AcceleratedWidget& widget,
> On 2016/06/21 17:20:52, rjkroege wrote:
> > On 2016/06/21 14:14:16, dnicoara wrote:
> > > nit: Why const& for AcceleratedWidget? It is supposed to be lightweight.
> > 
> > To permit the mojo struct traits implementation to be zero-copy, it has to
be
> > const&. You will be seeing more of this for the same reason in subsequent
CLs.
> 
> Ah, good to know, thanks for the explanation. Out of curiosity is there a mojo
> traits dos/don'ts guide?

https://www.chromium.org/developers/design-documents/mojo/type-mapping is the
best guide so far that I've seen.

> 
>
https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
> File ui/ozone/platform/drm/host/drm_cursor.cc (right):
> 
>
https://codereview.chromium.org/2088533002/diff/1/ui/ozone/platform/drm/host/...
> ui/ozone/platform/drm/host/drm_cursor.cc:216:
> evdev_thread_checker_.DetachFromThread();
> On 2016/06/21 17:20:52, rjkroege wrote:
> > On 2016/06/21 14:14:16, dnicoara wrote:
> > > Unless this is not called on the evdev thread you shouldn't detach. (In
fact
> > you
> > > should DCHECK it since that makes sure |evdev_tread_checker_| is attached
to
> > the
> > > correct thread)
> > 
> > The evdev_thread_checker_ is initialized on the main thread in the
> constructor.
> > So, I need to call DetachFromThread here to force the thread checker to be
> > attached to the evdev thread instead yes? This is true empirically: the
thread
> > where this class is constructed is most definitely not the same thread as
> where
> > InitializeOnEvdev is called.
> 
> Ah, I see your reasoning. My understanding is that you should then call
> DetachFromThread() when |evdev_thread_checker_| is created (in DrmCursor
> constructor) and call CalledOnValidThread() in here. That way all call sites
> that need to be done on the Evdev thread are checked.

Ah. Nice. Done that way.

[dnicoara, 2016-06-21 19:13:48]

lgtm

[rjkroege, 2016-06-21 19:16:04]

The CQ bit was checked by rjkroege@chromium.org

[commit-bot: I haz the power, 2016-06-21 19:16:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088533002/20001

[commit-bot: I haz the power, 2016-06-21 19:55:44]

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

[commit-bot: I haz the power, 2016-06-21 19:55:46]

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

[rjkroege, 2016-06-21 21:23:34]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2016-06-21 21:23:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/2088533002/#ps40001
(title: "build fix")

[commit-bot: I haz the power, 2016-06-21 21:23:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088533002/40001

[commit-bot: I haz the power, 2016-06-21 22:22:37]

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

[commit-bot: I haz the power, 2016-06-21 22:22:39]

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

[rjkroege, 2016-06-22 12:32:03]

The CQ bit was checked by rjkroege@chromium.org

[rjkroege, 2016-06-22 12:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/2088533002/#ps60001
(title: "fix build")

[commit-bot: I haz the power, 2016-06-22 12:32:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088533002/60001

[commit-bot: I haz the power, 2016-06-22 13:04:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-22 13:06:12]

Description was changed from

==========
Refactor ozone drm cursor code for mojo

Adjust code in ozone drm platform to simplify the introduction of mojo IPC for
cursor control.

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

to

==========
Refactor ozone drm cursor code for mojo

Adjust code in ozone drm platform to simplify the introduction of mojo IPC for
cursor control.

BUG=620927

Committed: https://crrev.com/6b5aaf6974e27f524ed8e6f87cfe511ff32c5c4e
Cr-Commit-Position: refs/heads/master@{#401265}
==========

[commit-bot: I haz the power, 2016-06-22 13:06:15]

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