Bug 94238 – [chromium] Add additional fields to WebGestureEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rick Byers]

To fix bug 93123, we're going to add a few gesture-type-specific fields to WebGestureEvent and PlatformGestureEvent.
The first step is to add these new fields to WebGestureEvent so that chromium can start setting them (redundantly with deltaX/deltaY temporarily).  Only once chromium is writing to the new fields, can I submit a CL to stop overloading deltaX/deltaY.

[Rick Byers]

Created attachment 158882 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Thanks.

[James Robinson]

What about using a union instead of growing the type's size even further, if these fields don't overlap?

[Adam Barth]

rjkroege, should we respond to jamesr's question before landing the patch?

[Rick Byers]

(In reply to comment #4)
> What about using a union instead of growing the type's size even further, if these fields don't overlap?

See bug 93123 - abarth@ said to prefer adding more fields over a union.  I did a bunch of measurements to confirm that the performance impact (even with 1024 new fields) is negligible.

[Robert Kroeger]

abarth@ jamesr@: Sorry for being too eager. Fixed.

[Rick Byers]

Also note that we don't use a union anywhere else in WebInputEvent.h.  For example, WebMouseEvent has a clickCount field that's only used for some event types (MouseDown, MouseUp, Click).

I also need to update PlatformGestureEvent.  I don't see a lot of unions in WebCore code, I wasn't sure if they were frowned upon.  The cleaner option is to have a bunch of sub-classes, but that adds a lot of complexity for almost no practical benefit.

[James Robinson]

Subclasses are the same as unions practically speaking, just a different way to spell it.  It's not a huge difference - but it does add up.  Piling on extra fields is OK, but IMO it's sloppy coding.

[Rick Byers]

(In reply to comment #9)
> Subclasses are the same as unions practically speaking, just a different way to spell it.  

I agree (although in practice the difference in the amount of code, and likelihood of accidentally violating type safety can be quite different).

> It's not a huge difference - but it does add up.  Piling on extra fields is OK, but IMO it's sloppy coding.

It's up to you and abarth - I'm happy to take whichever approach you guys agree is better.  Unions (or subclassing) was our first plan.  After doing the perf measurements, I can't help but find myself thinking the shorter/simpler code is a better trade-off.  IMHO a simpler but more wasteful approach isn't always sloppiness if an explicit cost/benefit trade-off is being made.

Anyway, like I said - let me know which approach you guys believe is better.

[Adam Barth]

I don't think it's a big deal one way or another.  I'm happy to defer to James here if he feels strongly.  I suspect it's not an issue we should get hung up on one way or another.

[Rick Byers]

(In reply to comment #11)
> I don't think it's a big deal one way or another.  I'm happy to defer to James here if he feels strongly.  I suspect it's not an issue we should get hung up on one way or another.

Agreed.  It may be more contentious for PlatformGestureEvent, but there's no reason we need to choose the same solution there.  It's trivial to use a union here in WebGestureEvent.  Patch forthcoming.

[Rick Byers]

Created attachment 159294 [details]
Patch

[James Robinson]

Comment on attachment 159294 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEvent.cpp:63
> +    int gestureData[14];

this size growth is temporary, right?

[WebKit Review Bot]

Comment on attachment 159294 [details]
Patch

Clearing flags on attachment: 159294

Committed r125981: <http://trac.webkit.org/changeset/125981>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rick Byers]

Thanks James.

(In reply to comment #14)
> (From update of attachment 159294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159294&action=review
> 
> > Source/WebKit/chromium/src/WebInputEvent.cpp:63
> > +    int gestureData[14];
> 
> this size growth is temporary, right?

Yes, by simplifying the bounding box stuff (x/y was unused) I'll get this down to 8 once I update chromium.

[James Robinson]

OK great! Feel free to CC me on the chromium patches if you need any reviews on that side.

[Lucas Forschler]

Mass move bugs into the DOM component.