StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>

StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>.

This CL defines StructTraits<mus::mojom::Event, unique_ptr<ui::Event>>. With
this mojo automatically uses unique_ptr<ui::Event> instead of mus::mojom::Event
when generating C++ header files for mojom interfaces, and events are
automatically serializied/deserialized and validated when sending/receiving over
mojo IPC.

BUG=578206
Committed: https://crrev.com/73be633b0f9b71668ab204c22bd14742bd6c7ead
Cr-Commit-Position: refs/heads/master@{#398107}

[Hadi, 2016-06-03 18:54:50]

Description was changed from

==========
WIP: StructTraits for mus::mojom::Event.

BUG=578206
==========

to

==========
StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>.

This CL defines StructTraits<mus::mojom::Event, unique_ptr<ui::Event>>. With
this mojo automatically uses unique_ptr<ui::Event> instead of mus::mojom::Event
when generating C++ header files for mojom interfaces, and events are
automatically serializied/deserialized and validated when sending/receiving over
mojo IPC.

BUG=578206
==========

[Hadi, 2016-06-03 19:02:00]

moshayedi@chromium.org changed reviewers:
+ rockot@chromium.org, sadrul@chromium.org, sky@chromium.org

[Hadi, 2016-06-03 19:02:03]

PTAL.

1. I kept the same structure of input_event_type_converters.cc for
event_struct_traits.cc, but will clean it up in a subsequent CL to make it
simpler. For example, if we add proper constructors to ui::PointerEvent, we can
construct touch pointer events without first constructing a ui::TouchEvent.

2. Now the type converters and param traits for events can be removed from code.
I thought I can do that in separate CLs to keep this CL a bit simpler.

[sky, 2016-06-03 20:06:51]

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
File components/mus/public/cpp/tests/window_tree_client_private.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
components/mus/public/cpp/tests/window_tree_client_private.cc:44:
std::move(event), observer_id);
Does this really work? If it does, doesn't that mean you're effectively taking
the pointer from a const object?

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
components/mus/public/cpp/tests/window_tree_client_unittest.cc:455: 1,
server_id(root), ui::Event::Clone(*ui_event.get()), 0);
Why do you need to clone anywhere in this file? ui_event is already a
unique_ptr.

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
File components/mus/ws/window_manager_state_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
components/mus/ws/window_manager_state_unittest.cc:142: EXPECT_EQ("InputEvent
window=1,1 event_action=7",
Why is this changing?

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
File components/mus/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
components/mus/ws/window_tree_unittest.cc:263: EXPECT_EQ("InputEvent window=0,3
event_action=16",
Why is the action changing?

https://codereview.chromium.org/1939133002/diff/320001/ui/events/mojo/event.t...
File ui/events/mojo/event.typemap (right):

https://codereview.chromium.org/1939133002/diff/320001/ui/events/mojo/event.t...
ui/events/mojo/event.typemap:13: "//ui/events/mojo/event_struct_traits.h",
You have 2 spaces in deps, so I would assume this should be 2 as well. Or should
the above be 4? Either way, make them consistent.

[James Cook, 2016-06-03 20:25:39]

On 2016/06/03 20:06:51, sky wrote:
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
> File components/mus/public/cpp/tests/window_tree_client_private.cc (right):
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
> components/mus/public/cpp/tests/window_tree_client_private.cc:44:
> std::move(event), observer_id);
> Does this really work? If it does, doesn't that mean you're effectively taking
> the pointer from a const object?
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
> File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
> components/mus/public/cpp/tests/window_tree_client_unittest.cc:455: 1,
> server_id(root), ui::Event::Clone(*ui_event.get()), 0);
> Why do you need to clone anywhere in this file? ui_event is already a
> unique_ptr.
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
> File components/mus/ws/window_manager_state_unittest.cc (right):
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
> components/mus/ws/window_manager_state_unittest.cc:142: EXPECT_EQ("InputEvent
> window=1,1 event_action=7",
> Why is this changing?
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
> File components/mus/ws/window_tree_unittest.cc (right):
> 
>
https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
> components/mus/ws/window_tree_unittest.cc:263: EXPECT_EQ("InputEvent
window=0,3
> event_action=16",
> Why is the action changing?
> 
>
https://codereview.chromium.org/1939133002/diff/320001/ui/events/mojo/event.t...
> File ui/events/mojo/event.typemap (right):
> 
>
https://codereview.chromium.org/1939133002/diff/320001/ui/events/mojo/event.t...
> ui/events/mojo/event.typemap:13: "//ui/events/mojo/event_struct_traits.h",
> You have 2 spaces in deps, so I would assume this should be 2 as well. Or
should
> the above be 4? Either way, make them consistent.

Drive-by question (not a suggestion):  It looks like it's still that case that
when you serialize a pointer-event in the window server you deserialize to a
mouse-event (and not a pointer-event) in the client. Do you think that will ever
change?

[Hadi, 2016-06-06 13:48:33]

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
File components/mus/public/cpp/tests/window_tree_client_private.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
components/mus/public/cpp/tests/window_tree_client_private.cc:44:
std::move(event), observer_id);
On 2016/06/03 20:06:50, sky wrote:
> Does this really work? If it does, doesn't that mean you're effectively taking
> the pointer from a const object?

My mistake. Fixed. (It worked, but I'm not sure why)

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
components/mus/public/cpp/tests/window_tree_client_unittest.cc:455: 1,
server_id(root), ui::Event::Clone(*ui_event.get()), 0);
On 2016/06/03 20:06:50, sky wrote:
> Why do you need to clone anywhere in this file? ui_event is already a
> unique_ptr.

Done.

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
File components/mus/ws/window_manager_state_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
components/mus/ws/window_manager_state_unittest.cc:142: EXPECT_EQ("InputEvent
window=1,1 event_action=7",
On 2016/06/03 20:06:50, sky wrote:
> Why is this changing?

Previously these numbers corresponded to values in mus::mojom::EventType, but
now they correspond to values in ui::EventType. mojom::EventType::KEY_PRESSED is
1 and ET_KEY_PRESSED is 7.

This is because we changed TestChangeTracker to work with ui::Event instead of
mojom::Event.

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
File components/mus/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/ws/wind...
components/mus/ws/window_tree_unittest.cc:263: EXPECT_EQ("InputEvent window=0,3
event_action=16",
On 2016/06/03 20:06:51, sky wrote:
> Why is the action changing?

Previously these numbers corresponded to values in mus::mojom::EventType, but
now they correspond to values in ui::EventType. mojom::EventType::POINTER_DOWN
is 4 and ET_POINTER_DOWN is 16.

https://codereview.chromium.org/1939133002/diff/320001/ui/events/mojo/event.t...
File ui/events/mojo/event.typemap (right):

https://codereview.chromium.org/1939133002/diff/320001/ui/events/mojo/event.t...
ui/events/mojo/event.typemap:13: "//ui/events/mojo/event_struct_traits.h",
On 2016/06/03 20:06:51, sky wrote:
> You have 2 spaces in deps, so I would assume this should be 2 as well. Or
should
> the above be 4? Either way, make them consistent.

Done.

[Hadi, 2016-06-06 13:54:59]

On 2016/06/03 20:25:39, James Cook wrote:
> Drive-by question (not a suggestion):  It looks like it's still that case that
> when you serialize a pointer-event in the window server you deserialize to a
> mouse-event (and not a pointer-event) in the client. Do you think that will
ever
> change?

I think the long term plan is to remove separate mouse/touch events and use
pointer events instead. See crbug.com/555750.

[Hadi, 2016-06-06 13:56:53]

On 2016/06/06 13:54:59, Hadi wrote:
> On 2016/06/03 20:25:39, James Cook wrote:
> > Drive-by question (not a suggestion):  It looks like it's still that case
that
> > when you serialize a pointer-event in the window server you deserialize to a
> > mouse-event (and not a pointer-event) in the client. Do you think that will
> ever
> > change?
> 
> I think the long term plan is to remove separate mouse/touch events and use
> pointer events instead. See crbug.com/555750.

crbug.com/615102 may also be a bit related.

[Ken Rockot(use gerrit already), 2016-06-06 14:03:34]

Sorry to catch this so late but I think this typemap should really use the new
pass_by_value option. See
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/tests/struct_wi...
for syntax.

This will make the bindings use a direct unique _ptr rather than a const ref to
one, which is a much more natural interface for a unique_ptr. Probably better to
do this ASAP since it will be more painful to change later.

[Hadi, 2016-06-06 14:35:27]

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
File components/mus/public/cpp/tests/window_tree_client_private.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
components/mus/public/cpp/tests/window_tree_client_private.cc:44:
std::move(event), observer_id);
On 2016/06/06 13:48:32, Hadi wrote:
> On 2016/06/03 20:06:50, sky wrote:
> > Does this really work? If it does, doesn't that mean you're effectively
taking
> > the pointer from a const object?
> 
> My mistake. Fixed. (It worked, but I'm not sure why)

Went back to using std::move() after using pass_by_value() as rockot@ suggested.

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/1939133002/diff/320001/components/mus/public/...
components/mus/public/cpp/tests/window_tree_client_unittest.cc:455: 1,
server_id(root), ui::Event::Clone(*ui_event.get()), 0);
On 2016/06/06 13:48:32, Hadi wrote:
> On 2016/06/03 20:06:50, sky wrote:
> > Why do you need to clone anywhere in this file? ui_event is already a
> > unique_ptr.
> 
> Done.

Went back to using clone after using pass_by_value(). Since we will use ui_event
later in this function we don't want to give up ownership of it.

[Hadi, 2016-06-06 14:36:13]

On 2016/06/06 14:03:34, Ken Rockot wrote:
> Sorry to catch this so late but I think this typemap should really use the new
> pass_by_value option. See
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/tests/struct_wi...
> for syntax.
> 
> This will make the bindings use a direct unique _ptr rather than a const ref
to
> one, which is a much more natural interface for a unique_ptr. Probably better
to
> do this ASAP since it will be more painful to change later.

Done.

[sky, 2016-06-06 17:09:56]

LGTM

[sadrul, 2016-06-06 17:10:59]

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:204: if (!event->IsPointerEvent() &&
!event->IsMouseWheelEvent())
Use has_pointer_data()?

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:295: } else {
Don't need the else.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:323: std::unique_ptr<ui::MouseEvent>
pre_wheel_event(new ui::MouseEvent(
Why do you need to create two events?

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:334: } else {
don't need the else.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:338: gfx::Point(), gfx::Point(),
ui::EventTimeForNow(),
send in |location| and |screen_location| here, instead of explicitly setting
them afterwards?

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:348: ui::PointerEvent* touch_event = new
ui::PointerEvent(ui::TouchEvent(
s/touch_event/pointer_event/g

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:364: default:
Do not have the default

[Hadi, 2016-06-06 17:45:48]

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:204: if (!event->IsPointerEvent() &&
!event->IsMouseWheelEvent())
On 2016/06/06 17:10:59, sadrul wrote:
> Use has_pointer_data()?

Done.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:295: } else {
On 2016/06/06 17:10:59, sadrul wrote:
> Don't need the else.

Done.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:323: std::unique_ptr<ui::MouseEvent>
pre_wheel_event(new ui::MouseEvent(
On 2016/06/06 17:10:59, sadrul wrote:
> Why do you need to create two events?

Done.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:334: } else {
On 2016/06/06 17:10:59, sadrul wrote:
> don't need the else.

Done.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:338: gfx::Point(), gfx::Point(),
ui::EventTimeForNow(),
On 2016/06/06 17:10:59, sadrul wrote:
> send in |location| and |screen_location| here, instead of explicitly setting
> them afterwards?

Done.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:348: ui::PointerEvent* touch_event = new
ui::PointerEvent(ui::TouchEvent(
On 2016/06/06 17:10:59, sadrul wrote:
> s/touch_event/pointer_event/g

Done.

https://codereview.chromium.org/1939133002/diff/380001/ui/events/mojo/event_s...
ui/events/mojo/event_struct_traits.cc:364: default:
On 2016/06/06 17:10:59, sadrul wrote:
> Do not have the default

Done. Replaced it with "case UNKNOWN:" (the only remaining case) to make it more
explicit and less error-prone in future if we decide to add new event types.

[Hadi, 2016-06-06 18:05:20]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2016-06-06 18:05:20]

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/1939133002/#ps400001
(title: "Addressed feedback.")

[commit-bot: I haz the power, 2016-06-06 18:05:50]

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

[sadrul, 2016-06-06 18:57:51]

lgtm In a follow-up CL, please add some tests like the latency-info tests Fady
is adding in
https://codereview.chromium.org/2040733002/diff/160001/ui/events/mojo/struct_...

[commit-bot: I haz the power, 2016-06-06 20:12:29]

Description was changed from

==========
StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>.

This CL defines StructTraits<mus::mojom::Event, unique_ptr<ui::Event>>. With
this mojo automatically uses unique_ptr<ui::Event> instead of mus::mojom::Event
when generating C++ header files for mojom interfaces, and events are
automatically serializied/deserialized and validated when sending/receiving over
mojo IPC.

BUG=578206
==========

to

==========
StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>.

This CL defines StructTraits<mus::mojom::Event, unique_ptr<ui::Event>>. With
this mojo automatically uses unique_ptr<ui::Event> instead of mus::mojom::Event
when generating C++ header files for mojom interfaces, and events are
automatically serializied/deserialized and validated when sending/receiving over
mojo IPC.

BUG=578206
==========

[commit-bot: I haz the power, 2016-06-06 20:12:30]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-06-06 20:14:59]

Description was changed from

==========
StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>.

This CL defines StructTraits<mus::mojom::Event, unique_ptr<ui::Event>>. With
this mojo automatically uses unique_ptr<ui::Event> instead of mus::mojom::Event
when generating C++ header files for mojom interfaces, and events are
automatically serializied/deserialized and validated when sending/receiving over
mojo IPC.

BUG=578206
==========

to

==========
StructTraits to map mus::mojom::Event to unique_ptr<ui::Event>.

This CL defines StructTraits<mus::mojom::Event, unique_ptr<ui::Event>>. With
this mojo automatically uses unique_ptr<ui::Event> instead of mus::mojom::Event
when generating C++ header files for mojom interfaces, and events are
automatically serializied/deserialized and validated when sending/receiving over
mojo IPC.

BUG=578206

Committed: https://crrev.com/73be633b0f9b71668ab204c22bd14742bd6c7ead
Cr-Commit-Position: refs/heads/master@{#398107}
==========

[commit-bot: I haz the power, 2016-06-06 20:15:01]

Patchset 21 (id:??) landed as
https://crrev.com/73be633b0f9b71668ab204c22bd14742bd6c7ead
Cr-Commit-Position: refs/heads/master@{#398107}