Bug 86270

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

Created attachment 141522 [details] Patch

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?

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

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.

Created attachment 141525 [details] Units fixed

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.

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?

(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

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

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.

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.

> 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.

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.

(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.

> 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.

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.

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.

Comment on attachment 141525 [details] Units fixed Clearing flags on attachment: 141525 Committed r116996: <http://trac.webkit.org/changeset/116996>

All reviewed patches have been landed. Closing bug.