Bug 148361 - Factor out and add logging to swipe-start hysteresis code
Summary: Factor out and add logging to swipe-start hysteresis code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-22 14:48 PDT by Tim Horton
Modified: 2015-08-22 21:16 PDT (History)
2 users (show)

See Also:


Attachments
Patch (15.16 KB, patch)
2015-08-22 14:49 PDT, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-08-22 14:48:28 PDT
Factor out and add logging to swipe-start hysteresis code
Comment 1 Tim Horton 2015-08-22 14:49:05 PDT
Created attachment 259725 [details]
Patch
Comment 2 Darin Adler 2015-08-22 19:34:28 PDT
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?
Comment 3 Tim Horton 2015-08-22 21:15:16 PDT
http://trac.webkit.org/changeset/188833
Comment 4 Tim Horton 2015-08-22 21:16:38 PDT
(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.