IPC::ParamTraits for ui::Event (towards ui::Events over mojo IPC)

Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over mojo IPC). Support for (scoped) pointers for abstract ui::Event objects implemented using ui::ScopedEvent, and trusting ui::EventType information to instantiate the correct concrete type.

BUG=584687
Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}

Committed: https://crrev.com/49745c73debc0b61be1f35ff6b01fe8ca6a14356
Cr-Commit-Position: refs/heads/master@{#379348}

[Mark Dittmer, 2016-02-12 17:04:41]

Description was changed from

==========
WIP: git cl format


WIP: Fix erroneous-dealloc bug in event serialization (events.h) and write
simple tests for the rest of event types. Gesture events tests currently
failing.


WIP: Add first few unit tests, implementations required for testing, and a fix
to PointerDetails serialization (yay tests)


WIP: Clean up ui::Event param traits code


WIP: ParamTraits for ui::Event, now with sizing


WIP: ParamTraits for ui::Event now building


WIP: Implement ParamTraits for ui::Event and co.

BUG=
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.
==========

[Mark Dittmer, 2016-02-12 17:04:42]

markdittmer@chromium.org changed reviewers:
+ sadrul@chromium.org

[Mark Dittmer, 2016-02-17 15:56:07]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687
==========

[sadrul, 2016-02-17 21:05:30]

sadrul@chromium.org changed reviewers:
+ rockot@chromium.org

[sadrul, 2016-02-17 21:05:31]

+rockot@

So turns out I don't actually know about this stuff enough to be able to review.
rockot@ would be able to give you the right feedback. (sorry for sitting on it
for this long)

https://codereview.chromium.org/1695783002/diff/20001/ui/events/ipc/event_par...
File ui/events/ipc/event_param_traits.h (right):

https://codereview.chromium.org/1695783002/diff/20001/ui/events/ipc/event_par...
ui/events/ipc/event_param_traits.h:33: //   int source_device_id_;
I think we want to send source_device_id_ and latency_ too (LatencyInfo already
has its param-traits). But we can revisit that later.

[sadrul, 2016-02-17 21:06:14]

Oh, about the CL description: we don't have any set rules, but a lot of folks
try to follow the git-style of commit messages:
http://chris.beams.io/posts/git-commit/#separate

[Ken Rockot(use gerrit already), 2016-02-17 21:31:47]

Sorry for lacking some context here, but why are you introducing new ParamTraits
if they're only going to be used for Mojo IPC? The only reason we have any
support for "native" types in mojom is to reuse existing ParamTraits while
transitioning existing messages to Mojo.

We have mojo::StructTraits to support mapping native types to (non-native) mojom
types for serialization. That is, mojom defines the wire format, and you
implement some traits which map your type to the wire format. I'd rather work
with you to make StructTraits sufficient for this use case instead of seeing you
introduce new ParamTraits.

[Ken Rockot(use gerrit already), 2016-02-17 21:47:53]

After taking a closer look, I see there's enough complexity to supporting
ui::Event with StructTraits that I don't want to block you on that. But I will
make it a priority to get support up to snuff so this can be converted to that
ASAP.

[Ken Rockot(use gerrit already), 2016-02-17 22:05:22]

generally lgtm but you'll need a security review for the IPC stuff anyway

https://codereview.chromium.org/1695783002/diff/20001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1695783002/diff/20001/ui/events/event.h#newco...
ui/events/event.h:241: friend struct IPC::ParamTraits<ui::ScopedEvent>;
This is really for Chrome IPC serialization. Here and everywhere else.

https://codereview.chromium.org/1695783002/diff/20001/ui/events/gesture_event...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/1695783002/diff/20001/ui/events/gesture_event...
ui/events/gesture_event_details.h:200: bool operator==(const
GestureEventDetails& other) const {
Maybe it would be better to expose something like IsEqualForTesting if you
actually want this to be used in testing only.

[Mark Dittmer, 2016-02-18 14:51:05]

Comments noted/fixed. rockot@, RSVP re: equality operator vs.
SpeciallyNamedNotionOfEquality.

https://codereview.chromium.org/1695783002/diff/20001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1695783002/diff/20001/ui/events/event.h#newco...
ui/events/event.h:241: friend struct IPC::ParamTraits<ui::ScopedEvent>;
On 2016/02/17 22:05:21, Ken Rockot wrote:
> This is really for Chrome IPC serialization. Here and everywhere else.

Done.

https://codereview.chromium.org/1695783002/diff/20001/ui/events/gesture_event...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/1695783002/diff/20001/ui/events/gesture_event...
ui/events/gesture_event_details.h:200: bool operator==(const
GestureEventDetails& other) const {
On 2016/02/17 22:05:21, Ken Rockot wrote:
> Maybe it would be better to expose something like IsEqualForTesting if you
> actually want this to be used in testing only.

Could do. There are two reasons I prefer implementing ==

(1) This really is what's meant by equality for this type (perhaps this
assertion betrays the comment above).
(2) IMHO, we get more readable test code and test code output being able to
ASSERT_EQ(ged1, ged2).

If it better follows our operator-impl + testing idioms to follow your
suggestion, feel free to push back and I'll change it.

https://codereview.chromium.org/1695783002/diff/20001/ui/events/ipc/event_par...
File ui/events/ipc/event_param_traits.h (right):

https://codereview.chromium.org/1695783002/diff/20001/ui/events/ipc/event_par...
ui/events/ipc/event_param_traits.h:33: //   int source_device_id_;
On 2016/02/17 21:05:31, sadrul wrote:
> I think we want to send source_device_id_ and latency_ too (LatencyInfo
already
> has its param-traits). But we can revisit that later.

Acknowledged.

[Mark Dittmer, 2016-02-18 14:57:27]

markdittmer@chromium.org changed reviewers:
+ kenrb@chromium.org

[Mark Dittmer, 2016-02-18 16:16:10]

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

[commit-bot: I haz the power, 2016-02-18 16:16:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/40001

[commit-bot: I haz the power, 2016-02-18 16:21:56]

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

[commit-bot: I haz the power, 2016-02-18 16:21:57]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mark Dittmer, 2016-02-18 17:23:42]

markdittmer@chromium.org changed reviewers:
+ tsepez@chromium.org

[Mark Dittmer, 2016-02-18 17:24:24]

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

[commit-bot: I haz the power, 2016-02-18 17:24:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/60001

[commit-bot: I haz the power, 2016-02-18 17:36:32]

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

[commit-bot: I haz the power, 2016-02-18 17:36:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Mark Dittmer, 2016-02-18 18:16:07]

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

[commit-bot: I haz the power, 2016-02-18 18:20:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/60001

[commit-bot: I haz the power, 2016-02-18 18:31:17]

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

[commit-bot: I haz the power, 2016-02-18 18:31:18]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Mark Dittmer, 2016-02-18 18:33:38]

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

[commit-bot: I haz the power, 2016-02-18 18:39:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/80001

[Tom Sepez, 2016-02-18 18:46:09]

tsepez@chromium.org changed reviewers:
+ jam@chromium.org

[Tom Sepez, 2016-02-18 18:46:10]

+jam so we can discuss IPC serializers as friends of higher layer classes (vs.
public members) vs ...

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:36: 
nit: using ScopedEvent = scoped_ptr<Event>;

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
jam@ and I have argued about this in the past -- it's a layering violation for
this code to know about IPC traits.  I'll add him to see if he still feels the
same way.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:273: explicit CancelModeEvent(EventType type,
Nit: no |explicit| as it's not a single-arg ctor.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:281: class MouseWheelEvent;
nit: move to top

https://codereview.chromium.org/1695783002/diff/80001/ui/events/gesture_event...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/gesture_event...
ui/events/gesture_event_details.h:199: // Supports comparison over internal
structures for testing.
nit: move up to public section.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
File ui/events/ipc/event_param_traits.cc (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
ui/events/ipc/event_param_traits.cc:106: #define CAST_EVENT(T)
(*static_cast<T*>(p.get()))
nit: I'd suggest removing CAST_EVENT and SIZE_EVENT at the cost of source size
to increase transparency.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
File ui/events/ipc/event_param_traits_unittest.cc (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
ui/events/ipc/event_param_traits_unittest.cc:21: #define CAST_EVENT(T, e)
(*static_cast<const T*>(e.get()))
nit: just write it out rather than defining these macros.

[commit-bot: I haz the power, 2016-02-18 19:11:50]

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

[commit-bot: I haz the power, 2016-02-18 19:11:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mark Dittmer, 2016-02-18 19:12:57]

Implemented most comments from tsepez. RSVP a couple.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:36: 
On 2016/02/18 18:46:10, Tom Sepez wrote:
> nit: using ScopedEvent = scoped_ptr<Event>;

Done.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
On 2016/02/18 18:46:10, Tom Sepez wrote:
> jam@ and I have argued about this in the past -- it's a layering violation for
> this code to know about IPC traits.  I'll add him to see if he still feels the
> same way.

What is the proposed implementation alternative? Require getters + setters for
all serializable data?

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:273: explicit CancelModeEvent(EventType type,
On 2016/02/18 18:46:10, Tom Sepez wrote:
> Nit: no |explicit| as it's not a single-arg ctor.

Done.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:281: class MouseWheelEvent;
On 2016/02/18 18:46:10, Tom Sepez wrote:
> nit: move to top

Done.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/gesture_event...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/gesture_event...
ui/events/gesture_event_details.h:199: // Supports comparison over internal
structures for testing.
On 2016/02/18 18:46:10, Tom Sepez wrote:
> nit: move up to public section.

Done.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
File ui/events/ipc/event_param_traits.cc (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
ui/events/ipc/event_param_traits.cc:106: #define CAST_EVENT(T)
(*static_cast<T*>(p.get()))
On 2016/02/18 18:46:10, Tom Sepez wrote:
> nit: I'd suggest removing CAST_EVENT and SIZE_EVENT at the cost of source size
> to increase transparency.

Can we meet in the middle and just eliminate CAST_EVENT? SIZE_EVENT is used as
an implName for EVENT_IMPL.

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
File ui/events/ipc/event_param_traits_unittest.cc (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/ipc/event_par...
ui/events/ipc/event_param_traits_unittest.cc:21: #define CAST_EVENT(T, e)
(*static_cast<const T*>(e.get()))
On 2016/02/18 18:46:10, Tom Sepez wrote:
> nit: just write it out rather than defining these macros.

In the case of all these macros, or CAST_EVENT in particular? The
limit/float/int ones really simplify some many-param-calls.

[jam, 2016-02-18 21:01:26]

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
On 2016/02/18 19:12:56, Mark Dittmer wrote:
> On 2016/02/18 18:46:10, Tom Sepez wrote:
> > jam@ and I have argued about this in the past -- it's a layering violation
for
> > this code to know about IPC traits.  I'll add him to see if he still feels
the
> > same way.
> 
> What is the proposed implementation alternative? Require getters + setters for
> all serializable data?

I care less about this now. 

My main question is who will use these traits? If it's just mus, why aren't the
ipc related code in mus' directory?

[Mark Dittmer, 2016-02-22 15:52:11]

On 2016/02/18 21:01:26, jam wrote:
> https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
> On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > jam@ and I have argued about this in the past -- it's a layering violation
> for
> > > this code to know about IPC traits.  I'll add him to see if he still feels
> the
> > > same way.
> > 
> > What is the proposed implementation alternative? Require getters + setters
for
> > all serializable data?
> 
> I care less about this now. 
> 
> My main question is who will use these traits? If it's just mus, why aren't
the
> ipc related code in mus' directory?

tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable change:
components/mus/DEPS now has +ipc. Previously, only mojom-spec (not Chrome
IPC-spec for use over mojo) was used in mus.

Does this satisfy remaining concerns?

[Fady Samuel, 2016-02-22 16:08:55]

On 2016/02/22 15:52:11, Mark Dittmer wrote:
> On 2016/02/18 21:01:26, jam wrote:
> > https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> > File ui/events/event.h (right):
> > 
> >
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> > ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
> > On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > > jam@ and I have argued about this in the past -- it's a layering
violation
> > for
> > > > this code to know about IPC traits.  I'll add him to see if he still
feels
> > the
> > > > same way.
> > > 
> > > What is the proposed implementation alternative? Require getters + setters
> for
> > > all serializable data?
> > 
> > I care less about this now. 
> > 
> > My main question is who will use these traits? If it's just mus, why aren't
> the
> > ipc related code in mus' directory?
> 
> tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable change:
> components/mus/DEPS now has +ipc. Previously, only mojom-spec (not Chrome
> IPC-spec for use over mojo) was used in mus.
> 
> Does this satisfy remaining concerns?

jam@: Do we have a general guideline for where ParamTraits, mojoms and
IPC-related structs should live? We chatted about this before, but I can't
recall if a decision was made.

[jam, 2016-02-22 17:13:24]

On 2016/02/22 16:08:55, Fady Samuel wrote:
> On 2016/02/22 15:52:11, Mark Dittmer wrote:
> > On 2016/02/18 21:01:26, jam wrote:
> > > https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> > > File ui/events/event.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> > > ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
> > > On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > > > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > > > jam@ and I have argued about this in the past -- it's a layering
> violation
> > > for
> > > > > this code to know about IPC traits.  I'll add him to see if he still
> feels
> > > the
> > > > > same way.
> > > > 
> > > > What is the proposed implementation alternative? Require getters +
setters
> > for
> > > > all serializable data?
> > > 
> > > I care less about this now. 
> > > 
> > > My main question is who will use these traits? If it's just mus, why
aren't
> > the
> > > ipc related code in mus' directory?
> > 
> > tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable change:
> > components/mus/DEPS now has +ipc. Previously, only mojom-spec (not Chrome
> > IPC-spec for use over mojo) was used in mus.
> > 
> > Does this satisfy remaining concerns?
> 
> jam@: Do we have a general guideline for where ParamTraits, mojoms and
> IPC-related structs should live? We chatted about this before, but I can't
> recall if a decision was made.

mojoms should live alongside the directory they're used/consumed in.
https://www.chromium.org/developers/content-module/content-api has a guideline
for content. similarly for blink, the mojoms that describe interfaces that blink
either implements or uses to communicate to its embedder should be in
third_party/webkit/public. ditto for mus.

for IPC traits, what we previously did is avoid putting them in layers that
don't know about IPC. i.e. we'd avoid making net or base know about IPC. in this
specific example, since only mus uses that struct over IPC, we can keep the
traits there for now. if later we need to use it in other modules, we can
reconsider where the best place is.

[Mark Dittmer, 2016-02-22 19:25:31]

On 2016/02/22 17:13:24, jam wrote:
> On 2016/02/22 16:08:55, Fady Samuel wrote:
> > On 2016/02/22 15:52:11, Mark Dittmer wrote:
> > > On 2016/02/18 21:01:26, jam wrote:
> > > > https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> > > > File ui/events/event.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> > > > ui/events/event.h:247: friend struct IPC::ParamTraits<ui::ScopedEvent>;
> > > > On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > > > > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > > > > jam@ and I have argued about this in the past -- it's a layering
> > violation
> > > > for
> > > > > > this code to know about IPC traits.  I'll add him to see if he still
> > feels
> > > > the
> > > > > > same way.
> > > > > 
> > > > > What is the proposed implementation alternative? Require getters +
> setters
> > > for
> > > > > all serializable data?
> > > > 
> > > > I care less about this now. 
> > > > 
> > > > My main question is who will use these traits? If it's just mus, why
> aren't
> > > the
> > > > ipc related code in mus' directory?
> > > 
> > > tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable
change:
> > > components/mus/DEPS now has +ipc. Previously, only mojom-spec (not Chrome
> > > IPC-spec for use over mojo) was used in mus.
> > > 
> > > Does this satisfy remaining concerns?
> > 
> > jam@: Do we have a general guideline for where ParamTraits, mojoms and
> > IPC-related structs should live? We chatted about this before, but I can't
> > recall if a decision was made.
> 
> mojoms should live alongside the directory they're used/consumed in.
> https://www.chromium.org/developers/content-module/content-api has a guideline
> for content. similarly for blink, the mojoms that describe interfaces that
blink
> either implements or uses to communicate to its embedder should be in
> third_party/webkit/public. ditto for mus.
> 
> for IPC traits, what we previously did is avoid putting them in layers that
> don't know about IPC. i.e. we'd avoid making net or base know about IPC. in
this
> specific example, since only mus uses that struct over IPC, we can keep the
> traits there for now. if later we need to use it in other modules, we can
> reconsider where the best place is.

fsamuel@, if I'm reading jam@ correctly, the components/mus/public/cpp/... is
the appropriate place for the ParamTraits, at least for now. Is there anything
else out of order here?

[Fady Samuel, 2016-02-23 15:30:06]

On 2016/02/22 19:25:31, Mark Dittmer wrote:
> On 2016/02/22 17:13:24, jam wrote:
> > On 2016/02/22 16:08:55, Fady Samuel wrote:
> > > On 2016/02/22 15:52:11, Mark Dittmer wrote:
> > > > On 2016/02/18 21:01:26, jam wrote:
> > > > >
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> > > > > File ui/events/event.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> > > > > ui/events/event.h:247: friend struct
IPC::ParamTraits<ui::ScopedEvent>;
> > > > > On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > > > > > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > > > > > jam@ and I have argued about this in the past -- it's a layering
> > > violation
> > > > > for
> > > > > > > this code to know about IPC traits.  I'll add him to see if he
still
> > > feels
> > > > > the
> > > > > > > same way.
> > > > > > 
> > > > > > What is the proposed implementation alternative? Require getters +
> > setters
> > > > for
> > > > > > all serializable data?
> > > > > 
> > > > > I care less about this now. 
> > > > > 
> > > > > My main question is who will use these traits? If it's just mus, why
> > aren't
> > > > the
> > > > > ipc related code in mus' directory?
> > > > 
> > > > tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable
> change:
> > > > components/mus/DEPS now has +ipc. Previously, only mojom-spec (not
Chrome
> > > > IPC-spec for use over mojo) was used in mus.
> > > > 
> > > > Does this satisfy remaining concerns?
> > > 
> > > jam@: Do we have a general guideline for where ParamTraits, mojoms and
> > > IPC-related structs should live? We chatted about this before, but I can't
> > > recall if a decision was made.
> > 
> > mojoms should live alongside the directory they're used/consumed in.
> > https://www.chromium.org/developers/content-module/content-api has a
guideline
> > for content. similarly for blink, the mojoms that describe interfaces that
> blink
> > either implements or uses to communicate to its embedder should be in
> > third_party/webkit/public. ditto for mus.
> > 
> > for IPC traits, what we previously did is avoid putting them in layers that
> > don't know about IPC. i.e. we'd avoid making net or base know about IPC. in
> this
> > specific example, since only mus uses that struct over IPC, we can keep the
> > traits there for now. if later we need to use it in other modules, we can
> > reconsider where the best place is.
> 
> fsamuel@, if I'm reading jam@ correctly, the components/mus/public/cpp/... is
> the appropriate place for the ParamTraits, at least for now. Is there anything
> else out of order here?

lgtm

[jam, 2016-02-23 16:45:15]

On 2016/02/22 19:25:31, Mark Dittmer wrote:
> On 2016/02/22 17:13:24, jam wrote:
> > On 2016/02/22 16:08:55, Fady Samuel wrote:
> > > On 2016/02/22 15:52:11, Mark Dittmer wrote:
> > > > On 2016/02/18 21:01:26, jam wrote:
> > > > >
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> > > > > File ui/events/event.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> > > > > ui/events/event.h:247: friend struct
IPC::ParamTraits<ui::ScopedEvent>;
> > > > > On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > > > > > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > > > > > jam@ and I have argued about this in the past -- it's a layering
> > > violation
> > > > > for
> > > > > > > this code to know about IPC traits.  I'll add him to see if he
still
> > > feels
> > > > > the
> > > > > > > same way.
> > > > > > 
> > > > > > What is the proposed implementation alternative? Require getters +
> > setters
> > > > for
> > > > > > all serializable data?
> > > > > 
> > > > > I care less about this now. 
> > > > > 
> > > > > My main question is who will use these traits? If it's just mus, why
> > aren't
> > > > the
> > > > > ipc related code in mus' directory?
> > > > 
> > > > tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable
> change:
> > > > components/mus/DEPS now has +ipc. Previously, only mojom-spec (not
Chrome
> > > > IPC-spec for use over mojo) was used in mus.
> > > > 
> > > > Does this satisfy remaining concerns?
> > > 
> > > jam@: Do we have a general guideline for where ParamTraits, mojoms and
> > > IPC-related structs should live? We chatted about this before, but I can't
> > > recall if a decision was made.
> > 
> > mojoms should live alongside the directory they're used/consumed in.
> > https://www.chromium.org/developers/content-module/content-api has a
guideline
> > for content. similarly for blink, the mojoms that describe interfaces that
> blink
> > either implements or uses to communicate to its embedder should be in
> > third_party/webkit/public. ditto for mus.
> > 
> > for IPC traits, what we previously did is avoid putting them in layers that
> > don't know about IPC. i.e. we'd avoid making net or base know about IPC. in
> this
> > specific example, since only mus uses that struct over IPC, we can keep the
> > traits there for now. if later we need to use it in other modules, we can
> > reconsider where the best place is.
> 
> fsamuel@, if I'm reading jam@ correctly, the components/mus/public/cpp/... is
> the appropriate place for the ParamTraits, at least for now. Is there anything
> else out of order here?

Sorry for not being clear. I meant components/mus. Up to you where exactly, but
I don't think it should be components/mus/public/cpp, as that's where the client
library is and this is just internal implementation details. so i would exclude
components/mus/public altogether.

[Fady Samuel, 2016-02-23 16:47:58]

On 2016/02/23 16:45:15, jam wrote:
> On 2016/02/22 19:25:31, Mark Dittmer wrote:
> > On 2016/02/22 17:13:24, jam wrote:
> > > On 2016/02/22 16:08:55, Fady Samuel wrote:
> > > > On 2016/02/22 15:52:11, Mark Dittmer wrote:
> > > > > On 2016/02/18 21:01:26, jam wrote:
> > > > > >
> https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h
> > > > > > File ui/events/event.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1695783002/diff/80001/ui/events/event.h#newco...
> > > > > > ui/events/event.h:247: friend struct
> IPC::ParamTraits<ui::ScopedEvent>;
> > > > > > On 2016/02/18 19:12:56, Mark Dittmer wrote:
> > > > > > > On 2016/02/18 18:46:10, Tom Sepez wrote:
> > > > > > > > jam@ and I have argued about this in the past -- it's a layering
> > > > violation
> > > > > > for
> > > > > > > > this code to know about IPC traits.  I'll add him to see if he
> still
> > > > feels
> > > > > > the
> > > > > > > > same way.
> > > > > > > 
> > > > > > > What is the proposed implementation alternative? Require getters +
> > > setters
> > > > > for
> > > > > > > all serializable data?
> > > > > > 
> > > > > > I care less about this now. 
> > > > > > 
> > > > > > My main question is who will use these traits? If it's just mus, why
> > > aren't
> > > > > the
> > > > > > ipc related code in mus' directory?
> > > > > 
> > > > > tsepez@ and jam@, I moved the ParamTraits to components/mus. Notable
> > change:
> > > > > components/mus/DEPS now has +ipc. Previously, only mojom-spec (not
> Chrome
> > > > > IPC-spec for use over mojo) was used in mus.
> > > > > 
> > > > > Does this satisfy remaining concerns?
> > > > 
> > > > jam@: Do we have a general guideline for where ParamTraits, mojoms and
> > > > IPC-related structs should live? We chatted about this before, but I
can't
> > > > recall if a decision was made.
> > > 
> > > mojoms should live alongside the directory they're used/consumed in.
> > > https://www.chromium.org/developers/content-module/content-api has a
> guideline
> > > for content. similarly for blink, the mojoms that describe interfaces that
> > blink
> > > either implements or uses to communicate to its embedder should be in
> > > third_party/webkit/public. ditto for mus.
> > > 
> > > for IPC traits, what we previously did is avoid putting them in layers
that
> > > don't know about IPC. i.e. we'd avoid making net or base know about IPC.
in
> > this
> > > specific example, since only mus uses that struct over IPC, we can keep
the
> > > traits there for now. if later we need to use it in other modules, we can
> > > reconsider where the best place is.
> > 
> > fsamuel@, if I'm reading jam@ correctly, the components/mus/public/cpp/...
is
> > the appropriate place for the ParamTraits, at least for now. Is there
anything
> > else out of order here?
> 
> Sorry for not being clear. I meant components/mus. Up to you where exactly,
but
> I don't think it should be components/mus/public/cpp, as that's where the
client
> library is and this is just internal implementation details. so i would
exclude
> components/mus/public altogether.

Ok, agreed, it's used by both the client and server. A while back I created a
common directory for that. components/mus/common.

[jam, 2016-02-23 18:42:07]

lgtm

[Mark Dittmer, 2016-02-23 18:49:52]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-02-23 18:49:52]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1695783002/#ps200001
(title: "Move event_param_traits from components/mus/public/cpp to
components/mus/common")

[commit-bot: I haz the power, 2016-02-23 18:54:49]

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

[commit-bot: I haz the power, 2016-02-23 19:15:00]

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

[commit-bot: I haz the power, 2016-02-23 19:15:01]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Mark Dittmer, 2016-02-23 19:43:33]

Remove erroneous ui/events dep on ipc; add missing components/mus/common dep on
ipc and ui/events

[Mark Dittmer, 2016-02-23 19:45:26]

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

[commit-bot: I haz the power, 2016-02-23 19:50:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/220001

[Tom Sepez, 2016-02-23 19:53:06]

lgtm

https://codereview.chromium.org/1695783002/diff/220001/components/mus/DEPS
File components/mus/DEPS (right):

https://codereview.chromium.org/1695783002/diff/220001/components/mus/DEPS#ne...
components/mus/DEPS:11: "+mojo/shell",
nit: duplicate.

https://codereview.chromium.org/1695783002/diff/220001/components/mus/common/...
File components/mus/common/event_param_traits_unittest.cc (right):

https://codereview.chromium.org/1695783002/diff/220001/components/mus/common/...
components/mus/common/event_param_traits_unittest.cc:24: #define LIMIT(T)
std::numeric_limits<T>
Nit: I'd still prefer that you write this out, but I'm not going to insist.

[Mark Dittmer, 2016-02-23 20:08:07]

Add components/mus/common dep //ui/gfx/ipc

[commit-bot: I haz the power, 2016-02-23 20:11:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Mark Dittmer, 2016-02-23 20:12:46]

Address nits from tsepez

[Mark Dittmer, 2016-02-23 20:12:50]

https://codereview.chromium.org/1695783002/diff/220001/components/mus/DEPS
File components/mus/DEPS (right):

https://codereview.chromium.org/1695783002/diff/220001/components/mus/DEPS#ne...
components/mus/DEPS:11: "+mojo/shell",
On 2016/02/23 19:53:06, Tom Sepez wrote:
> nit: duplicate.

Done.

https://codereview.chromium.org/1695783002/diff/220001/components/mus/common/...
File components/mus/common/event_param_traits_unittest.cc (right):

https://codereview.chromium.org/1695783002/diff/220001/components/mus/common/...
components/mus/common/event_param_traits_unittest.cc:24: #define LIMIT(T)
std::numeric_limits<T>
On 2016/02/23 19:53:06, Tom Sepez wrote:
> Nit: I'd still prefer that you write this out, but I'm not going to insist.

Done.

[Mark Dittmer, 2016-02-23 20:13:11]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-02-23 20:13:12]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, fsamuel@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1695783002/#ps260001
(title: "Address nits from tsepez")

[commit-bot: I haz the power, 2016-02-23 20:16:39]

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

[commit-bot: I haz the power, 2016-02-23 22:16:46]

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

[commit-bot: I haz the power, 2016-02-23 22:16:48]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Mark Dittmer, 2016-03-02 13:43:55]

rebase

[Mark Dittmer, 2016-03-02 13:45:14]

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

[commit-bot: I haz the power, 2016-03-02 13:45:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/280001

[commit-bot: I haz the power, 2016-03-02 13:59:59]

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

[commit-bot: I haz the power, 2016-03-02 14:00:01]

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

[Mark Dittmer, 2016-03-02 14:39:08]

Fix: typeof(cancelable) = bool

[Mark Dittmer, 2016-03-02 14:40:03]

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

[commit-bot: I haz the power, 2016-03-02 14:40:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/300001

[commit-bot: I haz the power, 2016-03-02 14:59:30]

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

[commit-bot: I haz the power, 2016-03-02 14:59:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mark Dittmer, 2016-03-02 16:09:09]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-03-02 16:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, tsepez@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1695783002/#ps300001
(title: "Fix: typeof(cancelable) = bool")

[commit-bot: I haz the power, 2016-03-02 16:09:22]

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

[commit-bot: I haz the power, 2016-03-02 17:19:40]

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

[commit-bot: I haz the power, 2016-03-02 17:19:41]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Mark Dittmer, 2016-03-02 19:15:11]

markdittmer@chromium.org changed reviewers:
+ sky@chromium.org

[Mark Dittmer, 2016-03-02 19:30:33]

Remove dead MetroViewerHostMsg_MouseButton from metro_viewer; conflicts with new
param traits

[Mark Dittmer, 2016-03-02 19:31:09]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-03-02 19:31:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, tsepez@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1695783002/#ps320001
(title: "Remove dead MetroViewerHostMsg_MouseButton from metro_viewer; conflicts
with new param traits")

[commit-bot: I haz the power, 2016-03-02 19:31:47]

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

[commit-bot: I haz the power, 2016-03-02 20:45:18]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687
==========

[commit-bot: I haz the power, 2016-03-02 20:45:20]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-03-02 20:47:05]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}
==========

[commit-bot: I haz the power, 2016-03-02 20:47:07]

Patchset 17 (id:??) landed as
https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

[Dmitry Titov, 2016-03-02 21:39:40]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1755703004/ by dimich@chromium.org.

The reason for reverting is: It broke compile on:
https://build.chromium.org/p/chromium.win/builders/Win8%20GN%20%28dbg%29

BUG=591488.

[Mark Dittmer, 2016-03-03 13:00:15]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}
==========

[Mark Dittmer, 2016-03-03 20:03:14]

Make mus_common a component to fix Windows build failure

[Fady Samuel, 2016-03-03 20:07:03]

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

[commit-bot: I haz the power, 2016-03-03 20:07:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695783002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695783002/340001

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

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

[commit-bot: I haz the power, 2016-03-03 21:22:24]

Dry run: This issue passed the CQ dry run.

[Mark Dittmer, 2016-03-03 21:25:20]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-03-03 21:25:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, tsepez@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1695783002/#ps340001
(title: "Make mus_common a component to fix Windows build failure")

[commit-bot: I haz the power, 2016-03-03 21:26:04]

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

[commit-bot: I haz the power, 2016-03-03 21:33:22]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}
==========

[commit-bot: I haz the power, 2016-03-03 21:33:23]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-03-03 21:34:41]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}
==========

[commit-bot: I haz the power, 2016-03-03 21:34:43]

Patchset 18 (id:??) landed as
https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}

[Ken Rockot(use gerrit already), 2016-03-03 22:36:02]

On 2016/03/03 at 21:34:43, commit-bot wrote:
> Patchset 18 (id:??) landed as
https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
> Cr-Commit-Position: refs/heads/master@{#379094}

This appears to be breaking Windows 8 GN compile still

[Dmitry Titov, 2016-03-03 22:39:08]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/1765693002/ by dimich@chromium.org.

The reason for reverting is: Breaks compile:
https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29....

[Mark Dittmer, 2016-03-04 16:29:09]

Explicate MUS_COMMON_EXPORT in header and impl

[jonross, 2016-03-04 16:34:07]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}
==========

[jonross, 2016-03-04 16:34:07]

jonross@chromium.org changed reviewers:
+ jonross@chromium.org

[Mark Dittmer, 2016-03-04 19:07:21]

The CQ bit was checked by markdittmer@chromium.org

[Mark Dittmer, 2016-03-04 19:07:22]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, tsepez@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1695783002/#ps360001
(title: "Explicate MUS_COMMON_EXPORT in header and impl")

[commit-bot: I haz the power, 2016-03-04 19:07:50]

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

[commit-bot: I haz the power, 2016-03-04 20:53:32]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}
==========

[commit-bot: I haz the power, 2016-03-04 20:53:34]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-03-04 20:56:06]

Description was changed from

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}
==========

to

==========
Implements and tests IPC::ParamTraits for ui::Event (towards ui::Events over
mojo IPC). Support for (scoped) pointers for abstract ui::Event objects
implemented using ui::ScopedEvent, and trusting ui::EventType information to
instantiate the correct concrete type.

BUG=584687

Committed: https://crrev.com/482ddfefe2602f440146eebf4b56e8e040fb81fa
Cr-Commit-Position: refs/heads/master@{#378821}

Committed: https://crrev.com/74b231ad33ec44bc2662fef2831a3cd90e1f3834
Cr-Commit-Position: refs/heads/master@{#379094}

Committed: https://crrev.com/49745c73debc0b61be1f35ff6b01fe8ca6a14356
Cr-Commit-Position: refs/heads/master@{#379348}
==========

[commit-bot: I haz the power, 2016-03-04 20:56:09]

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

[brucedawson, 2016-03-08 21:43:35]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-03-08 21:43:37]

https://codereview.chromium.org/1695783002/diff/360001/components/mus/common/...
File components/mus/common/event_param_traits_unittest.cc (right):

https://codereview.chromium.org/1695783002/diff/360001/components/mus/common/...
components/mus/common/event_param_traits_unittest.cc:90: ASSERT_EQ(!!a->flags()
& MouseEventFlags::EF_FROM_TOUCH,
This ASSERT_EQ doesn't make sense. Each parameter is a bool masked against an
enum such that both parameters will always be zero. VS 2015 refuses to compile
it.

Presumably either the !! should be deleted, or parentheses are needed around the
& expression.

https://codereview.chromium.org/1773813004 is a possible fix.