Bug 147320 – Use _NSScrollingPredominantAxisFilter for wheel event filtering on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

AppKit now exposes _NSScrollingPredominantAxisFilter, so refactor Mac to use it for wheel event filtering instead of WheelEventDeltaTracker. Make sure this doesn't break wheel event filtering that other platforms by using the WheelEventDeltaTracker as a fallback.

[Wenson Hsieh]

Created attachment 257602 [details]
Patch

[Wenson Hsieh]

This is a work in progress. Still have to figure out why I have to invert the wheel event delta at EventHandler.cpp:2791.

[WebKit Commit Bot]

Attachment 257602 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 257603 [details]
Patch

[WebKit Commit Bot]

Attachment 257603 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 257603 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2791
> +    FloatSize filteredDelta(-wheelEvent->deltaX(), -wheelEvent->deltaY());

This is strange. The delta is passed into the filter points in the opposite direction as the delta received in this function, so this negative sign is just here for now as a hack to make unfiltered scrolling go in the right direction. I'll look into this...

> Source/WebCore/page/MainFrame.cpp:38
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

^ whoops, my bad. This changes things...

[Wenson Hsieh]

Created attachment 257608 [details]
Patch

[WebKit Commit Bot]

Attachment 257608 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 257620 [details]
Patch

[WebKit Commit Bot]

Attachment 257620 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Attachment 257620 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 259483 [details]
Patch

[WebKit Commit Bot]

Attachment 259483 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 259483 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use _NSScrollingPredominantAxisFilter for wheel event filtering on Mac.

I removed the period at the end of this line.

[Simon Fraser (smfr)]

Comment on attachment 259483 [details]
Patch

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

> Source/WebCore/page/WheelEventDeltaFilter.cpp:34
> +    : m_currentFilteredDelta(0, 0)

No need to initialize FloatSize.

> Source/WebCore/page/WheelEventDeltaFilter.cpp:44
> +    , m_deltaTracker(WheelEventDeltaTracker())

No need to explicitly call zero-argument constructor.

> Source/WebCore/page/WheelEventDeltaFilter.h:50
> +class WheelEventDeltaFilterPlatformInvariant final : public WheelEventDeltaFilter {

"PlatformInvariant" seems like naming overkill. Maybe BasicWheelEventDeltaFilter

> Source/WebCore/page/WheelEventDeltaFilter.h:60
> +    WheelEventDeltaTracker m_deltaTracker;

Why does WheelEventDeltaTracker exist independently from this class? Can it just be merged into this?

> Source/WebCore/page/mac/WheelEventDeltaFilterMac.mm:31
> +#import <AppKit/NSScrollingInputFilter_Private.h>

This will need some SPI header stuff.

> Source/WebCore/page/mac/WheelEventDeltaFilterMac.mm:83
> +bool WheelEventDeltaFilterMac::isFilteringDeltas() const
> +{
> +    return m_isFilteringDeltas;
> +}
> +
> +FloatSize WheelEventDeltaFilterMac::filteredDelta() const
> +{
> +    return m_currentFilteredDelta;
> +}
> +    
> +FloatSize WheelEventDeltaFilterMac::filteredVelocity() const
> +{
> +    return m_currentFilteredVelocity;
> +}

These could all be inline.

> Source/WebKit2/WebProcess/WebPage/EventDispatcher.cpp:66
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> +    , m_recentWheelEventDeltaFilter(std::make_unique<WheelEventDeltaFilterMac>())
> +#else
> +    , m_recentWheelEventDeltaFilter(std::make_unique<WheelEventDeltaFilterPlatformInvariant>())
> +#endif

Shame to see all this here. Maybe add static WheelEventDeltaFilter::create() which is implemented per-platform.

[Wenson Hsieh]

Created attachment 259516 [details]
Patch

[WebKit Commit Bot]

Attachment 259516 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 259483 [details]
Patch

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

Thanks for the review! I just put up a v2 that addresses all of the points.

>> Source/WebCore/page/WheelEventDeltaFilter.cpp:34
>> +    : m_currentFilteredDelta(0, 0)
> 
> No need to initialize FloatSize.

Removed.

>> Source/WebCore/page/WheelEventDeltaFilter.cpp:44
>> +    , m_deltaTracker(WheelEventDeltaTracker())
> 
> No need to explicitly call zero-argument constructor.

Removed.

>> Source/WebCore/page/WheelEventDeltaFilter.h:50
>> +class WheelEventDeltaFilterPlatformInvariant final : public WheelEventDeltaFilter {
> 
> "PlatformInvariant" seems like naming overkill. Maybe BasicWheelEventDeltaFilter

Renamed to BasicWheelEventDeltaFilter.

>> Source/WebCore/page/WheelEventDeltaFilter.h:60
>> +    WheelEventDeltaTracker m_deltaTracker;
> 
> Why does WheelEventDeltaTracker exist independently from this class? Can it just be merged into this?

Good point -- it's not being used outside of this context anywhere else in WebKit. Done!

>> Source/WebCore/page/mac/WheelEventDeltaFilterMac.mm:31
>> +#import <AppKit/NSScrollingInputFilter_Private.h>
> 
> This will need some SPI header stuff.

Added NSScrollingInputFilterSPI.h.

>> Source/WebCore/page/mac/WheelEventDeltaFilterMac.mm:83
>> +}
> 
> These could all be inline.

Fixed.

>> Source/WebKit2/WebProcess/WebPage/EventDispatcher.cpp:66
>> +#endif
> 
> Shame to see all this here. Maybe add static WheelEventDeltaFilter::create() which is implemented per-platform.

Sounds good. Fixed.

[Wenson Hsieh]

Comment on attachment 259516 [details]
Patch

Forgot to update WheelEventDeltaFilter::create. Putting up another patch soon.

[Wenson Hsieh]

Created attachment 259523 [details]
Patch

[WebKit Commit Bot]

Attachment 259523 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 259523 [details]
Patch

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

> Source/WebCore/page/WheelEventDeltaFilter.h:46
> +    WEBCORE_EXPORT virtual FloatSize filteredDelta() const = 0;

Why is this function virtual? Both classes just return m_currentFilteredDelta

> Source/WebCore/page/WheelEventDeltaFilter.h:52
> +const size_t basicWheelEventDeltaFilterWindowSize = 3;

Don't think this needs to be in the header.

> Source/WebCore/page/WheelEventDeltaFilter.h:68
> +    virtual bool isFilteringDeltas() const override
> +    {
> +        return m_isTrackingDeltas;

Why the naming disparity?

> Source/WebCore/page/WheelEventDeltaFilter.h:80
> +    bool m_isTrackingDeltas;

{ false }

> Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:58
> +    bool m_isFilteringDeltas { false };

Name differs from the basic filter. Why not move to the base class?

> Source/WebCore/page/mac/WheelEventDeltaFilterMac.mm:38
> +    , m_predominantAxisFilter([[_NSScrollingPredominantAxisFilter alloc] init])

This is a leak. You need an adoptNS here.

> Source/WebCore/platform/spi/mac/NSScrollingInputFilterSPI.h:39
> +- (BOOL)resetIfOutOfDate:(NSTimeInterval)timestamp;

Unused. Only declare things you actually use.

[Build Bot]

Comment on attachment 259523 [details]
Patch

Attachment 259523 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/83586

New failing tests:
fast/events/platform-wheelevent-in-scrolling-div.html
fast/events/continuous-platform-wheelevent-in-scrolling-div.html

[Build Bot]

Created attachment 259553 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Wenson Hsieh]

Created attachment 259728 [details]
Patch

[WebKit Commit Bot]

Attachment 259728 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 259732 [details]
Patch

[WebKit Commit Bot]

Attachment 259732 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 259735 [details]
Patch

[WebKit Commit Bot]

Attachment 259735 [details] did not pass style-queue:


ERROR: Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:34:  _NSScrollingPredominantAxisFilter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Committed r188860: <http://trac.webkit.org/changeset/188860>

[David Kilzer (:ddkilzer)]

(In reply to comment #31)
> Committed r188860: <http://trac.webkit.org/changeset/188860>

Follow-up build fix for Mac:
<https://trac.webkit.org/changeset/188864>

[Wenson Hsieh]

(In reply to comment #31)
> Committed r188860: <http://trac.webkit.org/changeset/188860>

Follow-up attempted iOS build fix in https://trac.webkit.org/changeset/188869