Factor out and add logging to swipe-start hysteresis code
Created attachment 259725 [details] Patch
Comment on attachment 259725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259725&action=review > Source/WebKit2/UIProcess/mac/ViewGestureController.h:106 > void setDidMoveSwipeSnapshotCallback(void(^)(CGRect)); Seems strange that we use a block here instead of a std::function. > Source/WebKit2/UIProcess/mac/ViewGestureController.h:207 > + PendingSwipeTracker(WebPageProxy& webPageProxy, std::function<void(NSEvent *, SwipeDirection)> trackSwipeCallback) > + : m_trackSwipeCallback(WTF::move(trackSwipeCallback)) > + , m_webPageProxy(webPageProxy) > + { > + } Not sure itβs better to have this defined in the header. It can still be inlined even if you put it in the .cpp file. > Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm:277 > -bool ViewGestureController::scrollEventCanBecomeSwipe(NSEvent *event, ViewGestureController::SwipeDirection& potentialSwipeDirection) > +static bool scrollEventCanInfluenceSwipe(NSEvent *event) > { > - if (event.phase != NSEventPhaseBegan) > - return false; > - > if (!event.hasPreciseScrollingDeltas) > return false; > > if (![NSEvent isSwipeTrackingFromScrollEventsEnabled]) > return false; > > - if (fabs(event.scrollingDeltaX) <= fabs(event.scrollingDeltaY)) > + return true; > +} Now that you have simplified this, I think it reads better as a one liner: return event.hasPreciseScrollingDeltas && ![NSEvent isSwipeTrackingFromScrollEventsEnabled]; > Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm:281 > + return fabs(y) >= fabs(x) * minimumScrollEventRatioForSwipe; In new code I think we should use std::abs instead of fabs, fabsf, abs, etc. > Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm:370 > + if (fabs(m_cumulativeDelta.width()) >= minimumHorizontalSwipeDistance) Same comment about std::abs. > Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm:740 > + if (m_didMoveSwipeSnapshotCallback) > + Block_release(m_didMoveSwipeSnapshotCallback); > + m_didMoveSwipeSnapshotCallback = Block_copy(callback); Is there a way to do this with a smart pointer instead?
http://trac.webkit.org/changeset/188833
(In reply to comment #2) > Comment on attachment 259725 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259725&action=review > > > Source/WebKit2/UIProcess/mac/ViewGestureController.h:106 > > void setDidMoveSwipeSnapshotCallback(void(^)(CGRect)); > > Seems strange that we use a block here instead of a std::function. > > > Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm:740 > > + if (m_didMoveSwipeSnapshotCallback) > > + Block_release(m_didMoveSwipeSnapshotCallback); > > + m_didMoveSwipeSnapshotCallback = Block_copy(callback); > > Is there a way to do this with a smart pointer instead? Agreed, the didMoveSwipeSnapshot callback needs a revisit.