Bug 87535 – [Chromium] Re-enable handling of smooth scrolling on Chromium Linux/Windows

• Component: UI Events
• Status: RESOLVED FIXED

[Robert Kroeger]

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.

[Robert Kroeger]

Created attachment 144557 [details]
Patch

[Robert Kroeger]

jamesr@: could you perhaps review?

[James Robinson]

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() ?

[Robert Kroeger]

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

[Robert Kroeger]

Created attachment 144823 [details]
Patch

[Robert Kroeger]

Note in passing: the gtk build fail would not seem to be caused by this change.

[James Robinson]

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

[Robert Kroeger]

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.

[Robert Kroeger]

Created attachment 146135 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146135 [details]
Patch

Clearing flags on attachment: 146135

Committed r119684: <http://trac.webkit.org/changeset/119684>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.