We should support Windows 7 gesture support that we support in old WebKit.
Created attachment 88498 [details] [PATCH] Fix
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.
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.
Created attachment 88529 [details] [PATCH] Fix v2
Landed panning in r83113.