Summary: | Add support for fractional scroll events | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tai-Hsu Lin <sheckylin> | ||||||||||||||
Component: | UI Events | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | UNCONFIRMED --- | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dino, fishd, jamesr, peter+ews, simon.fraser, tkent, tkent+wkapi, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=234360 | ||||||||||||||||
Attachments: |
|
Description
Tai-Hsu Lin
2012-11-08 01:28:20 PST
Created attachment 172958 [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. 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 on attachment 172958 [details] Patch Attachment 172958 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14774104 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? 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. Created attachment 173212 [details]
Add support for fractional scroll events
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.
Created attachment 173214 [details]
Add support for fractional scroll events
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 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 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.
Created attachment 173572 [details]
Add support for fractional scroll events
Created attachment 173583 [details]
Add support for fractional scroll events
Created attachment 173584 [details]
Add support for fractional scroll events
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 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 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 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. > 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.
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 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. 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. 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. > 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.
|