Bug 87535 - [Chromium] Re-enable handling of smooth scrolling on Chromium Linux/Windows
: [Chromium] Re-enable handling of smooth scrolling on Chromium Linux/Windows
Status: RESOLVED FIXED
: WebKit
Event Handling
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-25 14:26 PST by
Modified: 2012-06-06 22:56 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-25 14:26:40 PST
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 From 2012-05-29 07:50:05 PST -------
Created an attachment (id=144557) [details]
Patch
------- Comment #2 From 2012-05-29 07:52:32 PST -------
jamesr@: could you perhaps review?
------- Comment #3 From 2012-05-29 17:21:12 PST -------
(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?

> 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 From 2012-05-30 07:28:56 PST -------
(In reply to comment #3)
> (From update of attachment 144557 [details] [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 From 2012-05-30 08:19:05 PST -------
Created an attachment (id=144823) [details]
Patch
------- Comment #6 From 2012-05-30 08:21:41 PST -------
Note in passing: the gtk build fail would not seem to be caused by this change.
------- Comment #7 From 2012-06-04 17:34:06 PST -------
(From update of attachment 144823 [details])
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 From 2012-06-06 15:43:30 PST -------
(From update of attachment 144823 [details])
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 From 2012-06-06 15:54:25 PST -------
Created an attachment (id=146135) [details]
Patch
------- Comment #10 From 2012-06-06 22:56:30 PST -------
(From update of attachment 146135 [details])
Clearing flags on attachment: 146135

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