[webkit-reviews] review denied: [Bug 21794] Middle-click panning should be springloaded while dragging : [Attachment 29508] Version with corrected tabs and indentation

[bugzilla-daemon at webkit.org]

Jon Honeycutt <jhoneycutt at apple.com> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 21794: Middle-click panning should be springloaded while dragging
https://bugs.webkit.org/show_bug.cgi?id=21794

Attachment 29508: Version with corrected tabs and indentation
https://bugs.webkit.org/attachment.cgi?id=29508&action=review

------- Additional Comments from Jon Honeycutt <jhoneycutt at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 42539)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,20 @@
> +2009-04-15  Itai Danan  <idanan at chromium.org>
> +	   As per Bug 21794 (https://bugs.webkit.org/show_bug.cgi?id=21794),
> +	   added an option for springloaded panscroll instead of sticky
> +	   scroll. To understand why this is required see the discussion in
> +	   issue 24722 (https://bugs.webkit.org/show_bug.cgi?id=24722).
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * page/EventHandler.cpp:
> +	   (WebCore::EventHandler::handleMousePressEvent):
> +	   (WebCore::EventHandler::handleMouseReleaseEvent):
> +	   * page/Settings.cpp:
> +	   (WebCore::Settings::Settings):
> +	   (WebCore::Settings::setStickyPanScroll):
> +	   * page/Settings.h:
> +	   (WebCore::Settings::stickyPanScroll):
> +

Please fill out your ChangeLog to describe the changes you are making to each
function, unless the change is something very trivial, such as adding a getter
or setter. It's usually safe to be more rather than less verbose - see other
entries in the ChangeLog for examples.

> Index: WebCore/page/EventHandler.cpp
> ===================================================================
> --- WebCore/page/EventHandler.cpp	(revision 42539)
> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -1102,11 +1102,12 @@ bool EventHandler::handleMousePressEvent
>  
>  #if ENABLE(PAN_SCROLLING)
>      Page* page = m_frame->page();
> -    if (page && page->mainFrame()->eventHandler()->panScrollInProgress() ||
m_autoscrollInProgress) {
> -	   stopAutoscrollTimer();
> -	   invalidateClick();
> -	   return true;
> -    }
> +    if (m_frame->settings()->stickyPanScroll())
> +	   if (page && page->mainFrame()->eventHandler()->panScrollInProgress()
|| m_autoscrollInProgress) {
> +	       stopAutoscrollTimer();
> +	       invalidateClick();
> +	       return true;
> +	   }

WebKit style requires multiline control clauses to have braces, and you should
join these into one statement:

if (m_frame->settings()->stickyPanScroll() &&
    (page && page->mainFrame()->eventHandler()->panScrollInProgress() ||
m_autoscrollInProgress)) {

>      
>      if (mouseEvent.button() == MiddleButton && !mev.isOverLink()) {
>	   RenderObject* renderer = mev.targetNode()->renderer();
> @@ -1348,6 +1349,13 @@ bool EventHandler::handleMouseReleaseEve
>      m_mousePressed = false;
>      m_currentMousePosition = mouseEvent.pos();
>  
> +    if (!m_frame->settings()->stickyPanScroll())
> +	   if (m_frame->page() &&
m_frame->page()->mainFrame()->eventHandler()->panScrollInProgress() ||
m_autoscrollInProgress) {
> +	       stopAutoscrollTimer();
> +	       invalidateClick();
> +	       return true;
> +	   }
> +

Same comment as above.

> Index: WebCore/page/Settings.cpp
> ===================================================================
> --- WebCore/page/Settings.cpp (revision 42539)
> +++ WebCore/page/Settings.cpp (working copy)
> @@ -90,6 +90,7 @@ Settings::Settings(Page* page)
>      , m_usesEncodingDetector(false)
>      , m_maximumDecodedImageSize(std::numeric_limits<size_t>::max())
>      , m_allowScriptsToCloseWindows(false)
> +    , m_stickyPanScroll(true)
>  {
>      // A Frame may not have been created yet, so we initialize the
AtomicString 
>      // hash before trying to use it.
> @@ -435,4 +436,9 @@ void Settings::setAllowScriptsToCloseWin
>      m_allowScriptsToCloseWindows = allowScriptsToCloseWindows;
>  }
>  
> +void Settings::setStickyPanScroll(bool stickyPanScroll)
> +{
> +    m_stickyPanScroll = stickyPanScroll;
> +}
> +

We usually place the definition of setters in the header file.

>  } // namespace WebCore
> Index: WebCore/page/Settings.h
> ===================================================================
> --- WebCore/page/Settings.h	(revision 42539)
> +++ WebCore/page/Settings.h	(working copy)
> @@ -214,6 +214,9 @@ namespace WebCore {
>	   void setAllowScriptsToCloseWindows(bool);
>	   bool allowScriptsToCloseWindows() const { return
m_allowScriptsToCloseWindows; }
>  
> +	   void setStickyPanScroll(bool);
> +	   bool stickyPanScroll() const { return m_stickyPanScroll; }
> +

I think this would be more clear if you added the word "uses", e.g.
usesStickyPanScroll().

Thanks for the patch! r- for the issues above, but other than the style issues,
it looks good to me.