ui: events: Add a class to hold common touch and stylus properties

ui: events: Add a class to hold common touch and stylus properties

This change introduces a new class, PointerDetails that holds event properties
common to touch and stylus type devices and which aligns with the design of the
Pointer Events specification. This allows MouseEvent (which is most appropriate
for stylus devices) and TouchEvent to have a common set of properties without
requiring duplication.

TEST=All existing unit tests pass, newly introduced tests pass. Interactive
testing on link shows touch and mouse not impacted.
BUG=516706
Committed: https://crrev.com/f62ea41d40ecf774dbb44233252388a55c3cf0ee
Cr-Commit-Position: refs/heads/master@{#344250}

[robert.bradford, 2015-07-29 17:27:53]

robert.bradford@intel.com changed reviewers:
+ rbyers@chromium.org, sadrul@chromium.org

[robert.bradford, 2015-07-29 17:27:54]

This CL (and the dependent one) is just my first thought about how we could
handle the new fields needed for PointerEvents stylus in blink whilst allowing
ash and the rest of the UI to behave as expected.

For Ozone and stylus support a new constructor on MouseEvent to expose the extra
fields is all that would be needed to ensure that stylus behaves like a mouse in
the UI and that web content can see the advanced stylus aspects.

For X11 i'm currently prototyping what it would look like, but I think it should
be pretty straightforward to extract the valuators.

Any indication of whether this seems a reasonable direction would be much
appreciated.

[sadrul, 2015-07-30 18:23:45]

On 2015/07/29 17:27:54, robert.bradford wrote:
> This CL (and the dependent one) is just my first thought about how we could
> handle the new fields needed for PointerEvents stylus in blink whilst allowing
> ash and the rest of the UI to behave as expected.
> 
> For Ozone and stylus support a new constructor on MouseEvent to expose the
extra
> fields is all that would be needed to ensure that stylus behaves like a mouse
in
> the UI and that web content can see the advanced stylus aspects.
> 
> For X11 i'm currently prototyping what it would look like, but I think it
should
> be pretty straightforward to extract the valuators.
> 
> Any indication of whether this seems a reasonable direction would be much
> appreciated.

Do we need a new Event class? Can we instead have a PointerDetails struct (like
GestureEventDetails) that has this information, and both MouseEvent and
TouchEvent contains this? I feel like that would be simpler.

[Rick Byers, 2015-07-30 18:31:56]

On 2015/07/30 18:23:45, sadrul wrote:
> On 2015/07/29 17:27:54, robert.bradford wrote:
> > This CL (and the dependent one) is just my first thought about how we could
> > handle the new fields needed for PointerEvents stylus in blink whilst
allowing
> > ash and the rest of the UI to behave as expected.
> > 
> > For Ozone and stylus support a new constructor on MouseEvent to expose the
> extra
> > fields is all that would be needed to ensure that stylus behaves like a
mouse
> in
> > the UI and that web content can see the advanced stylus aspects.
> > 
> > For X11 i'm currently prototyping what it would look like, but I think it
> should
> > be pretty straightforward to extract the valuators.
> > 
> > Any indication of whether this seems a reasonable direction would be much
> > appreciated.
> 
> Do we need a new Event class? Can we instead have a PointerDetails struct
(like
> GestureEventDetails) that has this information, and both MouseEvent and
> TouchEvent contains this? I feel like that would be simpler.

Long term the right path is perhaps to unify MouseEvent and TouchEvent into a
single PointerEvent class.  Using a PointerDetails sounds like a good stepping
stone towards that, like we're doing with WebMouseEvent and WebTouchPoint
containing WebPointerProperties in blink.

[robert.bradford, 2015-08-10 18:51:41]

> 
> Do we need a new Event class? Can we instead have a PointerDetails struct
(like
> GestureEventDetails) that has this information, and both MouseEvent and
> TouchEvent contains this? I feel like that would be simpler.

Hi, sorry for the delay, I was investigating using the struct. And I quite like
it: I added a PointerEventDetails struct and then modified the MouseEvent ctor
(adding) and the TouchEvent ctor (replacing the radius and force).

The hugeness of this CL is just all the unittests changed to add the new ctor
param (I used a clang tool do all the MouseEvent changes.)

For the review, sadrul, can we just focus on the changes in
ui/events/event.[cc|h] and ui/events/event_constants.h as those are the only
really interesting changes.

I left TouchEvent::rotation_angle_ out as its not part of the the Pointer Events
spec.

This solution has the benefit over the intermediate class because we don't have
to add new ctors. or mutators. This is similar set of advantages over using
multiple inheritance (like how it's done in blink.)

As always, feedback appreciated!

[sadrul, 2015-08-11 04:46:37]

On 2015/08/10 18:51:41, robert.bradford wrote:
> > 
> > Do we need a new Event class? Can we instead have a PointerDetails struct
> (like
> > GestureEventDetails) that has this information, and both MouseEvent and
> > TouchEvent contains this? I feel like that would be simpler.
> 
> Hi, sorry for the delay, I was investigating using the struct. And I quite
like
> it: I added a PointerEventDetails struct and then modified the MouseEvent ctor
> (adding) and the TouchEvent ctor (replacing the radius and force).
> 
> The hugeness of this CL is just all the unittests changed to add the new ctor
> param (I used a clang tool do all the MouseEvent changes.)
> 
> For the review, sadrul, can we just focus on the changes in
> ui/events/event.[cc|h] and ui/events/event_constants.h as those are the only
> really interesting changes.
> 
> I left TouchEvent::rotation_angle_ out as its not part of the the Pointer
Events
> spec.
> 
> This solution has the benefit over the intermediate class because we don't
have
> to add new ctors. or mutators. This is similar set of advantages over using
> multiple inheritance (like how it's done in blink.)
> 
> As always, feedback appreciated!

I sent out tryjobs in a few bots. It looks like there are some build failures in
mojo. Would you mind taking a look at them?

[sadrul, 2015-08-11 04:48:47]

Also, please file a bug and reference it from BUG=, otherwise we might end up
landing the patch with 'BUG=<TO BE FILED>' :)

[Rick Byers, 2015-08-11 13:32:51]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[Rick Byers, 2015-08-11 13:44:54]

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

[Rick Byers, 2015-08-11 13:44:55]

The basic approach here seems reasonable to me.  I'd caution against
over-fitting to the pointer events API - you should do what makes the most sense
for chromium, it's blink's job to massage that into the right web-exposed API. 
You should continue to avoid letting the specific shape of WebPointerProperties
to leak up to chromium - that's entirely temporary and motivated primarily by
tactical concerns.

Moving myself to cc as my expertise is mainly on the blink side.  If you want
another set of eyes in addition to Sadrul, jdduke or tdresser are also good
reviewers for chromium event plumbing.

[robert.bradford, 2015-08-11 17:48:34]

robert.bradford@intel.com changed reviewers:
- rbyers@chromium.org

[robert.bradford, 2015-08-11 17:48:35]

sadrul@ please take a look at the latest patch, all the compile problems should
be fixed now (:-) unless some new patches have introduced new users of
MouseEvent/TouchEvent.)

I've also updated the description to make it reflect how this change actually
works now, including the BUG= line.

There are 2 TODOs in the code which I will address in follow-up CLs as this CL
is already pretty large and that will allow us to remove the compatability
accessors and radius mutators.

I also added some unit tests for the PointerEventDetails structure itself and
it's relationship with MouseEvent and TouchEvent.

[robert.bradford, 2015-08-11 17:58:48]

Also i've marked this change TBR by pkasting@ and derat@ as i'm not sure it's
fair to expect fine grained review of all those unittest changes.

[sadrul, 2015-08-13 15:44:17]

Sidenote: it's a bit depressing how much non-test code directly synthesizes
mouse-events.

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

https://codereview.chromium.org/1260453006/diff/40001/ui/events/event.h#newco...
ui/events/event.h:327: struct EVENTS_EXPORT PointerEventDetails {
Maybe EventPointerDetails (to be more inline with EventPointerType), or just
PointerDetails?

https://codereview.chromium.org/1260453006/diff/40001/ui/events/event.h#newco...
ui/events/event.h:412: const PointerEventDetails& details);
Would it be better to always initialize PointerEventDetails with
POINTER_TYPE_MOUSE, instead of receiving it through the constructor? The caller
can then change the type to _PEN when needed. That way, the MouseEvent
construction remains simple for most cases.

ditto for TouchEvent below.

WDYT?

[robert.bradford, 2015-08-13 17:42:50]

On 2015/08/13 15:44:17, sadrul wrote:
> Sidenote: it's a bit depressing how much non-test code directly synthesizes
> mouse-events.
> 
> https://codereview.chromium.org/1260453006/diff/40001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/1260453006/diff/40001/ui/events/event.h#newco...
> ui/events/event.h:327: struct EVENTS_EXPORT PointerEventDetails {
> Maybe EventPointerDetails (to be more inline with EventPointerType), or just
> PointerDetails?
> 
>
https://codereview.chromium.org/1260453006/diff/40001/ui/events/event.h#newco...
> ui/events/event.h:412: const PointerEventDetails& details);
> Would it be better to always initialize PointerEventDetails with
> POINTER_TYPE_MOUSE, instead of receiving it through the constructor? The
caller
> can then change the type to _PEN when needed. That way, the MouseEvent
> construction remains simple for most cases.
> 
> ditto for TouchEvent below.
> 
> WDYT?

Let me get some feedback before I jump in with an implementation: Rather than
having the details "logically immutable" (I know there are a couple places where
they get modified via mutators) and specified only at construct time the details
would be changed after the MouseEvent is constructed but before it is
dispatched.

e.g for stylus in ozone we would get the MouseEvent, and then get a pointer to
the details (rather than a const &) and then update the pointer type and the
other stylus fields. This would work but seems a little bit out of place as in
general it seems the Event classes are designed to be immutable once created. 

Alternatively what about adding a MouseEvent/TouchEvent ctor that lets you
specify a PointerDetails struct and in all other cases it will be filled with
default version? But the cost is a new ctor. Let me know what you prefer.

Both would result in much smaller CL :-)

[robert.bradford, 2015-08-14 13:45:34]

Hi sadrul@ WDYT about this latest patch.

I s/struct/class/ in alignment with the coding style.

[sadrul, 2015-08-17 14:44:43]

A small change. Otherwise, looks good!

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

https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h#newco...
ui/events/event.h:614: PointerDetails* pointer_details() { return
&pointer_details_; }
Can this return a const& instead?

The callers should go through the MouseEvent/TouchEvent to mutate it (e.g.
MouseEvent::set_pointer_type(...) and so on)

[robert.bradford, 2015-08-17 16:23:33]

On 2015/08/17 14:44:43, sadrul wrote:
> A small change. Otherwise, looks good!
> 
> https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h#newco...
> ui/events/event.h:614: PointerDetails* pointer_details() { return
> &pointer_details_; }
> Can this return a const& instead?
> 
> The callers should go through the MouseEvent/TouchEvent to mutate it (e.g.
> MouseEvent::set_pointer_type(...) and so on)

Done. I was conservative and only added those needed to MouseEvent/TouchEvent. 

I was just re-reviewing the Pointer Events specification and one thing I noticed
is that it says all devices that don't support pressure/force should provide 0.5
if any button is down. My feeling is that this is a detail that should be
handled in the code that maps the event to a blink event (i.e. in
content/browser/renderer_host) rather than in this CL. A very basic heuristic of
"if (button_down && force == 0.0f) force = 0.5f" should suffice. What do you
think?

[Rick Byers, 2015-08-17 16:33:06]

On 2015/08/17 16:23:33, robert.bradford wrote:
> On 2015/08/17 14:44:43, sadrul wrote:
> > A small change. Otherwise, looks good!
> > 
> > https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h
> > File ui/events/event.h (right):
> > 
> >
>
https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h#newco...
> > ui/events/event.h:614: PointerDetails* pointer_details() { return
> > &pointer_details_; }
> > Can this return a const& instead?
> > 
> > The callers should go through the MouseEvent/TouchEvent to mutate it (e.g.
> > MouseEvent::set_pointer_type(...) and so on)
> 
> Done. I was conservative and only added those needed to MouseEvent/TouchEvent.

> 
> I was just re-reviewing the Pointer Events specification and one thing I
noticed
> is that it says all devices that don't support pressure/force should provide
0.5
> if any button is down. My feeling is that this is a detail that should be
> handled in the code that maps the event to a blink event (i.e. in
> content/browser/renderer_host) rather than in this CL. A very basic heuristic
of
> "if (button_down && force == 0.0f) force = 0.5f" should suffice. What do you
> think?

Details like this should definitely be left to blink, and you should be free to
use whatever API is most convenient and clear within chromium.  But we need to
make sure there's sufficient information.  Eg. can a real pressure-sensitive
device ever report 0 force?  If so, then it may be better to use a constant like
-1 to represent unknown.  Or maybe the best option is to coerce any reports of 0
pressure from the OS to 0.001 so that we can safely reserve 0 for 'unknown'.

[robert.bradford, 2015-08-17 16:52:05]

On 2015/08/17 16:33:06, Rick Byers wrote:
> On 2015/08/17 16:23:33, robert.bradford wrote:
> > On 2015/08/17 14:44:43, sadrul wrote:
> > > A small change. Otherwise, looks good!
> > > 
> > > https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h
> > > File ui/events/event.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1260453006/diff/60001/ui/events/event.h#newco...
> > > ui/events/event.h:614: PointerDetails* pointer_details() { return
> > > &pointer_details_; }
> > > Can this return a const& instead?
> > > 
> > > The callers should go through the MouseEvent/TouchEvent to mutate it (e.g.
> > > MouseEvent::set_pointer_type(...) and so on)
> > 
> > Done. I was conservative and only added those needed to
MouseEvent/TouchEvent.
> 
> > 
> > I was just re-reviewing the Pointer Events specification and one thing I
> noticed
> > is that it says all devices that don't support pressure/force should provide
> 0.5
> > if any button is down. My feeling is that this is a detail that should be
> > handled in the code that maps the event to a blink event (i.e. in
> > content/browser/renderer_host) rather than in this CL. A very basic
heuristic
> of
> > "if (button_down && force == 0.0f) force = 0.5f" should suffice. What do you
> > think?
> 
> Details like this should definitely be left to blink, and you should be free
to
> use whatever API is most convenient and clear within chromium.  But we need to
> make sure there's sufficient information.  Eg. can a real pressure-sensitive
> device ever report 0 force?  If so, then it may be better to use a constant
like
> -1 to represent unknown.  Or maybe the best option is to coerce any reports of
0
> pressure from the OS to 0.001 so that we can safely reserve 0 for 'unknown'.

Fortunately we don't have to worry about lack of information in this case as the
0.5 emulation is only necessary if we're in an "active buttons state" and that
requires physical contact for both pen and touch (as the specification currently
stands). Any device that reports pressure information will have a non-zero value
(as it's got physical contact) and any device that does not will have the value
emulated to 0.5.

Or to put it another way a device that reports 0 pressure for physical contact
(vs hovering) should always have it's value emulated by blink to 0.5

[sadrul, 2015-08-18 17:12:53]

LGTM

[robert.bradford, 2015-08-19 14:02:06]

The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 14:02:34]

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

[commit-bot: I haz the power, 2015-08-19 15:53:48]

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

[commit-bot: I haz the power, 2015-08-19 15:53:48]

Dry run: This issue passed the CQ dry run.

[robert.bradford, 2015-08-19 16:32:25]

The CQ bit was checked by robert.bradford@intel.com

[commit-bot: I haz the power, 2015-08-19 16:32:54]

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

[commit-bot: I haz the power, 2015-08-19 16:37:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-19 16:38:45]

Patchset 5 (id:??) landed as
https://crrev.com/f62ea41d40ecf774dbb44233252388a55c3cf0ee
Cr-Commit-Position: refs/heads/master@{#344250}