[aura-mus] Add ui::CursorData, with mojo serialization.

[aura-mus] Add ui::CursorData, with mojo serialization.

This creates a new ui::CursorData class which is a cross-platform way of
moving cursor descriptions over a mojo pipe. This maps to the new
ui.mojom.CursorData struct via added type traits. Finally, adds unit
tests for serialization.

BUG=705037
Review-Url: https://codereview.chromium.org/2786983003
Cr-Commit-Position: refs/heads/master@{#461801}
Committed: https://chromium.googlesource.com/chromium/src/+/0c80ac5421d4b7f81595b525163d54b686ca8785

[Elliot Glaysher, 2017-03-30 20:59:09]

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

[commit-bot: I haz the power, 2017-03-30 21:00:27]

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

[Elliot Glaysher, 2017-03-30 22:25:25]

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

[commit-bot: I haz the power, 2017-03-30 22:26:02]

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

[Elliot Glaysher, 2017-03-30 22:53:32]

erg@chromium.org changed reviewers:
+ bungeman@google.com, sky@chromium.org, tsepez@chromium.org

[Elliot Glaysher, 2017-03-30 22:53:33]

tsepez: new struct CursorData in the moved cursor.mojom, otherwise just moves
and testing.

sky: main review

bungeman: DEPS for //services/ui/public/interfaces/cursor/ dependency on
//skia/public/interfaces. (The client library, //services/ui/cpp/, already uses
this.)

https://codereview.chromium.org/2786983003/diff/1/services/ui/public/interfac...
File services/ui/public/interfaces/cursor/cursor_struct_traits.h (right):

https://codereview.chromium.org/2786983003/diff/1/services/ui/public/interfac...
services/ui/public/interfaces/cursor/cursor_struct_traits.h:22: static const
std::vector<SkBitmap>& cursor_frames(const ui::CursorData& c);
A fun learning experience: this compiles if I return cursor_frames by value
instead of const ref, but crashes at runtime.

[commit-bot: I haz the power, 2017-03-31 00:38:28]

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

[commit-bot: I haz the power, 2017-03-31 00:38:29]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2017-03-31 03:10:59]

bungeman@google.com changed reviewers:
+ bsalomon@google.com, reed@google.com

[bungeman-skia, 2017-03-31 03:10:59]

reed and bsalomon are probably better api reviewers for this, though I'm sure
they won't be overly excited about more use of SkBitmap.

[sky, 2017-03-31 04:57:58]

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor.mojom (right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:65: CursorType native_type;
cursor_type?

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:71: gfx.mojom.Point hotspot;
Is this pixels or dips? Generally if we don't say it's assumed to be dips, but I
suspect this is pixels. I could certainly be wrong.

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:74: skia.mojom.BitmapArray
cursor_frames;
Do cursors vary depending upon dpi? Is it expected that cursor_frames is
specific to the dpi of the display?

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:74: skia.mojom.BitmapArray
cursor_frames;
Would it be better to use shared memory for the bitmaps? How often do we send
cursors around?

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor_struct_traits.cc (right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor_struct_traits.cc:36: if
(!data.ReadHotspot(&hotspot) || !data.ReadCursorFrames(&cursor_frames)) {
no {}

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor_struct_traits_test.mojom
(right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor_struct_traits_test.mojom:11:
EchoCursorData(CursorData in) => (CursorData out);
You shouldn't need to write a mojom to exercise type converters. See recent
threads on this.
https://chromium.googlesource.com/chromium/src/+/master/ui/base/accelerators/...
is an example.

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
File ui/base/cursor/cursor_data.h (right):

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
ui/base/cursor/cursor_data.h:40: CursorData(int type);
explicit

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
ui/base/cursor/cursor_data.h:50: uint32_t frame_delay_ms() const { return
frame_delay_ms_; }
TimeDelta?

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
ui/base/cursor/cursor_data.h:51: gfx::Point hotspot() const { return hotspot_; }
const gfx::Point&

[Tom Sepez, 2017-03-31 16:07:07]

LGTM with question

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor.mojom (right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:74: skia.mojom.BitmapArray
cursor_frames;
Arbitrarily deep?  Not sure it matters ...

[bsalomon, 2017-03-31 16:25:10]

On 2017/03/31 03:10:59, bungeman-skia wrote:
> reed and bsalomon are probably better api reviewers for this, though I'm sure
> they won't be overly excited about more use of SkBitmap.

Is it possible to use SkImage instead of SkBitmap for this use case? SkBitmap is
somewhat deprecated and we'd prefer Chrome uses SkImage whenever possible.

[Elliot Glaysher, 2017-03-31 20:35:32]

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

[commit-bot: I haz the power, 2017-03-31 20:35:57]

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

[Elliot Glaysher, 2017-03-31 21:18:16]

ptal 

On 2017/03/31 16:25:10, bsalomon wrote:
> Is it possible to use SkImage instead of SkBitmap for this use case? SkBitmap
is
> somewhat deprecated and we'd prefer Chrome uses SkImage whenever possible.

The source side of where I want to use this already creates SkBitmaps and
ui::CursorData will be consuming those. There isn't currently a mojo transport
mechanism for SkImages (while there is for SkBitmap), so that would have to be
built-out, too. Finally, the cursor code in the window server and ozone layers
which will be consuming ui::CursorData already consumes SkBitmaps.

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor.mojom (right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:65: CursorType native_type;
On 2017/03/31 04:57:57, sky wrote:
> cursor_type?

Done.

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:71: gfx.mojom.Point hotspot;
On 2017/03/31 04:57:57, sky wrote:
> Is this pixels or dips? Generally if we don't say it's assumed to be dips, but
I
> suspect this is pixels. I could certainly be wrong.

added "_in_pixels" suffix.

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:74: skia.mojom.BitmapArray
cursor_frames;
On 2017/03/31 16:07:07, Tom Sepez wrote:
> Arbitrarily deep?  Not sure it matters ...

I'm not sure what you're asking here.

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:74: skia.mojom.BitmapArray
cursor_frames;
On 2017/03/31 04:57:57, sky wrote:
> Do cursors vary depending upon dpi? Is it expected that cursor_frames is
> specific to the dpi of the display?

You're right and I misread this. Added an |image_scale_factor| float to
represent what the scale factor of |cursor_frames| is.

(I had confused this as part of the rotation and display dpi transformation. My
bad.)

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor.mojom:74: skia.mojom.BitmapArray
cursor_frames;
On 2017/03/31 04:57:57, sky wrote:
> Would it be better to use shared memory for the bitmaps? How often do we send
> cursors around?

Assuming the user is actually checking IsSameAs() before sending a cursor, only
on cursor changes.

As far as I can tell though, we don't have a way to send bitmaps across mojo
pipes using shared memory. This would require modification of the skia bindings.

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor_struct_traits.cc (right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor_struct_traits.cc:36: if
(!data.ReadHotspot(&hotspot) || !data.ReadCursorFrames(&cursor_frames)) {
On 2017/03/31 04:57:57, sky wrote:
> no {}

(now legitimately multiline.)

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/cursor/cursor_struct_traits_test.mojom
(right):

https://codereview.chromium.org/2786983003/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/cursor/cursor_struct_traits_test.mojom:11:
EchoCursorData(CursorData in) => (CursorData out);
On 2017/03/31 04:57:57, sky wrote:
> You shouldn't need to write a mojom to exercise type converters. See recent
> threads on this.
>
https://chromium.googlesource.com/chromium/src/+/master/ui/base/accelerators/...
> is an example.

Deleted this file and used the Serialize/Deserialize manually.

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
File ui/base/cursor/cursor_data.h (right):

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
ui/base/cursor/cursor_data.h:40: CursorData(int type);
On 2017/03/31 04:57:58, sky wrote:
> explicit

Done.

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
ui/base/cursor/cursor_data.h:50: uint32_t frame_delay_ms() const { return
frame_delay_ms_; }
On 2017/03/31 04:57:58, sky wrote:
> TimeDelta?

Done. (Here and throughout the patch.)

https://codereview.chromium.org/2786983003/diff/20001/ui/base/cursor/cursor_d...
ui/base/cursor/cursor_data.h:51: gfx::Point hotspot() const { return hotspot_; }
On 2017/03/31 04:57:58, sky wrote:
> const gfx::Point&

Done.

[commit-bot: I haz the power, 2017-03-31 21:51:06]

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

[commit-bot: I haz the power, 2017-03-31 21:51:07]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-31 23:40:53]

LGTM

[Elliot Glaysher, 2017-04-04 17:09:10]

ping bsalomon

[bsalomon, 2017-04-04 17:21:59]

lgtm

[Elliot Glaysher, 2017-04-04 17:27:53]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2017-04-04 17:27:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2786983003/#ps40001
(title: "everybody comments")

[commit-bot: I haz the power, 2017-04-04 17:28:26]

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-04-04 17:43:06]

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

[commit-bot: I haz the power, 2017-04-04 17:43:08]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Elliot Glaysher, 2017-04-04 17:45:48]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-04-04 17:46:13]

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-04-04 19:25:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491327948734530,
"parent_rev": "cd49590a296f0f2a28325672a03a9d512b03587b", "commit_rev":
"0c80ac5421d4b7f81595b525163d54b686ca8785"}

[commit-bot: I haz the power, 2017-04-04 19:26:14]

Description was changed from

==========
[aura-mus] Add ui::CursorData, with mojo serialization.

This creates a new ui::CursorData class which is a cross-platform way of
moving cursor descriptions over a mojo pipe. This maps to the new
ui.mojom.CursorData struct via added type traits. Finally, adds unit
tests for serialization.

BUG=705037
==========

to

==========
[aura-mus] Add ui::CursorData, with mojo serialization.

This creates a new ui::CursorData class which is a cross-platform way of
moving cursor descriptions over a mojo pipe. This maps to the new
ui.mojom.CursorData struct via added type traits. Finally, adds unit
tests for serialization.

BUG=705037

Review-Url: https://codereview.chromium.org/2786983003
Cr-Commit-Position: refs/heads/master@{#461801}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c80ac5421d4b7f81595b525163d...
==========

[commit-bot: I haz the power, 2017-04-04 19:26:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0c80ac5421d4b7f81595b525163d...