Bug 49824 – WebKit2: Support Windows 7 gestures

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

We should support Windows 7 gesture support that we support in old WebKit.

[Brian Weinstein]

Created attachment 88498 [details]
[PATCH] Fix

[Adam Roben (:aroben)]

Comment on attachment 88498 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=88498&action=review

Let's give this another once-over. Looks pretty good though!

> Source/WebKit2/UIProcess/WebPageProxy.cpp:676
> +bool WebPageProxy::initializeGestureFromPoint(const IntPoint& point)
> +{
> +    bool canBeginPanning;
> +    process()->sendSync(Messages::WebPage::InitializeGestureFromPoint(point), Messages::WebPage::InitializeGestureFromPoint::Reply(canBeginPanning), m_pageID);
> +    return canBeginPanning;
> +}

Will this hang forever if the web process is hung? That wouldn't be good!

> Source/WebKit2/UIProcess/win/WebView.cpp:508
> +    bool canSingleFingerPan = m_page->initializeGestureFromPoint(IntPoint(localPoint));

The variable name is so much more descriptive than the function name that it seems like a better function name is warranted.

Is there a distinction between single-finger panning and multi-finger panning? Other parts of your code just say "canBeginPanning".

Is the explicit IntPoint constructor really needed?

> Source/WebKit2/UIProcess/win/WebView.cpp:518
> +    return SetGestureConfigPtr()(m_window, 0, 1, &gc, sizeof(GESTURECONFIG));

sizeof(gc) would be slightly more future-proof.

> Source/WebKit2/UIProcess/win/WebView.cpp:526
> +    if (!GetGestureInfoPtr() || !CloseGestureInfoHandlePtr()) {
> +        handled = false;
> +        return 0;
> +    }

Should this have an assertion similar to the one in onGestureNotify?

> Source/WebKit2/UIProcess/win/WebView.cpp:532
> +    if (!GetGestureInfoPtr()(gestureHandle, reinterpret_cast<PGESTUREINFO>(&gi))) {

Is the reinterpret_cast really needed? gi is already a GESTUREINFO.

> Source/WebKit2/UIProcess/win/WebView.cpp:555
> +        int deltaX = currentX - m_lastPanX;
> +        int deltaY = currentY - m_lastPanY;
> +
> +        m_lastPanX = currentX;
> +        m_lastPanY = currentY;
> +
> +        m_page->scrollByPanGesture(IntSize(-deltaX, -deltaY));

You could reverse the deltaX/Y calculations instead of negating them in the only place you use them.

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:206
> +    InitializeGestureFromPoint(WebCore::IntPoint point) -> (bool canBeginPanning)
> +    ScrollByPanGesture(WebCore::IntSize size)
> +    GestureEnded()

I'd suggest these names:

GestureWillBegin(point)
GestureDidScroll(size)
GestureDidEnd()

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:376
> +        HitTestResult result(scollView->windowToContents(point));

Can you use assignment here instead of constructor syntax? Maybe that isn't allowed.

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:390
> +    if (m_gestureTargetNode) {

This can be an early return.

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:407
> +    m_gestureTargetNode->renderer()->enclosingLayer()->scrollByRecursively(size.width(), size.height());

Do we need to null-check enclosingLayer()?

> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:412
> +    m_gestureTargetNode = 0;

Please use nullptr instead of 0.

[Brian Weinstein]

Comment on attachment 88498 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=88498&action=review

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:676
>> +}
> 
> Will this hang forever if the web process is hung? That wouldn't be good!

No, the UI process remains responsive.

>> Source/WebKit2/UIProcess/win/WebView.cpp:508
>> +    bool canSingleFingerPan = m_page->initializeGestureFromPoint(IntPoint(localPoint));
> 
> The variable name is so much more descriptive than the function name that it seems like a better function name is warranted.
> 
> Is there a distinction between single-finger panning and multi-finger panning? Other parts of your code just say "canBeginPanning".
> 
> Is the explicit IntPoint constructor really needed?

When setting up the gestures, there is a distinction, but under the covers when determine whether or not we can pan, there isn't a distinction. The explicit constructor isn't needed.

>> Source/WebKit2/UIProcess/win/WebView.cpp:518
>> +    return SetGestureConfigPtr()(m_window, 0, 1, &gc, sizeof(GESTURECONFIG));
> 
> sizeof(gc) would be slightly more future-proof.

Fixed.

>> Source/WebKit2/UIProcess/win/WebView.cpp:526
>> +    }
> 
> Should this have an assertion similar to the one in onGestureNotify?

It probably should have the assert. Added:

ASSERT(GetGestureInfoPtr())
ASSERT(CloseGestureInfoHandlePtr())

>> Source/WebKit2/UIProcess/win/WebView.cpp:532
>> +    if (!GetGestureInfoPtr()(gestureHandle, reinterpret_cast<PGESTUREINFO>(&gi))) {
> 
> Is the reinterpret_cast really needed? gi is already a GESTUREINFO.

No, it isn't necessary.

>> Source/WebKit2/UIProcess/win/WebView.cpp:555
>> +        m_page->scrollByPanGesture(IntSize(-deltaX, -deltaY));
> 
> You could reverse the deltaX/Y calculations instead of negating them in the only place you use them.

I thought it would be easier to do it here, but I can reverse the calculations and add a comment.

>> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:206
>> +    GestureEnded()
> 
> I'd suggest these names:
> 
> GestureWillBegin(point)
> GestureDidScroll(size)
> GestureDidEnd()

Fixed.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:376
>> +        HitTestResult result(scollView->windowToContents(point));
> 
> Can you use assignment here instead of constructor syntax? Maybe that isn't allowed.

Sure. Fixed.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:390
>> +    if (m_gestureTargetNode) {
> 
> This can be an early return.

Fixed.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:407
>> +    m_gestureTargetNode->renderer()->enclosingLayer()->scrollByRecursively(size.width(), size.height());
> 
> Do we need to null-check enclosingLayer()?

I haven't seen a case where it was needed, but added it to be safe.

>> Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:412
>> +    m_gestureTargetNode = 0;
> 
> Please use nullptr instead of 0.

Done.

[Brian Weinstein]

Created attachment 88529 [details]
[PATCH] Fix v2

[Brian Weinstein]

Landed panning in r83113.