Meaning of deltaX/deltaY in PlatformGestureEvent is overloaded

• Component: UI Events
• Status: RESOLVED INVALID

[Rick Byers]

WebCore::PlatformGestureEvent has deltaX and deltaY fields that are used for a variety of different things for different gesture types:
  - For scroll update it's the scroll amount
  - For pinch it's a scale factor
  - For fling it's a velocity
  - For tap it's the tap count
This is confusing and error prone.

We should split these uses out into new fields, and where we care about wasting too much space new sub-classes of PlatformGestureEvent (or potentially a union).

I'm not sure what the best trade-off between simplicity and space is.  For example, PlatformMouseEvent keeps a tapCount field, even though it's only relevent for mousedown/mouseup/click events.  So to be consistent we should probably just add a tapCount to PlatformGestureEvent.

For the others (which really are x and y values), perhaps we should use "extraX" and "extraY" fields and just have properly-named accessors that validate they are only called when the event is of the required type?

[Adam Barth]

I wouldn't worry too much about space.  There are only a handful of these objects at a time.  It's not like you're adding bytes to Node, of which there are thousands in each page.

[Rick Byers]

There was some concern over the performance, especially given that we're trying hard to drive down latency of some gesture events (scroll updates in particular).  I just did some measurements, and I agree there's no point worrying about a few extra fields.

In particular (for those that don't want to take my word for it), here is what I did:
 - Added an array of N floats to chromium's WebGestureEvent, which all get initialized to 0
 - Added an identical array to PlatformGestureEvent, and copied each element one-by-one (in a for loop) from WebGestureEvent
 - Measured latency of about 100 tap events on ChromeOS with different values of N by plumbing high-resolution event timestamps through to javascript using this patch: https://codereview.appspot.com/6452060 and comparing event timestamps to window.performance.webkitNow().  
- I did this without any touch event handlers on the page so that we wouldn't have the extra overhead of touch event round trip (which could add ~10ms in my tests).

With no extra padding (N=0), average latency on my hardware was about 3.8ms (+/- 0.8ms)
With N=1024 or 10240, any difference in latency was indistinguishable from noise
For N=10k, the average latency went up to about 6ms.

So as long as we keep the number of additional fields under 1k, I don't think we should worry about performance at all.

[Rick Byers]

Created attachment 161543 [details]
Patch

[Build Bot]

Comment on attachment 161543 [details]
Patch

Attachment 161543 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13684827

[Early Warning System Bot]

Comment on attachment 161543 [details]
Patch

Attachment 161543 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13683916

[Robert Kroeger]

Have you considered using subclasses for the PlatformGestureEvent? While unions would seem to be the right choice for WebGestureEvent, it is not clear to me that PGEs could not benefit from division.

You could conceivably separate the WCIHi code from the PGE change if you want a smaller CL?

[Rick Byers]

Created attachment 161690 [details]
Patch

[Rick Byers]

(In reply to comment #6)
> Have you considered using subclasses for the PlatformGestureEvent? While unions would seem to be the right choice for WebGestureEvent, it is not clear to me that PGEs could not benefit from division.

Yeah, it's certainly an option.  But at first glance it doesn't seem worth the extra complexity to me (more types, more Builder types, etc.).  I think you could make the argument that almost every Platform*Event type could be split into a lot more sub classes (eg. PlatformMouseEvent has some fields that are only used by some event types).  What's the right trade-off?

jamesr@ what's your opinion?
 
> You could conceivably separate the WCIHi code from the PGE change if you want a smaller CL?

Sure.  I thought this was simple enough that it wasn't worth adding another serializing step (it's already 6 CLs back and forth between chromium and WebKit to do the full clean-up).  But if you think this is too big, I'm happy to split this piece into two.

[Rick Byers]

Ok, I've split the WebGestureEvents changes off into bug 95573.  I'll upload a new patch here based against it.

[Rick Byers]

Created attachment 163506 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 163506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163506&action=review

> Source/WebCore/platform/PlatformGestureEvent.h:47
> +        memset(&m_data, 0, sizeof(m_data));

Why do we need to memset here? The rationale of this change should be explained in the change log.

[Rick Byers]

Thanks for your comments!

(In reply to comment #11)
> (From update of attachment 163506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163506&action=review
> 
> > Source/WebCore/platform/PlatformGestureEvent.h:47
> > +        memset(&m_data, 0, sizeof(m_data));
> 
> Why do we need to memset here? The rationale of this change should be explained in the change log.

jamesr@ argued that we should use a union here to avoid wasting space for event-type specific data (like the tap-count I added for GestureTapDown).  If we're going to use a union, then memset to zero all the union fields on initialization seems like a good idea to me, but we could explicitly initialize all the union members or some subset of them (but which subset?).

Once we have consensus on the approach, I'll update the Changelog with more rationale.

[Ryosuke Niwa]

I don't think we should memset m_data unless they're used, in which case, we should be using the initialization list anyways. Calling memset is quite expensive.

[James Robinson]

I think memset() is the right thing to do for a union type.  I'm not sure what you mean about "quite expensive" - do you expect it to be more expensive than zeroing out the fields by hand?

Saying "use the initializer list" doesn't make a ton of sense for unions.

[Ryosuke Niwa]

(In reply to comment #14)
> I think memset() is the right thing to do for a union type.

If we're going to use values in the union, then sure. But if we're not going to access/use the union, then there's no need to initialize it.

> I'm not sure what you mean about "quite expensive" - do you expect it to be more expensive than zeroing out the fields by hand?

Yes. memset results in a function call in many platforms.

> Saying "use the initializer list" doesn't make a ton of sense for unions.

Why not? Don't we know the type, i.e. which value in the union to use, at the time of initialization?

[Hugo Parente Lima]

PlatformGestureEvent was removed from WebKit on bug 122650.