[webkit-reviews] review denied: [Bug 26488] No Support for Single Finger or Two Finger Panning in Windows 7 : [Attachment 31557] Patch to enable single finger and two-finger panning with Window bounce
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 19 13:51:25 PDT 2009
Steve Falkenburg <sfalken at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 26488: No Support for Single Finger or Two Finger Panning in Windows 7
https://bugs.webkit.org/show_bug.cgi?id=26488
Attachment 31557: Patch to enable single finger and two-finger panning with
Window bounce
https://bugs.webkit.org/attachment.cgi?id=31557&action=review
------- Additional Comments from Steve Falkenburg <sfalken at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 44858)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,19 @@
> +2009-06-19 Brian Weinstein <bweinstein at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + https://bugs.webkit.org/show_bug.cgi?id=26488
Would be nice to include a short description of the bugzilla bug w/ the URL
> Index: WebCore/platform/PlatformWheelEvent.h
> ===================================================================
> --- WebCore/platform/PlatformWheelEvent.h (revision 44832)
> +++ WebCore/platform/PlatformWheelEvent.h (working copy)
> @@ -99,6 +99,7 @@ namespace WebCore {
>
> #if PLATFORM(WIN)
> PlatformWheelEvent(HWND, WPARAM, LPARAM, bool isMouseHWheel);
> + PlatformWheelEvent(HWND, float, float, float, float);
We include parameter names when it isn't obvious from the type. 4 floats
strung together isn't understandable for someone to know how to call this.
Also, it seems odd to call this PlatformWheelEvent if we're not actually
calling this in response to a wheel event.
> #endif
>
> #if PLATFORM(WX)
> Index: WebCore/platform/win/WheelEventWin.cpp
> ===================================================================
> --- WebCore/platform/win/WheelEventWin.cpp (revision 44832)
> +++ WebCore/platform/win/WheelEventWin.cpp (working copy)
> @@ -62,6 +62,29 @@ static int verticalScrollLines()
> return scrollLines;
> }
>
> +PlatformWheelEvent::PlatformWheelEvent(HWND hWnd, float deltaX, float
deltaY, float xLoc, float yLoc)
> + : m_isAccepted(false)
> + , m_shiftKey(false)
> + , m_ctrlKey(false) // FIXME: Do we need modifier keys for gestures?
Does the control key do something special for gestures?
> + , m_altKey(false)
> + , m_metaKey(false)
> +{
> + // Scroll down scrolling in x direction to make it less finicky
> + m_deltaX = deltaX / 3;
The divide by 3 here looks out of place - I think you copied it from the other
PlatformWheelEvent ctor - would be nice to combine the common code for these.
> + m_deltaY = deltaY;
> +
> + m_wheelTicksX = m_deltaX;
> + m_wheelTicksY = m_deltaY;
> +
> + // Global Position is just x, y location of event
> + POINT point = {xLoc, yLoc};
> + m_globalPosition = (IntPoint) point;
We like to avoid C style casts. I think you can just remove the cast
completely, since there's a constructor for IntPoint that takes a POINT:
m_globalPosition = point;
> +
> + // Position needs to be translated to our client
> + ScreenToClient(hWnd, &point);
> + m_position = (IntPoint) point;
Same problem here. I'd recommend doing this without a cast.
> Index: WebKit/win/WebView.cpp
> ===================================================================
> --- WebKit/win/WebView.cpp (revision 44832)
> +++ WebKit/win/WebView.cpp (working copy)
> @@ -95,6 +95,7 @@
> #include <WebCore/ResourceHandle.h>
> #include <WebCore/ResourceHandleClient.h>
> #include <WebCore/ScriptValue.h>
> +#include <WebCore/Scrollbar.h>
> #include <WebCore/ScrollbarTheme.h>
> #include <WebCore/SecurityOrigin.h>
> #include <WebCore/SelectionController.h>
> @@ -127,6 +128,18 @@
> #include <ShlObj.h>
> #include <tchar.h>
> #include <windowsx.h>
> +#include "SoftLinking.h"
> +#include "WindowsTouch.h"
Given the current include sorting in this file, I'd recommend moving these 2
new lines up with the other includes for files within WebKit.
>
> +bool WebView::gestureNotify(WPARAM wParam, LPARAM lParam)
> +{
> + GESTURENOTIFYSTRUCT* gn = (GESTURENOTIFYSTRUCT*) lParam;
You should use a C++ style cast.
> +
> + Frame* coreFrame = core(m_mainFrame);
> + if (!coreFrame)
> + return false;
> +
> + ScrollView* view = (ScrollView*) coreFrame->document()->view();
This should not require a cast, since view() returns a FrameView* and FrameView
is derived from ScrollView.
Also, I think this can be coreFrame->view().
> + if (!view)
> + return false;
> +
> + // If we don't have this function, we shouldn't be receiving this
message
> + if (!SetGestureConfigPtr())
> + return false;
I think this can just be an assert - I don't think you could end up with the
WM_GESTURENOTIFY if the lib wasn't present.
> +
> + DWORD dwPanWant;
> + DWORD dwPanBlock;
> +
> + if (gn->ptsLocation.x > view->visibleWidth()) {
> + //We are in the scrollbar, turn off panning
> + dwPanWant = GC_PAN | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
> + dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY |
GC_PAN_WITH_SINGLE_FINGER_VERTICALLY;
> + } else {
> + //We aren't in the scrollbar, turn on panning
> + dwPanWant = GC_PAN | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY |
GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
> + dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;
> + }
> +
> + GESTURECONFIG gc = { GID_PAN, dwPanWant , dwPanBlock } ;
> + UINT uiGcs = 1;
Not necessary to stick this in a local temporarily. I'm not sure the name uiGcs
adds a lot of value.
> + BOOL bResult = SetGestureConfigPtr()(m_viewWindow, 0, uiGcs, &gc,
sizeof(GESTURECONFIG));
> +
> + return bResult;
could just return the result of the call directly. No need to assign it to a
temporary local.
> +}
> +
> +bool WebView::gesture(WPARAM wParam, LPARAM lParam)
> +{
> + GESTUREINFO gi = {0};
> +
> + BOOL eventValue = false;
> +
> + PlatformWheelEvent* wheelEvent;
> + long currentX;
> + long currentY;
> + long deltaX;
> + long deltaY;
Can define these closer to their first use.
> +
> + // We want to bail out if we don't have either of these functions
> + if (!GetGestureInfoPtr() || !CloseGestureInfoHandlePtr())
> + return false;
> +
> + gi.cbSize = sizeof(GESTUREINFO);
Could just move the gi definition directly above this line. That would make its
initial state more clear.
> +
> + BOOL bResult = GetGestureInfoPtr()((HGESTUREINFO)lParam, (PGESTUREINFO)
&gi);
> +
> + if (bResult){
Early return would be better here.
Don't need bResult. Could just do if (GetGesture...
You need a space between ){
> + switch (gi.dwID){
Space between ){
> + case GID_BEGIN:
> + m_lastPanX = gi.ptsLocation.x;
> + m_lastPanY = gi.ptsLocation.y;
> +
> + CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);
C style cast - shouldn't use this.
> + break;
> + case GID_ZOOM:
> + // FIXME: Do we want to implement a new kind of zoom here?
<rdar://problem/6980304>
> + CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);
C style cast - shouldn't use this.
> + break;
> + case GID_PAN: {
> + Frame* coreFrame = core(m_mainFrame);
> + if (!coreFrame)
> + return false;
> +
> + ScrollView* view = (ScrollView*)
coreFrame->document()->view();
I don't think this cast is needed.
> + if (!view)
> + return false;
> +
> + Scrollbar* vertScrollbar = view->verticalScrollbar();
> + if (!vertScrollbar)
> + return true; //No panning of any kind when no
vertical scrollbar, matches IE8
Doesn't this leak the gesture info?
> +
> + // Where are the fingers currently?
> + currentX = gi.ptsLocation.x;
> + currentY = gi.ptsLocation.y;
> +
> + // How far did we pan in each direction?
> + deltaX = currentX - m_lastPanX;
> + deltaY = currentY - m_lastPanY;
> +
> + // Calculate the overpan for window bounce
> + m_yOverpan -= m_lastPanY - currentY;
> + m_xOverpan -= m_lastPanX - currentX;
> +
> + // Update our class variables with updated values
> + m_lastPanX = currentX;
> + m_lastPanY = currentY;
> +
> + // Represent the pan gesture as a mouse wheel event
> + wheelEvent = new PlatformWheelEvent(m_viewWindow, deltaX,
deltaY, currentX, currentY);
It looks like you're leaking the wheelEvent. Can you just make this stack
allocated?
> + eventValue =
coreFrame->eventHandler()->handleWheelEvent(*wheelEvent);
> +
> + // FIXME: Soon we'll have horizontal window bounce too, just
haven't implemented yet
> + if (vertScrollbar->currentPos() == 0) {
> + // FIXME: Is something else besides 0 here? 0 worked in
tests, and window bounce worked
> + if (UpdatePanningFeedbackPtr()) {
Is it useful to check for all of these functions separately? Seems like if 1 is
available, they'd all be there.
> + UpdatePanningFeedbackPtr()(m_viewWindow, 0,
m_yOverpan, gi.dwFlags & GF_INERTIA);
> + }
> + } else if (vertScrollbar->currentPos() >=
vertScrollbar->maximum()) {
> + if (UpdatePanningFeedbackPtr()) {
> + UpdatePanningFeedbackPtr()(m_viewWindow, 0,
m_yOverpan, gi.dwFlags & GF_INERTIA);
> + }
> + }
> + if (gi.dwFlags & GF_BEGIN) {
> + if (BeginPanningFeedbackPtr()) {
> + BeginPanningFeedbackPtr()(m_viewWindow);
> + m_yOverpan = 0;
> + }
> + } else if (gi.dwFlags & GF_END) {
> + if (EndPanningFeedbackPtr()) {
> + EndPanningFeedbackPtr()(m_viewWindow, true);
> + m_yOverpan = 0;
> + }
> + }
> +
> + CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);
C style cast - shouldn't use this.
> + return eventValue;
Is it correct to close the gesture info if we could return false?
> +
> + break;
> + }
> + default:
> + // You have encountered an unknown gesture - return false to
pass it to DefWindowProc
> + CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);
C style cast - shouldn't use this.
> + return false;
> + break;
> + }
> + CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);
> + } else {
> + DWORD dwErr = GetLastError();
> + ASSERT(dwErr <= 0); //dwErr > 0 is an error
condition
Not sure this is a useful assert- seems odd since in this code path, we
actually expect an error.
> Index: WebKit/win/WebView.h
> ===================================================================
> --- WebKit/win/WebView.h (revision 44832)
> +++ WebKit/win/WebView.h (working copy)
> @@ -742,6 +742,8 @@ public:
> bool onUninitMenuPopup(WPARAM, LPARAM);
> void performContextMenuAction(WPARAM, LPARAM, bool byPosition);
> bool mouseWheel(WPARAM, LPARAM, bool isMouseHWheel);
> + bool gesture(WPARAM, LPARAM);
> + bool gestureNotify(WPARAM, LPARAM);
> bool execCommand(WPARAM wParam, LPARAM lParam);
> bool keyDown(WPARAM, LPARAM, bool systemKeyDown = false);
> bool keyUp(WPARAM, LPARAM, bool systemKeyDown = false);
> @@ -911,6 +913,12 @@ protected:
> HWND m_topLevelParent;
>
> OwnPtr<HashSet<WebCore::String> > m_embeddedViewMIMETypes;
> +
> + //Variables needed to store gesture information
> + long m_lastPanX;
> + long m_lastPanY;
> + long m_xOverpan;
> + long m_yOverpan;
You need to initialize these in the WebView ctor.
> };
>
> #endif
> Index: WebKit/win/WindowsTouch.h
> ===================================================================
> --- WebKit/win/WindowsTouch.h (revision 0)
> +++ WebKit/win/WindowsTouch.h (revision 0)
> +#ifndef WindowsTouch_H
> +#define WindowsTouch_H
I think our coding guidelines call for _h instead of _H.
> +#endif
> \ No newline at end of file
You should add a newline.
>
> Property changes on: WebKit/win/WindowsTouch.h
> ___________________________________________________________________
> Added: svn:executable
> + *
For new files you should:
svn pd svn:executable
svn ps svn:eol-style native
More information about the webkit-reviews
mailing list