[webkit-reviews] review denied: [Bug 49824] WebKit2: Support Windows 7 gestures : [Attachment 88498] [PATCH] Fix

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 49824: WebKit2: Support Windows 7 gestures
https://bugs.webkit.org/show_bug.cgi?id=49824

Attachment 88498: [PATCH] Fix
https://bugs.webkit.org/attachment.cgi?id=88498&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.wid
th(), 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.