Sometimes scroll position is jerky during rubber-band, affects nytimes.com

[Beth Dakin]

To repro this issue at nytimes.com

1. Navigate to nytimes.com
2. Using a two-finger scroll gesture, pull the web page down to reveal linen at the top. Hold your fingers in place to keep the linen visible for a while.

Expected result: The linen keeps showing for as long as your fingers are down on the trackpad.
Actual result: After a moment, the top of the page will snap up and the linen will disappear.

<rdar://problem/12679549>

[Beth Dakin]

Created attachment 184140 [details]
Patch

[WebKit Review Bot]

Attachment 184140 [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/WebCore/page/scrolling/ScrollingTree.h:82:  The parameter name "isRubberBanding" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


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

[Simon Fraser (smfr)]

Comment on attachment 184140 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1929
> +    if (Page* page = m_frame->page()) {
> +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> +            if (!scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread())
> +                return scrollingCoordinator->isRubberBandInProgress();
> +        }
> +    }
> +#endif
> +
> +    // If the main thread updates the scroll position for this FrameView, we should return
> +    // ScrollAnimator::isRubberBandInProgress().
> +    if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
> +        return scrollAnimator->isRubberBandInProgress();

It's a shame that the ScrollingCoordinator and ScrollAnimator don't know about eachother, otherwise you could just ask one of them for the answer. It's pretty unwieldy to have to add code like this in several places.

[James Robinson]

Comment on attachment 184140 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1915
> +#if ENABLE(THREADED_SCROLLING)

why this compile-time guard?  The code this depends on (ScrollingCoordinator, etc) are all available outside this feature define and platforms that don't set this (say chromium) will still need this code

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 184140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184140&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1929
> > +    if (Page* page = m_frame->page()) {
> > +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> > +            if (!scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread())
> > +                return scrollingCoordinator->isRubberBandInProgress();
> > +        }
> > +    }
> > +#endif
> > +
> > +    // If the main thread updates the scroll position for this FrameView, we should return
> > +    // ScrollAnimator::isRubberBandInProgress().
> > +    if (ScrollAnimator* scrollAnimator = existingScrollAnimator())
> > +        return scrollAnimator->isRubberBandInProgress();
> 
> It's a shame that the ScrollingCoordinator and ScrollAnimator don't know about eachother, otherwise you could just ask one of them for the answer. It's pretty unwieldy to have to add code like this in several places.

Agreed. :-/

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 184140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184140&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1915
> > +#if ENABLE(THREADED_SCROLLING)
> 
> why this compile-time guard?  The code this depends on (ScrollingCoordinator, etc) are all available outside this feature define and platforms that don't set this (say chromium) will still need this code

Alright, I can remove it. I added it because the ScrollingCoordinator function does not do anything on Chromium right now, and didn't know if the bug existed in Chromium anyway.

[James Robinson]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 184140 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184140&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:1915
> > > +#if ENABLE(THREADED_SCROLLING)
> > 
> > why this compile-time guard?  The code this depends on (ScrollingCoordinator, etc) are all available outside this feature define and platforms that don't set this (say chromium) will still need this code
> 
> Alright, I can remove it. I added it because the ScrollingCoordinator function does not do anything on Chromium right now, and didn't know if the bug existed in Chromium anyway.

Rubber-banding isn't implemented at all in chromium's implementation of off-main-thread scrolling today, so an implementation of ScrollingCoordinator that returns false for isRubberBandInProgress() will be accurate until we fix that.

[Beth Dakin]

Thanks! http://trac.webkit.org/changeset/140571