Bug 109712 – [Chromium] Compositor is applying scroll offset using 'programmatic' API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[John Knottenbelt]

Chromium's compositor invokes WebViewImpl::applyScrollAndScale where it calls mainFrameImpl()->frameView()->scrollBy(scrollDelta) and/or Page::setPageScaleFactor which calls into FrameView::setScrollPosition.

It looks like FrameView::setScrollPosition intended as the entry point for programmatic scrolls, given the code:

void FrameView::setScrollPosition(const IntPoint& scrollPoint)
{
    TemporaryChange<bool> changeInProgrammaticScroll(m_inProgrammaticScroll, true);
...
}

The compositor normally represents user scrolls (e.g. scroll gestures / flings) so by calling into Frame::setScrollPosition, we allowing the m_inProgrammaticScroll to differentiate between user scrolls and programmatic scrolls.

[John Knottenbelt]

If I change Page::setPageScaleFactor to use view->setScrollOffset instead of view->setScrollPosition, and WebViewImpl::applyScrollAndScale to use setScrollOffset instead of ScrollBy, we can avoid the compositor triggering the 'programmatic' scroll FrameView::setScrollPosition.

setScrollPosition looks like it is updating scroll bars, before calling setScrollOffset. I tried it in Chrome on Android, and scroll bars seemed to be updated OK, however.

[John Knottenbelt]

Another effect of my proposal above is that it breaks webkit_unit_test WebViewTest.ResetScrollAndScaleState. The pageScaleFactor fails to be restored. This is because by not calling into FrameView::setScrollPosition, we don't set the m_inProgrammaticScroll flag. This flag controls whether m_wasScrolledByUser can be set -- if we're in not in a programmatic scroll, m_wasScrolledByUser will be set. Finally, when restoring the scroll position, if the page has been scrolled by the user (m_wasScrolledByUser), the scroll and scale state won't be restored.

[John Knottenbelt]

So my understanding is as follows:

The FrameView::setScrollPosition API, is considered to be a programmatic scroll. The programmatic scroll flag stops the 'wasScrolledByUser' flag from being set. 

The wasScrolledByUser flag controls whether the scroll position should be restored when the page loads. If the user has already scrolled the page, the scroll position should not be restored.

wasScrolledByUser can be set from EventHandler::setFrameWasScrolledByUser() in response to certain scroll events, for example EventHandler::scrollOverflow.

The question, then, is: should scroll updates from Chromium's compositor be considered to be programmatic scrolls or user scrolls?

[James Robinson]

(In reply to comment #3)
> So my understanding is as follows:
> The question, then, is: should scroll updates from Chromium's compositor be considered to be programmatic scrolls or user scrolls?

I believe the answer is user scrolls - more specifically, it should behave the same as if WebKit handled the gestures leading to scrolls which should go down the user-scrolled path.

[Alexandre Elias]

Yes, scroll updates coming from CC should always be the result of gestures.  The only other situation that could cause them if content size changes and they are clamped, but generally speaking, WebKit should have pre-clamped things such that this doesn't happen.

[John Knottenbelt]

Created attachment 192032 [details]
Patch

[John Knottenbelt]

(In reply to comment #6)
> Created an attachment (id=192032) [details]
> Patch

The patch is rather rough, but I'd appreciate some feedback as I'm out of time to work on it for today.

Specifically, is the idea of adding in the flag to the FrameView::setScrollPosition and FrameView::scrollBy good/bad?

Also, I'm not yet sure of all the cases in WebViewImpl where a scroll should be considered programmatic and where it should be considered user. Up until now *all* of the scrolls from there have been treated as programmatic, so it's really a question of which ones should be treated as user.

Thanks!

[Build Bot]

Comment on attachment 192032 [details]
Patch

Attachment 192032 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17074117

[James Robinson]

Comment on attachment 192032 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1812
> +void FrameView::setScrollPosition(const IntPoint& scrollPoint, bool isProgrammaticScroll)

The existing code that uses setScrollPosition has not needed this parameter.  I don't think you should add it.

[James Robinson]

Today code uses a different set of APIs to handle user scrolls vs programmatic scrolls, for better or for worse.  Can you preserve this behavior in your patch (by calling the appropriate APIs) instead of adding more branches in existing codepaths?

[John Knottenbelt]

Created attachment 192246 [details]
Patch

[John Knottenbelt]

(In reply to comment #10)
> Today code uses a different set of APIs to handle user scrolls vs programmatic scrolls, for better or for worse.  Can you preserve this behavior in your patch (by calling the appropriate APIs) instead of adding more branches in existing codepaths?

It seems that there are various ways of scrolling non-programmatically. I modelled WebViewImpl::updateMainFrameScrollPosition after ScrollingCoordinator::updateMainFrameScrollPosition. This amounts to calling ScrollableArea::notifyScrollPositionChanged.

The difficulty is WebViewImpl::setPageScaleFactor. This calls Page::setPageScaleFactor which calls FrameView::setScrollPosition which is always a programmatic scroll. 

This patch works around this problem by scrolling first with updateMainFrameScrollPosition so that Page::setPageScaleFactor will only set the scale factor and not also (programmatically) scroll.

Is this the sort of thing you had in mind?

[WebKit Review Bot]

Comment on attachment 192246 [details]
Patch

Attachment 192246 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17014250

New failing tests:
html5lib/generated/run-tests16-data.html

[Eric Seidel (no email)]

I guess that test is flaky?

[James Robinson]

(In reply to comment #12)
> (In reply to comment #10)
> > Today code uses a different set of APIs to handle user scrolls vs programmatic scrolls, for better or for worse.  Can you preserve this behavior in your patch (by calling the appropriate APIs) instead of adding more branches in existing codepaths?
> 
> It seems that there are various ways of scrolling non-programmatically. I modelled WebViewImpl::updateMainFrameScrollPosition after ScrollingCoordinator::updateMainFrameScrollPosition. This amounts to calling ScrollableArea::notifyScrollPositionChanged.
> 
> The difficulty is WebViewImpl::setPageScaleFactor. This calls Page::setPageScaleFactor which calls FrameView::setScrollPosition which is always a programmatic scroll. 
> 
> This patch works around this problem by scrolling first with updateMainFrameScrollPosition so that Page::setPageScaleFactor will only set the scale factor and not also (programmatically) scroll.
> 
> Is this the sort of thing you had in mind?

Yes, this looks a lot more like what I had in mind.  I'm gardening until the middle of next week so I may not be able to take a careful look at this patch until that's over.

[John Knottenbelt]

(In reply to comment #13)
> (From update of attachment 192246 [details])
> Attachment 192246 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://webkit-commit-queue.appspot.com/results/17014250
> 
> New failing tests:
> html5lib/generated/run-tests16-data.html

Yes, definitely flaky: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=html5lib%2Fgenerated%2Frun-tests16-data.html

[John Knottenbelt]

Created attachment 192781 [details]
Patch

[John Knottenbelt]

- Removed the need for a separate test data file by using data:// URLs. 

- WebViewImpl::setPageScaleFactor is now performing a user scroll, which breaks  WebViewTest.ResetScrollAndScaleState test. Fixed the test by clearing the user scroll flag. 

- Added an additional test for short documents where the page must be zoomed in before the scroll can occur.

If you get any quiet times during your gardening (chance would be a fine thing!) would be great to get your review.

[John Knottenbelt]

Created attachment 192902 [details]
Patch

[John Knottenbelt]

- Simplify updateMainFrameScrollPosition

[Alexandre Elias]

Could you explain why you need to call frameView->setConstrainsScrollingToContentEdge(false) ?

[John Knottenbelt]

Created attachment 193120 [details]
Patch

[John Knottenbelt]

(In reply to comment #21)
> Could you explain why you need to call frameView->setConstrainsScrollingToContentEdge(false) ?

Hmm, actually I don't need to do this. I had copied this from ScrollingCompositor::updateMainFrameScrollPosition and it helped work around a problem where although the page scale had changed, the scroll was being prevented because the content hadn't yet realised the new scroll dimensions.

I've changed my patch to call Document::updateLayoutIgnorePendingStylesheets() if necessary in the same way as Page::setPageScaleFactor does. This updates the relevant constraints so that the subsequent scroll is possible.

[Alexandre Elias]

Comment on attachment 193120 [details]
Patch

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

LGTM

> Source/WebKit/chromium/src/WebViewImpl.cpp:3010
> +        if (!page()->settings()->applyPageScaleFactorInCompositor())

This setting is always true in Chromium now anyway, so you can delete this.

[John Knottenbelt]

Comment on attachment 193120 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3010
>> +        if (!page()->settings()->applyPageScaleFactorInCompositor())
> 
> This setting is always true in Chromium now anyway, so you can delete this.

I would prefer to keep it for now, as many of our unit tests (e.g. WebViewTest.ResetScrollAndScaleState) are running without this flag. I think it would be better to do a single focused change on removing the setting entirely, when the time is right. What do you think?

[Alexandre Elias]

The focused change is here, planning to land it soon: https://bugs.webkit.org/show_bug.cgi?id=111809

[John Knottenbelt]

Right, ok, will make this change.

It sounds like it makes sense to land my patch after yours? Otherwise, I will need to add a call to "webViewImpl->settings()->setApplyPageScaleFactorInCompositor(true);" in WebViewTest.ResetScrollAndScaleState, which you would need to then remove in your patch.

(In reply to comment #26)
> The focused change is here, planning to land it soon: https://bugs.webkit.org/show_bug.cgi?id=111809

[James Robinson]

Comment on attachment 193120 [details]
Patch

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

R=me

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:3010
>>> +        if (!page()->settings()->applyPageScaleFactorInCompositor())
>> 
>> This setting is always true in Chromium now anyway, so you can delete this.
> 
> I would prefer to keep it for now, as many of our unit tests (e.g. WebViewTest.ResetScrollAndScaleState) are running without this flag. I think it would be better to do a single focused change on removing the setting entirely, when the time is right. What do you think?

I think you should delete it. This code otherwise will be DOA

[John Knottenbelt]

Created attachment 193284 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193284 [details]
Patch

Clearing flags on attachment: 193284

Committed r145898: <http://trac.webkit.org/changeset/145898>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112512

[John Knottenbelt]

I discovered the reason for the flakiness. The tests use data: URLs to load in the HTML content to test with. On Desktop this is loaded synchronously, but on Android, the loads are asynchronous (See https://chromiumcodereview.appspot.com/10440068 ). 

Fix this problem by using the mocked URL loader that all the other tests are using.

[John Knottenbelt]

Created attachment 193601 [details]
Patch

[James Robinson]

Comment on attachment 193601 [details]
Patch

Nice find!

[WebKit Review Bot]

Comment on attachment 193601 [details]
Patch

Clearing flags on attachment: 193601

Committed r146091: <http://trac.webkit.org/changeset/146091>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.