Bug 86270 – [Chromium] android/WebInputEventFactory should handle wheel events and gesture events

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[Chromium] android/WebInputEventFactory should handle wheel events and gesture events

[Adam Barth]

Created attachment 141520 [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]

Created attachment 141522 [details]
Patch

[James Robinson]

Comment on attachment 141522 [details]
Patch

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

> Source/WebKit/chromium/public/android/WebInputEventFactory.h:59
> +                                                        long timeStampSeconds,

a timestamp in seconds of an integral type seems completely useless. is this milliseconds (like DOMTimeStamp)? is the type here wrong or the name?

[Adam Barth]

Comment on attachment 141522 [details]
Patch

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

> Source/WebKit/chromium/public/android/WebInputEventFactory.h:37
> +#define WEBKIT_UPDATED_WEB_INPUT_EVENT_FACTORY_API 1

I'll use this #define to make the API change land smoothly in Chromium.  The only file that appears to care currently is browser/renderer_host/native_web_keyboard_event_android.cc

[Adam Barth]

Comment on attachment 141522 [details]
Patch

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

>> Source/WebKit/chromium/public/android/WebInputEventFactory.h:59
>> +                                                        long timeStampSeconds,
> 
> a timestamp in seconds of an integral type seems completely useless. is this milliseconds (like DOMTimeStamp)? is the type here wrong or the name?

You're right.  I've goofed this up.  One moment.

[Adam Barth]

Created attachment 141525 [details]
Units fixed

[Adam Barth]

Turns out I don't need WEBKIT_UPDATED_WEB_INPUT_EVENT_FACTORY_API because the API is correct here.  The downstream one is the one that's backwards.

[James Robinson]

Comment on attachment 141525 [details]
Units fixed

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

> Source/WebKit/chromium/public/android/WebInputEventFactory.h:49
> +    enum MouseWheelDirectionType {

do you know the history / story with this? what's a sideways mousewheel, and what's it have to do with android?

[Adam Barth]

(In reply to comment #9)
> (From update of attachment 141525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141525&action=review
> 
> > Source/WebKit/chromium/public/android/WebInputEventFactory.h:49
> > +    enum MouseWheelDirectionType {
> 
> do you know the history / story with this? what's a sideways mousewheel, and what's it have to do with android?

I'm not sure what the relation is with Android, but sideways mousewheel events appear to exist.  See, for example: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/gtk/WebInputEventFactory.cpp#L535

[Adam Barth]

Comment on attachment 141525 [details]
Units fixed

Green times on the Android build try server: http://build.chromium.org/p/tryserver.chromium/builders/android/builds/12566

[Peter Beverloo]

This change looks good to me from cr-Android's perspective, adding aelias@ who has done recent work in this area as a FYI as we'll want to sync this to downstream. I can also confirm that there are no build errors when using a WebKit compile.

[Alexandre Elias]

In the Android branch we are using millisecond timestamps for the arguments here, since that's  the Android convention.  I realize the WebKit standard is seconds, but the role of these converter functions is to convert from platform events to WebKit events, so it seemed like a natural place to do the / 1000.  Otherwise all the callers need to remember to do it themselves.

[Adam Barth]

> In the Android branch we are using millisecond timestamps for the arguments here, since that's  the Android convention.  I realize the WebKit standard is seconds, but the role of these converter functions is to convert from platform events to WebKit events, so it seemed like a natural place to do the / 1000.  Otherwise all the callers need to remember to do it themselves.

Yeah, I decided not to worry about that problem for this patch.  The existing functions in this file already use seconds.  If/when we convert to milliseconds, we should do them all at once so there isn't an inconsistency.

[Alexandre Elias]

The problem is that this patch will break Chrome for Android when it's auto-merged into the Android branch because all the callers will assume milliseconds.  This same millisecond/second issue has caused problems in the past, there won't even be compile errors and it's hard to track down.

In the Android branch, all of the functions have been changed to milliseconds.  The easiest solution would be just to bring these two files fully in sync with the Android branch version in one WebKit patch.

[Peter Beverloo]

(In reply to comment #15)
> The problem is that this patch will break Chrome for Android when it's auto-merged into the Android branch because all the callers will assume milliseconds.  This same millisecond/second issue has caused problems in the past, there won't even be compile errors and it's hard to track down.
> 
> In the Android branch, all of the functions have been changed to milliseconds.  The easiest solution would be just to bring these two files fully in sync with the Android branch version in one WebKit patch.

That's not an argument against doing it the right way of course :). This is a clear example of a patch which should be cherry picked ahead of being included in a merge, which we can definitely do after this lands.

[Adam Barth]

> The problem is that this patch will break Chrome for Android when it's auto-merged into the Android branch because all the callers will assume milliseconds.  This same millisecond/second issue has caused problems in the past, there won't even be compile errors and it's hard to track down.

Correct.  I plan to merge the patch manually after it lands in order to avoid that problem.

> In the Android branch, all of the functions have been changed to milliseconds.  The easiest solution would be just to bring these two files fully in sync with the Android branch version in one WebKit patch.

Let's do that in a followup patch.  It's unclear to me whether seconds or milliseconds are best.  We don't need to resolve this issue now, so we should delay worrying about it until we need to resolve it.

[Alexandre Elias]

OK, we can change the Android branch version to use the seconds version (and may as well change back the keyboardEvent function and reorder the arguments again to cancel out what I did in https://gerrit-int.chromium.org/#change,15496).  Please make sure to also create an Android-branch Chrome-side patch that divides by 1000.0 at all the call sites.

[Adam Barth]

In the future, the sorts of changes in https://gerrit-int.chromium.org/#change,15496 upstream first and then merge the changes back downstream.  These sorts of changes need review from WebKit reviewers.  Making the changes downstream bypasses that feedback, which means we'll likely need to re-do them again in the future.

[WebKit Review Bot]

Comment on attachment 141525 [details]
Units fixed

Clearing flags on attachment: 141525

Committed r116996: <http://trac.webkit.org/changeset/116996>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.