[webkit-reviews] review granted: [Bug 135476] Implement long mouse press over links. Part of 135257 - Add long mouse press gesture. : [Attachment 236247] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 8 13:57:27 PDT 2014


Tim Horton <thorton at apple.com> has granted Peyton Randolph
<prandolph at apple.com>'s request for review:
Bug 135476: Implement long mouse press over links. Part of 135257 - Add long
mouse press gesture.
https://bugs.webkit.org/show_bug.cgi?id=135476

Attachment 236247: Patch
https://bugs.webkit.org/attachment.cgi?id=236247&action=review

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236247&action=review


> Source/WebCore/page/EventHandler.cpp:166
> +#if ENABLE(LONG_MOUSE_PRESS)

Simon noted that you might want to get rid of these ENABLEs, since there's
nothing particularly scary or platform-specific about this feature, and it
has/will have a runtime flag, and won't do anything if the client doesn't care,
anyway. That said, I think that can happen later, perhaps in
https://bugs.webkit.org/show_bug.cgi?id=135721.

> Source/WebCore/page/EventHandler.cpp:168
> +const int longMousePressPointsHysteresis = 5;

Maybe "maximumDistanceForLongMousePress", or something (that's not a great name
either, but a little better). "pointsHysteresis" reads oddly to me.

> Source/WebCore/page/EventHandler.cpp:780
> +	   beginLongMousePress();

Is this "beginTrackingPotentialLongMousePress"? because I don't think the
*long* mouse press is technically beginning yet. But I dunno!

> Source/WebCore/page/EventHandler.cpp:1585
> +    // FIXME: bubble long mouse press up to the client.

Capital 'B', perhaps. Also, do you have a bug number for these?

> Source/WebCore/page/EventHandler.cpp:1619
> +    if (mouseEvent.button() != LeftButton || mouseEvent.type() !=
PlatformEvent::MouseMoved)

Why would long press be restricted to the left mouse button? Should we not
support long-press of any arbitrary button and let the client decide which ones
it cares about? I could see that being useful.

> Source/WebCore/page/EventHandler.cpp:1630
> +    if (!mouseHysteresisExceeded(mouseEvent.position(),
longMousePressPointsHysteresis))
> +	   return false;
> +
> +    cancelLongMousePress();
> +
> +    return false;

I think this would read better as just:

if (mouseHysteresisExceeded(mouseEvent.position(),
longMousePressPointsHysteresis))
    cancelLongMousePress();

return false;

> Source/WebCore/page/EventHandler.cpp:3527
> +bool EventHandler::mouseHysteresisExceeded(const FloatPoint&
viewportLocation, const int pointsThreshold) const

This is still technically about drag, in some sense. Maybe
"dragDistanceExceedsThreshold"? That sounds good in an if statement ("if
dragDistanceExceedsThreshold").

> Source/WebCore/page/EventHandler.cpp:3535
> +    return abs(delta.width()) >= pointsThreshold || abs(delta.height()) >=
pointsThreshold;

Should this be something like delta.diagonalLengthSquared() >= pointsThreshold
* pointsThreshold, to make a circle of hysteresis?

I see you got this from existing code, so maybe don't change it, but I wonder
why!


More information about the webkit-reviews mailing list