[Webkit-unassigned] [Bug 21794] Middle-click panning should be springloaded while dragging

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21794


jhoneycutt at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29508|review?                     |review-
               Flag|                            |




------- Comment #20 from jhoneycutt at apple.com  2009-05-22 22:07 PDT -------
(From update of attachment 29508)
> 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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.