Bug 87535 - [Chromium] Re-enable handling of smooth scrolling on Chromium Linux/Windows
Summary: [Chromium] Re-enable handling of smooth scrolling on Chromium Linux/Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Kroeger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-25 14:26 PDT by Robert Kroeger
Modified: 2012-06-06 22:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.66 KB, patch)
2012-05-29 07:50 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (12.05 KB, patch)
2012-05-30 08:19 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (11.59 KB, patch)
2012-06-06 15:54 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Kroeger 2012-05-25 14:26:40 PDT
Very late in the implementation of ChromiumOS trackpad scrolling, we discovered that to also support smooth wheel scrolling Chromium Linux/Windows requires differentiating hasPreciseScrollingDeltas PlatformWheelEvents from !hasPreciseScrollingDeltas wheel events. Consequently, enable the hasPreciseScrollingDelta feature of PlatformWheelEvents for Chromium Linux/Windows.
Comment 1 Robert Kroeger 2012-05-29 07:50:05 PDT
Created attachment 144557 [details]
Patch
Comment 2 Robert Kroeger 2012-05-29 07:52:32 PDT
jamesr@: could you perhaps review?
Comment 3 James Robinson 2012-05-29 17:21:12 PDT
Comment on attachment 144557 [details]
Patch

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

> Source/WebCore/platform/PlatformWheelEvent.h:71
> +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)

Do these phases make sense for CHROMIUM non-OS X platforms?

> Source/WebCore/platform/PlatformWheelEvent.h:93
> +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)

I understand that hasPreciseScrollingDeltas makes sense for non-OS X chromium platforms, but do the other bits make any sense?

> Source/WebCore/platform/ScrollAnimator.cpp:95
> +    ScrollGranularity granularity = e.hasPreciseScrollingDeltas() ? ScrollByPrecisePixel : ScrollByPixel;

how is hasPreciseScrollingDeltas supposed to interact with PlatformWheelEvent::granularity() ?
Comment 4 Robert Kroeger 2012-05-30 07:28:56 PDT
(In reply to comment #3)
> (From update of attachment 144557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144557&action=review
> 
> > Source/WebCore/platform/PlatformWheelEvent.h:71
> > +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)
> 
> Do these phases make sense for CHROMIUM non-OS X platforms?

Not at present. Something similar would probably be necessary when we go about writing support for inertial scroll. 

I couldn't decide here which would be better: more convoluted #ifs or just including the unused fields. I thought that doing it this way made the patch simpler. I'll do it the other way in p2.

> 
> > Source/WebCore/platform/PlatformWheelEvent.h:93
> > +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)
> 
> I understand that hasPreciseScrollingDeltas makes sense for non-OS X chromium platforms, but do the other bits make any sense?
> 
> > Source/WebCore/platform/ScrollAnimator.cpp:95
> > +    ScrollGranularity granularity = e.hasPreciseScrollingDeltas() ? ScrollByPrecisePixel : ScrollByPixel;
> 
> how is hasPreciseScrollingDeltas supposed to interact with PlatformWheelEvent::granularity() ?

hasPreciseScrollingDeltas is ignored when granularity() != ScrollByPixelWheelEvent. If granularity == ScrollByPixelWheelEvent and hasPreciseScrollingDeltas is false, ScrollAnimatorNone should animate the scroll. Otherwise, ScrollAnimatorNone should simply scroll by the specified amount.
Comment 5 Robert Kroeger 2012-05-30 08:19:05 PDT
Created attachment 144823 [details]
Patch
Comment 6 Robert Kroeger 2012-05-30 08:21:41 PDT
Note in passing: the gtk build fail would not seem to be caused by this change.
Comment 7 James Robinson 2012-06-04 17:34:06 PDT
Comment on attachment 144823 [details]
Patch

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

R=me.  The ifdefs are pretty unfortunate, but I think they reflect fairly accurately the complexities of the different bits :(

> Source/WebCore/platform/ScrollAnimatorNone.cpp:416
> +

spurious newline
Comment 8 Robert Kroeger 2012-06-06 15:43:30 PDT
Comment on attachment 144823 [details]
Patch

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

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:416
>> +
> 
> spurious newline

fixed in p3.
Comment 9 Robert Kroeger 2012-06-06 15:54:25 PDT
Created attachment 146135 [details]
Patch
Comment 10 WebKit Review Bot 2012-06-06 22:56:30 PDT
Comment on attachment 146135 [details]
Patch

Clearing flags on attachment: 146135

Committed r119684: <http://trac.webkit.org/changeset/119684>
Comment 11 WebKit Review Bot 2012-06-06 22:56:35 PDT
All reviewed patches have been landed.  Closing bug.