[Webkit-unassigned] [Bug 60779] Bug in rubber banding logic for scroll animators

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-05-13 11:38:15 PST ---
(From update of attachment 93478)
View in context: https://bugs.webkit.org/attachment.cgi?id=93478&action=review

r- because this needs some renaming work

> Source/WebCore/dom/Document.cpp:425
> +    , m_countWheelEventHandlers(0)

'count' reads like a verb. I'd call it 'm_numWheelEventHandlers' or 'm_wheelEventHandlersCount'

> Source/WebCore/dom/Document.h:1101
> +    unsigned countWheelEventHandlers() const { return m_countWheelEventHandlers; }

This needs a new name too.

> Source/WebCore/page/ChromeClient.h:324
> +        virtual void updateCountWheelEventHandlers(unsigned) = 0;

This sounds like you want the callee to update their count. I think it's really 'numWheelEventHandlersChanged()', no?

> Source/WebCore/page/EventHandler.cpp:-2144
> -    view = m_frame->view();
> -    if (!view)
> -        return false;
> -

Why this change?

> Source/WebCore/page/Frame.cpp:1122
> +static unsigned sumTotalWheelEventHandlers(const Frame* frame)

Awkwardly named.

> Source/WebCore/page/Frame.cpp:1132
> +    for (Frame* child = frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
> +        count += sumTotalWheelEventHandlers(child);

You could walk the entire tree via the FrameTree rather than recursing.

> Source/WebCore/page/Frame.cpp:1137
> +void Frame::recalculateTotalWheelEventHandlers() const

Odd that a 'recalculate' method is const. This should be more like notifyChomeClientOfNewWheelEventHandlerCount() or something.

> Source/WebCore/page/Frame.h:211
> +        // Should only be called on the main frame of a page.
> +        void recalculateTotalWheelEventHandlers() const;

I don't see an ASSERT that matches the comment.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:791
> +        // after a gesture begins, we go through

Sentence case please.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:805
> +            if ((wheelEvent.deltaX() > 0 &&
> +                !m_scrollableArea->shouldRubberBandInDirection(ScrollLeft) &&
> +                (!m_scrollableArea->horizontalScrollbar() ||
> +                m_scrollableArea->scrollPosition(m_scrollableArea->horizontalScrollbar()) <= m_scrollableArea->minimumScrollPosition().x()))
> +                ||
> +                (wheelEvent.deltaX() < 0 &&
> +                !m_scrollableArea->shouldRubberBandInDirection(ScrollRight) &&
> +                (!m_scrollableArea->horizontalScrollbar() ||
> +                m_scrollableArea->scrollPosition(m_scrollableArea->horizontalScrollbar()) >= m_scrollableArea->maximumScrollPosition().x()))) {
> +                ScrollAnimator::handleWheelEvent(wheelEvent);

Ouch. Some inline helper functions would make this hugely more readable.

> Source/WebKit2/UIProcess/WebPageProxy.h:229
> +    bool shouldWaitForWheelEvents();

Should be const.

> Source/WebKit2/UIProcess/WebPageProxy.h:616
> +    void updateCountWheelEventHandlers(unsigned count) { m_wheelEventHandlerCount = count; }

updateWheelEventHandlersCount()

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:65
> +    UpdateCountWheelEventHandlers(unsigned count)

Rename

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:770
> +    default:
> +        return true;

The default clause is defeating the compiler's ability to warn if not all values are handled.

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