Bug 60779 – Bug in rubber banding logic for scroll animators

• Component: UI Events
• Status: RESOLVED FIXED

[Jon Lee]

<rdar://problem/9002109>

[Jon Lee]

Created attachment 93473 [details]
Patch

[Darin Adler]

Comment on attachment 93473 [details]
Patch

Not a WebKit patch.

[Jon Lee]

Created attachment 93478 [details]
Patch

[William Siegrist]

The content of attachment 93473 [details] has been deleted by
    William Siegrist <[e-mail]>
who provided the following reason:

deleted by request of jonlee

The token used to delete this attachment was generated at 2011-05-13 10:39:09 PST.

[Early Warning System Bot]

Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8693246

[Gustavo Noronha (kov)]

Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8695180

[Simon Fraser (smfr)]

Comment on attachment 93478 [details]
Patch

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.

[Jon Lee]

(In reply to comment #7)

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

Earlier in the method this check is already performed. Unless there's a possibility that the frame will lose the view by the time we hit this line, I don't think we have to check twice.

--

I am noting all of your renaming suggestions, and will upload a new patch shortly.

[Simon Fraser (smfr)]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > > Source/WebCore/page/EventHandler.cpp:-2144
> > > -    view = m_frame->view();
> > > -    if (!view)
> > > -        return false;
> > > -
> > 
> > Why this change?
> 
> Earlier in the method this check is already performed. Unless there's a possibility that the frame will lose the view by the time we hit this line, I don't think we have to check twice.

You have to be really careful in code that allows arbitrary JS to be executed. This code is probably here because the event handler can run JS which results in the frame getting destroyed.

[WebKit Review Bot]

Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8699104

[WebKit Review Bot]

Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8694234

[Gyuyoung Kim]

Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8695211

[Darin Adler]

Comment on attachment 93478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93478&action=review

Noticed the word coalesced spelled with two l's as "coalesced" in a few places.

> Source/WebCore/page/ChromeClient.h:173
> +        virtual bool shouldRubberBandInDirection(ScrollDirection) = 0;

These will need to be reimplemented for many platforms. That’s why the build of every platform except for Mac is failing.

>> 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?

These will need to be reimplemented for many platforms. That’s why the build of every platform except for Mac is failing.

This name should probably be setWheelEventHandlerCount.

[Jon Lee]

Created attachment 93516 [details]
Patch

Including Simon's and Darin's suggestions, as well as making additional spelling corrections to variable names.

[WebKit Review Bot]

Attachment 93516 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:766:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jon Lee]

Created attachment 93517 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 93517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93517&action=review

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:775
> +static inline bool horizontalScrollPositionIsPastMinimum(ScrollableArea* scrollableArea)
> +{
> +    return scrollableArea->scrollPosition(scrollableArea->horizontalScrollbar()) <= scrollableArea->minimumScrollPosition().x();
> +}
> +
> +static inline bool horizontalScrollPositionIsPastMaximum(ScrollableArea* scrollableArea)
> +{
> +    return scrollableArea->scrollPosition(scrollableArea->horizontalScrollbar()) >= scrollableArea->maximumScrollPosition().x();
> +}

It seems reasonable to add these to ScrollableArea

[Jon Lee]

Created attachment 93558 [details]
Patch

Moved method to ScrollableArea, and added some additional logic to fix some regressions found in the rubber banding behavior.

[Sam Weinig]

Comment on attachment 93558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93558&action=review

Looking really good, but lets go another around.

> Source/WebCore/page/Frame.cpp:1130
> +    for (const Frame* frame = this; frame; frame = frame->tree()->traverseNext())
> +        if (frame->document())
> +            count += frame->document()->wheelEventHandlerCount();

This should have braces around for-loop.

> Source/WebCore/page/FrameView.cpp:314
> +    scrollAnimator()->didAddHorizontalScrollbar(scrollbar);

This should call ScrollView::didAddHorizontalScrollbar(scrollbar) instead of calling the animator directly.

> Source/WebCore/page/FrameView.cpp:319
> +    scrollAnimator()->willRemoveHorizontalScrollbar(scrollbar);

Same here.

> Source/WebKit2/UIProcess/API/C/WKPage.h:291
> +WK_EXPORT bool WKPageShouldWaitForWheelEvents(WKPageRef page);

I would rather this have a name and functionality that would work for all builds, including those without gesture events.  Perhaps, something along the lines of horizontal scrolls being noops...

[Jon Lee]

Created attachment 93569 [details]
Patch

Incorporated Sam's suggestions. Opted for "WebPageProxy::willHandleHorizontalScrollEvents" to relate it to the "didNotHandleWheelEvent" callback. Also, removed preprocessor so that it works for all builds.

[Jon Lee]

Created attachment 93599 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 93599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93599&action=review

> Source/WebCore/platform/ScrollableArea.h:137
> +    bool isHorizontalScrollerPinnedToLeft() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) <= minimumScrollPosition().x(); }
> +    bool isHorizontalScrollerPinnedToRight() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) >= maximumScrollPosition().x(); }
> +    

'left' and 'right' are loaded terms in a mixed writing-direction world. I'd rename these to isHorizontalScrollerAtMinimum() and isHorizontalScrollerAtMaximum or something.

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:149
> +    int m_aggregateScroll;

Not sure what 'aggregate' means here. Would 'cumulative' be a better term to use?

Also, if this is only ever in X, the name should reflect that.

[Jon Lee]

(In reply to comment #22)
> (From update of attachment 93599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93599&action=review
> 
> > Source/WebCore/platform/ScrollableArea.h:137
> > +    bool isHorizontalScrollerPinnedToLeft() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) <= minimumScrollPosition().x(); }
> > +    bool isHorizontalScrollerPinnedToRight() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) >= maximumScrollPosition().x(); }
> > +    
> 
> 'left' and 'right' are loaded terms in a mixed writing-direction world. I'd rename these to isHorizontalScrollerAtMinimum() and isHorizontalScrollerAtMaximum or something.

Done.

> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.h:149
> > +    int m_aggregateScroll;
> 
> Not sure what 'aggregate' means here. Would 'cumulative' be a better term to use?
> 
> Also, if this is only ever in X, the name should reflect that.

Renamed to m_cumulativeHorizontalScroll.

[Jon Lee]

Created attachment 93611 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 93611 [details]
Patch

rs=me

[Jon Lee]

http://trac.webkit.org/changeset/86584

[Andrew Wilson]

This looks like it's causing crashes downstream in Chromium:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2FaboutBlank%2Fwindow-open-self-about-blank.html&showExpectations=true&group=%40ToT%20-%20chromium.org

Are you sure that willRemoveHorizontalScrollbar() is always guaranteed to be called exactly once for each call to didAddHorizontalScrollbar()? Not sure whether this is a bug in the Chromium platform layer calling this routine too often, or an invalid assumption in the patch?

[Jon Lee]

Fix to regression failures: http://trac.webkit.org/changeset/86597

[Raphael Kubo da Costa (:rakuco)]

I'm on r87310, but still experience the same crash described in comment #27 while running some DRT tests on the EFL port, such as tables/mozilla/core/one_row.html, fast/dom/createElementNS.html or fast/dom/clone-node-form-elements-with-attr.html.

[Darin Adler]

If a bug fix is causing problems or crashes, reopening the bug is not the way to report it. That will just confuse people about the state of the patch or bug. It makes sense to reopen the bug if we rolled out the patch, but otherwise, please file a new bug about the new problem and add comments to the old bug.

[Rob Bradford]

(In reply to comment #30)
> If a bug fix is causing problems or crashes, reopening the bug is not the way to report it. That will just confuse people about the state of the patch or bug. It makes sense to reopen the bug if we rolled out the patch, but otherwise, please file a new bug about the new problem and add comments to the old bug.

Opened that corresponding bug report: https://bugs.webkit.org/show_bug.cgi?id=64954

[Nico Weber]

Comment on attachment 93611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93611&action=review

> Source/WebCore/dom/Document.h:1103
> +    // Used to keep track of horizontal scrollbars and onmousewheel event handlers only.

The "horizontal scrollbars" part sounds wrong to me. The code looks like this is just the number of onmousewheel events, and nothing else?