[webkit-reviews] review denied: [Bug 21794] Middle-click panning should be springloaded while dragging : [Attachment 27128] Turn off pan scroll when middle button is released in a drag gesture

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Loren Segal
<lsegal at soen.ca>'s request for review:
Bug 21794: Middle-click panning should be springloaded while dragging
https://bugs.webkit.org/show_bug.cgi?id=21794

Attachment 27128: Turn off pan scroll when middle button is released in a drag
gesture
https://bugs.webkit.org/attachment.cgi?id=27128&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 40333)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,12 @@
> +2009-01-28  Loren Segal  <lsegal at soen.ca>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Turn off pan scrolling when middle mouse button is released during a
mouse drag gesture.

You should mention the bug title and URL in your ChangeLog.

> +
> +	   * page/EventHandler.cpp:
> +	   * page/EventHandler.h:

The prepare-ChangeLog script will insert the names of the functions you
modified. You should make a comment about each function you modified explaining
your change.

> +++ page/EventHandler.h	(working copy)
> @@ -216,8 +216,22 @@ private:
>      void handleKeyboardSelectionMovement(KeyboardEvent*);
>      
>      Cursor selectCursor(const MouseEventWithHitTestResults&, Scrollbar*);
> +    Cursor selectPanScrollCursor();
>      void setPanScrollCursor();

I don't think we need selectPanScrollCursor anymore. You can just put the code
into setPanScrollCursor. That should lead to a smaller diff.

> @@ -299,6 +313,7 @@ private:
>  
>      IntPoint m_panScrollStartPos;
>      bool m_panScrollInProgress;
> +    bool m_panScrollIsScrolling;

The name "m_panScrollIsScrolling" is a little confusing when taken together
with m_panScrollInProgress. I'd suggest something like m_panScrollHasScrolled
or m_panScrollHasScrolledSinceStarting.

Let's take care of these last few issues and get this landed!