Bug 60779 - Bug in rubber banding logic for scroll animators
: Bug in rubber banding logic for scroll animators
Status: RESOLVED FIXED
: WebKit
Event Handling
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2011-05-13 09:56 PST by
Modified: 2011-08-11 16:39 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-13 09:56:17 PST
<rdar://problem/9002109>
------- Comment #1 From 2011-05-13 10:06:17 PST -------
Created an attachment (id=93473) [details]
Patch
------- Comment #2 From 2011-05-13 10:22:05 PST -------
(From update of attachment 93473 [details])
Not a WebKit patch.
------- Comment #3 From 2011-05-13 10:31:40 PST -------
Created an attachment (id=93478) [details]
Patch
------- Comment #4 From 2011-05-13 10:39:29 PST -------
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 From 2011-05-13 11:13:15 PST -------
(From update of attachment 93478 [details])
Attachment 93478 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8693246
------- Comment #6 From 2011-05-13 11:30:45 PST -------
(From update of attachment 93478 [details])
Attachment 93478 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8695180
------- Comment #7 From 2011-05-13 11:38:15 PST -------
(From update of attachment 93478 [details])
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 From 2011-05-13 11:58:37 PST -------
(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 From 2011-05-13 12:01:30 PST -------
(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 From 2011-05-13 12:15:56 PST -------
(From update of attachment 93478 [details])
Attachment 93478 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8699104
------- Comment #11 From 2011-05-13 12:22:34 PST -------
(From update of attachment 93478 [details])
Attachment 93478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8694234
------- Comment #12 From 2011-05-13 13:10:35 PST -------
(From update of attachment 93478 [details])
Attachment 93478 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8695211
------- Comment #13 From 2011-05-13 13:59:45 PST -------
(From update of attachment 93478 [details])
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 From 2011-05-13 14:45:14 PST -------
Created an attachment (id=93516) [details]
Patch

Including Simon's and Darin's suggestions, as well as making additional spelling corrections to variable names.
------- Comment #15 From 2011-05-13 14:48:42 PST -------
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 From 2011-05-13 14:50:59 PST -------
Created an attachment (id=93517) [details]
Patch
------- Comment #17 From 2011-05-13 15:09:52 PST -------
(From update of attachment 93517 [details])
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 From 2011-05-14 11:07:13 PST -------
Created an attachment (id=93558) [details]
Patch

Moved method to ScrollableArea, and added some additional logic to fix some regressions found in the rubber banding behavior.
------- Comment #19 From 2011-05-14 11:59:18 PST -------
(From update of attachment 93558 [details])
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 From 2011-05-14 14:04:55 PST -------
Created an attachment (id=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 From 2011-05-15 19:50:55 PST -------
Created an attachment (id=93599) [details]
Patch
------- Comment #22 From 2011-05-15 22:16:01 PST -------
(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.

> 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 From 2011-05-15 23:15:29 PST -------
(In reply to comment #22)
> (From update of attachment 93599 [details] [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 From 2011-05-15 23:22:13 PST -------
Created an attachment (id=93611) [details]
Patch
------- Comment #25 From 2011-05-16 07:57:16 PST -------
(From update of attachment 93611 [details])
rs=me
------- Comment #26 From 2011-05-16 10:29:55 PST -------
http://trac.webkit.org/changeset/86584
------- Comment #27 From 2011-05-16 13:20:00 PST -------
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 From 2011-05-16 16:42:57 PST -------
Fix to regression failures: http://trac.webkit.org/changeset/86597
------- Comment #29 From 2011-05-26 10:14:13 PST -------
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 From 2011-06-18 11:40:08 PST -------
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 From 2011-07-21 09:01:09 PST -------
(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 From 2011-08-11 16:39:40 PST -------
(From update of attachment 93611 [details])
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?