Bug 101563 - Add support for fractional scroll events
Summary: Add support for fractional scroll events
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-08 01:28 PST by Tai-Hsu Lin
Modified: 2012-11-13 07:57 PST (History)
10 users (show)

See Also:


Attachments
Patch (19.52 KB, patch)
2012-11-08 01:36 PST, Tai-Hsu Lin
no flags Details | Formatted Diff | Diff
Add support for fractional scroll events (19.66 KB, patch)
2012-11-08 23:10 PST, Tai-Hsu Lin
no flags Details | Formatted Diff | Diff
Add support for fractional scroll events (19.67 KB, patch)
2012-11-08 23:14 PST, Tai-Hsu Lin
no flags Details | Formatted Diff | Diff
Add support for fractional scroll events (20.60 KB, patch)
2012-11-11 23:33 PST, Tai-Hsu Lin
no flags Details | Formatted Diff | Diff
Add support for fractional scroll events (20.65 KB, patch)
2012-11-12 00:59 PST, Tai-Hsu Lin
no flags Details | Formatted Diff | Diff
Add support for fractional scroll events (20.70 KB, patch)
2012-11-12 01:06 PST, Tai-Hsu Lin
simon.fraser: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tai-Hsu Lin 2012-11-08 01:28:20 PST
Add support for fractional scroll events
Comment 1 Tai-Hsu Lin 2012-11-08 01:36:48 PST
Created attachment 172958 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-08 01:41:17 PST
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.
Comment 3 Peter Beverloo (cr-android ews) 2012-11-08 01:51:05 PST
Comment on attachment 172958 [details]
Patch

Attachment 172958 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14771080
Comment 4 WebKit Review Bot 2012-11-08 05:06:03 PST
Comment on attachment 172958 [details]
Patch

Attachment 172958 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14774104
Comment 5 Adam Barth 2012-11-08 10:22:01 PST
Looks like this patch doesn't build.  Do we need to stage this change in order to not break the build?

Also, it would be helpful to explain why we want to make this change.  Is there some particular use case that requires floating point at this time?
Comment 6 Tai-Hsu Lin 2012-11-08 19:49:32 PST
Sorry, let me take a look and fix it.

Please refer to chromium-os:35171. We want this for more precise scrolling due to less rounding. The scroll increments currently get unnecessary rounding in WebKit while the compositor in Chrome can actually accept floating points. The difference might not be huge but I think it is still good to fix.
Comment 7 Tai-Hsu Lin 2012-11-08 23:10:26 PST
Created attachment 173212 [details]
Add support for fractional scroll events
Comment 8 WebKit Review Bot 2012-11-08 23:13:08 PST
Attachment 173212 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebInputHandlerClient.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Tai-Hsu Lin 2012-11-08 23:14:48 PST
Created attachment 173214 [details]
Add support for fractional scroll events
Comment 10 WebKit Review Bot 2012-11-09 00:55:09 PST
Comment on attachment 173214 [details]
Add support for fractional scroll events

Attachment 173214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14770549
Comment 11 Peter Beverloo (cr-android ews) 2012-11-09 03:41:08 PST
Comment on attachment 173214 [details]
Add support for fractional scroll events

Attachment 173214 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14788104
Comment 12 Adam Barth 2012-11-09 11:16:55 PST
Comment on attachment 173214 [details]
Add support for fractional scroll events

Ok.  You're still having build trouble.  I suspect you'll need to stage this change with an ifdef.
Comment 13 Tai-Hsu Lin 2012-11-11 23:33:11 PST
Created attachment 173572 [details]
Add support for fractional scroll events
Comment 14 Tai-Hsu Lin 2012-11-12 00:59:54 PST
Created attachment 173583 [details]
Add support for fractional scroll events
Comment 15 Tai-Hsu Lin 2012-11-12 01:06:36 PST
Created attachment 173584 [details]
Add support for fractional scroll events
Comment 16 WebKit Review Bot 2012-11-12 01:11:31 PST
Attachment 173584 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebInputHandlerClient.h:27:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 WebKit Review Bot 2012-11-12 01:20:46 PST
Comment on attachment 173584 [details]
Add support for fractional scroll events

Attachment 173584 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14689546
Comment 18 Peter Beverloo (cr-android ews) 2012-11-12 03:46:05 PST
Comment on attachment 173584 [details]
Add support for fractional scroll events

Attachment 173584 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14804590
Comment 19 Simon Fraser (smfr) 2012-11-12 09:02:24 PST
Comment on attachment 173584 [details]
Add support for fractional scroll events

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

> Source/WebCore/platform/ScrollAnimatorNone.h:105
> +#if PLATFORM(CHROMIUM)
>      virtual void scrollBy(const IntPoint&);
> +#else
> +    virtual void scrollBy(const FloatPoint&);
> +#endif

Platform #ifdefs like this in shared files are not cool. You should change everyone to FloatPoint, or no-one.
Comment 20 Adam Barth 2012-11-12 09:49:38 PST
> Ok.  You're still having build trouble.  I suspect you'll need to stage this change with an ifdef.

To be clear: I meant an ifdef for the API, not for WebCore.
Comment 21 Tai-Hsu Lin 2012-11-12 22:34:59 PST
Simon,

The ifdefs are only temporary for the patch to build. I will remove ifdefs and change to floating point for everyone after I applied the corresponding patch on the chromium side.

Adam,

I am sorry but maybe I misunderstood your comments. Could you shed more light on me? What do you mean by ifdef for the API and not for WebCore? 

Thanks a lot!
Comment 22 Adam Barth 2012-11-12 23:20:01 PST
Comment on attachment 173584 [details]
Add support for fractional scroll events

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

>> Source/Platform/chromium/public/WebInputHandlerClient.h:27
>> +#include "config.h"
> 
> Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]

You cannot include config.h in any API headers (any headers that contain "public" in their path).

> Source/Platform/chromium/public/WebInputHandlerClient.h:63
> +#if PLATFORM(CHROMIUM)
>      virtual void scrollBy(WebPoint, WebSize) = 0;
> +#else
> +    virtual void scrollBy(WebPoint, WebFloatPoint) = 0;
> +#endif

This file is used only by Chromium.  Having a PLATFORM(CHROMIUM) ifdef here does not make any sense.  Instead, you'll probably need to create a new ifdef to represent this change so you can land it without breaking the Chromium compile.  After this patch is rolled in the Chromium, you can update all the code in svn.chromium.org to use the new API (and to define the ifdef).  Then you can delete the ifdef and the old API from svn.webkit.org.
Comment 23 Adam Barth 2012-11-12 23:21:05 PST
Your patch looks pretty confused.  Is there someone local who can help you understand what you need to do to make this change?  It's going to be pretty inefficient for me to help you make this change via the code review process.
Comment 24 Tai-Hsu Lin 2012-11-12 23:46:56 PST
Adam,

Thanks for your time! I am getting some ideas now.

Unfortunately there is no one in my local office that are familiar with the WebKit infrastructure. We just happened to need some change here so we could send fractional scroll events in Chrome OS.
Comment 25 Adam Barth 2012-11-13 07:57:39 PST
> Unfortunately there is no one in my local office that are familiar with the WebKit infrastructure.

It looks like you're almost in the same time zone as tkent.  He might be a good person to talk with since at least you won't need to wait for the time zone delay for each response.