Bug 148361 – Factor out and add logging to swipe-start hysteresis code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Factor out and add logging to swipe-start hysteresis code

[Tim Horton]

Created attachment 259725 [details]
Patch

[Darin Adler]

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?

[Tim Horton]

http://trac.webkit.org/changeset/188833

[Tim Horton]

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