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.
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.
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.
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?
(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.
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.
Created attachment 192032 [details] Patch
(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!
Comment on attachment 192032 [details] Patch Attachment 192032 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17074117
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.
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?
Created attachment 192246 [details] Patch
(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?
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
I guess that test is flaky?
(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.
(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
Created attachment 192781 [details] Patch
- 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.
Created attachment 192902 [details] Patch
- Simplify updateMainFrameScrollPosition
Could you explain why you need to call frameView->setConstrainsScrollingToContentEdge(false) ?
Created attachment 193120 [details] Patch
(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.
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.
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?
The focused change is here, planning to land it soon: https://bugs.webkit.org/show_bug.cgi?id=111809
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
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
Created attachment 193284 [details] Patch
Comment on attachment 193284 [details] Patch Clearing flags on attachment: 193284 Committed r145898: <http://trac.webkit.org/changeset/145898>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 112512
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.
Created attachment 193601 [details] Patch
Comment on attachment 193601 [details] Patch Nice find!
Comment on attachment 193601 [details] Patch Clearing flags on attachment: 193601 Committed r146091: <http://trac.webkit.org/changeset/146091>