Bug 60779 - Bug in rubber banding logic for scroll animators
: Bug in rubber banding logic for scroll animators
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Event Handling
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To: Jon Lee
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-13 09:56 PDT by Jon Lee
Modified: 2011-08-11 16:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (deleted)
2011-05-13 10:06 PDT, Jon Lee
no flags Details
Patch (41.67 KB, patch)
2011-05-13 10:31 PDT, Jon Lee
darin: review-
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch (60.59 KB, patch)
2011-05-13 14:45 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (60.27 KB, patch)
2011-05-13 14:50 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (62.11 KB, patch)
2011-05-14 11:07 PDT, Jon Lee
sam: review-
Details | Formatted Diff | Diff
Patch (61.77 KB, patch)
2011-05-14 14:04 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (61.80 KB, patch)
2011-05-15 19:50 PDT, Jon Lee
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (62.12 KB, patch)
2011-05-15 23:22 PDT, Jon Lee
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2011-05-13 09:56:17 PDT
<rdar://problem/9002109>
Comment 1 Jon Lee 2011-05-13 10:06:17 PDT
Created attachment 93473 [details]
Patch
Comment 2 Darin Adler 2011-05-13 10:22:05 PDT
Comment on attachment 93473 [details]
Patch

Not a WebKit patch.
Comment 3 Jon Lee 2011-05-13 10:31:40 PDT
Created attachment 93478 [details]
Patch
Comment 4 William Siegrist 2011-05-13 10:39:29 PDT
The content of attachment 93473 [details] has been deleted by
    William Siegrist <wsiegrist@apple.com>
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.
Comment 5 Early Warning System Bot 2011-05-13 11:13:15 PDT
Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8693246
Comment 6 Gustavo Noronha (kov) 2011-05-13 11:30:45 PDT
Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8695180
Comment 7 Simon Fraser (smfr) 2011-05-13 11:38:15 PDT
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.
Comment 8 Jon Lee 2011-05-13 11:58:37 PDT
(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.
Comment 9 Simon Fraser (smfr) 2011-05-13 12:01:30 PDT
(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.
Comment 10 WebKit Review Bot 2011-05-13 12:15:56 PDT
Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8699104
Comment 11 WebKit Review Bot 2011-05-13 12:22:34 PDT
Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8694234
Comment 12 Gyuyoung Kim 2011-05-13 13:10:35 PDT
Comment on attachment 93478 [details]
Patch

Attachment 93478 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8695211
Comment 13 Darin Adler 2011-05-13 13:59:45 PDT
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.
Comment 14 Jon Lee 2011-05-13 14:45:14 PDT
Created attachment 93516 [details]
Patch

Including Simon's and Darin's suggestions, as well as making additional spelling corrections to variable names.
Comment 15 WebKit Review Bot 2011-05-13 14:48:42 PDT
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.
Comment 16 Jon Lee 2011-05-13 14:50:59 PDT
Created attachment 93517 [details]
Patch
Comment 17 Simon Fraser (smfr) 2011-05-13 15:09:52 PDT
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
Comment 18 Jon Lee 2011-05-14 11:07:13 PDT
Created attachment 93558 [details]
Patch

Moved method to ScrollableArea, and added some additional logic to fix some regressions found in the rubber banding behavior.
Comment 19 Sam Weinig 2011-05-14 11:59:18 PDT
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...
Comment 20 Jon Lee 2011-05-14 14:04:55 PDT
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.
Comment 21 Jon Lee 2011-05-15 19:50:55 PDT
Created attachment 93599 [details]
Patch
Comment 22 Simon Fraser (smfr) 2011-05-15 22:16:01 PDT
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.
Comment 23 Jon Lee 2011-05-15 23:15:29 PDT
(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.
Comment 24 Jon Lee 2011-05-15 23:22:13 PDT
Created attachment 93611 [details]
Patch
Comment 25 Simon Fraser (smfr) 2011-05-16 07:57:16 PDT
Comment on attachment 93611 [details]
Patch

rs=me
Comment 26 Jon Lee 2011-05-16 10:29:55 PDT
http://trac.webkit.org/changeset/86584
Comment 27 Andrew Wilson 2011-05-16 13:20:00 PDT
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?
Comment 28 Jon Lee 2011-05-16 16:42:57 PDT
Fix to regression failures: http://trac.webkit.org/changeset/86597
Comment 29 Raphael Kubo da Costa (:rakuco) 2011-05-26 10:14:13 PDT
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.
Comment 30 Darin Adler 2011-06-18 11:40:08 PDT
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.
Comment 31 Rob Bradford 2011-07-21 09:01:09 PDT
(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
Comment 32 Nico Weber 2011-08-11 16:39:40 PDT
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?