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.
Created attachment 158882 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Thanks.
What about using a union instead of growing the type's size even further, if these fields don't overlap?
rjkroege, should we respond to jamesr's question before landing the patch?
(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.
abarth@ jamesr@: Sorry for being too eager. Fixed.
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.
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.
(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.
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.
(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.
Created attachment 159294 [details] Patch
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?
Comment on attachment 159294 [details] Patch Clearing flags on attachment: 159294 Committed r125981: <http://trac.webkit.org/changeset/125981>
All reviewed patches have been landed. Closing bug.
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.
OK great! Feel free to CC me on the chromium patches if you need any reviews on that side.
Mass move bugs into the DOM component.