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.
Created attachment 257602 [details] Patch
This is a work in progress. Still have to figure out why I have to invert the wheel event delta at EventHandler.cpp:2791.
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.
Created attachment 257603 [details] Patch
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.
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...
Created attachment 257608 [details] Patch
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.
Created attachment 257620 [details] Patch
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.
Created attachment 259483 [details] Patch
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.
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.
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.
Created attachment 259516 [details] Patch
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.
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.
Comment on attachment 259516 [details] Patch Forgot to update WheelEventDeltaFilter::create. Putting up another patch soon.
Created attachment 259523 [details] Patch
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.
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.
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
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
Created attachment 259728 [details] Patch
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.
Created attachment 259732 [details] Patch
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.
Created attachment 259735 [details] Patch
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.
Committed r188860: <http://trac.webkit.org/changeset/188860>
(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>
(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