Bug 101563 – Add support for fractional scroll events

• Component: UI Events
• Status: UNCONFIRMED

[Tai-Hsu Lin]

Add support for fractional scroll events

[Tai-Hsu Lin]

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 172958 [details]
Patch

Attachment 172958 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14771080

[WebKit Review Bot]

Comment on attachment 172958 [details]
Patch

Attachment 172958 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14774104

[Adam Barth]

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?

[Tai-Hsu Lin]

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.

[Tai-Hsu Lin]

Created attachment 173212 [details]
Add support for fractional scroll events

[WebKit Review Bot]

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.

[Tai-Hsu Lin]

Created attachment 173214 [details]
Add support for fractional scroll events

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Adam Barth]

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.

[Tai-Hsu Lin]

Created attachment 173572 [details]
Add support for fractional scroll events

[Tai-Hsu Lin]

Created attachment 173583 [details]
Add support for fractional scroll events

[Tai-Hsu Lin]

Created attachment 173584 [details]
Add support for fractional scroll events

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Simon Fraser (smfr)]

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.

[Adam Barth]

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

[Tai-Hsu Lin]

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!

[Adam Barth]

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.

[Adam Barth]

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.

[Tai-Hsu Lin]

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.

[Adam Barth]

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