Scroll delta synchronization with MFBA

[Sunny Sachanandani]

I've been trying to get the main frame before activation mode for cc scheduler enabled and shipped in M52. This mode allows greater throughput by pipelining BeginMainFrame with activation and is currently behind a finch field trial (MainFrameBeforeActivation).

There's a bug with scrolling being broken and jumping around. The test case in the bug just ensures that the mouse wheel event handler takes 1s to run - the preventDefault isn't required to trigger the bug. I think this bug happens because the scroll deltas we send in BeginMainFrame are from the active tree and hence too old if we don't wait for activation i.e. MFBA mode.

I've been told that although scroll offset is shared between the active, pending and main trees; scroll deltas which are calculated from the scroll tree is not. That seems like a fundamental problem to ever having MFBA work correctly with scrolling. Any ideas on how to solve this?

Thanks!

- Sunny

[Brian C. Anderson]

It looks like SyncedProperty::PullDeltaForMainThread can send the same sent_delta_ twice since MFBA breaks assumptions in that class. It only tracks active_delta_ and sent_delta_. I tried adding a pending_delta_ in the following hacky patch and it seems to work. I wonder if there are some other weird side effects though. I don't know how clobber_active_value_ is supposed to work for example.

 

Thanks!

- Sunny

[Adrienne Walker]

+aelias

I think your thought of having a second staging delta called pending delta makes a lot of sense to me, as I think having two sent deltas in flight is the only way you're going to be able to handle this.  Regarding clobber_active_value, it comes during a commit when the main thread has cancelled a scroll animation.  It just means that during activation, the value coming from the pending_base should just be used directly and ignore outstanding deltas on the active tree.  I think this will continue to work with your patch, and you don't need a clobber_pending_value member.

There are definitely some scrolling tests in layer_tree_host_unittest_scroll.cc that try to test all these cases, so it'd be good to have some test cases for MFBA, scrolling, and aborted commits/activations there.

[Alexandre Elias]

The overall direction seems OK, but my main concern is to make sure the state model remains as simple as possible.  In particular, I think after https://codereview.chromium.org/2036833002, tracking active_delta_ and active_base_ separately should no longer be necessary, since the primary role of active_delta_ has been usurped by pending_delta_.  I'd prefer that the two active values be merged into active_current_.

This implies SyncedProperty::ActiveBase() and SyncedProperty::Delta() will need to be deleted, which is a nice API cleanup anyway.  Looks like there are no production callers of ActiveBase(), and there are only two remaining callers of SyncedProperty::Delta() that can feasibly be deleted:

A) the flooring hack in ScrollTree::PullDeltaForMainThread which I believe is equivalent to flooring directly on Current() instead

B) the changed_page_scale boolean in LayerTreeImpl::PushPageScaleFactorAndLimits which doesn't need to know a full delta value.

[Sunny Sachanandani]

One question: What should the effect of clobber_active_value_ be in between commit and activation? I.e. should the delta sent to the main thread in between commit and activation (of the previous frame) be identity if clobber_active_value_ = true?

[Alexandre Elias]

That sounds right, but personally I don't have a particularly strong opinion on what should happen there anyway (animation interruption is best-effort so it will look glitchy regardless of what we do).  There are a fair amount of tests for scroll animation interruption, so I think whatever makes the tests green should be good to go.

[Sunny Sachanandani]

I have a simpler CL here: https://codereview.chromium.org/2035543003/

The main idea is to keep track of the sent_delta_ that was sent for the pending tree - this is the pending_delta_ in my CL.