cc'ing bokan@ who recently looked at another scroll animation issue.

+ajuma@ and loyso@ who own animations in CC.

Presumably, fixing ScrollAnimationUpdateTarget to revive a "finished" animation or return false wouldn't really help right? Since we're already trying to "update existing" in the ScrollAnimated/ScrollAnimatedBegin methods. The test is hoping to bubble which requires us to be in the "targetting" part of those methods so we really need to wait until the CurrentlyScrollingNode is cleared. That happens in LayerTreeHostImpl::ScrollOffsetAnimationFinished so maybe we could make that happen sooner?

Alternatively, ScrollAnimationUpdateTarget could return failure if the animation is already finished and the calling code could then clear it and start a new one.

Does ScrollAnimationUpdateTarget() actually fail to apply the delta to the currently-scrolling node (i.e. this is a real bug outside of test code)?

Or is it just that the test expected chaining to a different node, and is using scroll-offset polling as a flaky proxy for animation completion?

The loops in that test look pretty hacky.  I wonder if we can plumb an animation completion signal through the RenderFrameProxy.

Alternatively the test could skip the smooth scrolling paths by setting has_precise_scrolling_deltas on the WebMouseWheelEvent.

skobes@ - Thanks for the tip about has_precise_scrolling_deltas ... I didn't realize that could be used to skip the animation pathways. I'm going to use that, in the short term at least, to get this test back online.

I believe that ScrollAnimationUpdateTarget() actually fails to apply the delta. Now, because of the small deltas from the test resulting in animations with a single step, it may be that this doesn't reproduce as easily in the real world.

Also, in the real world it's probably quite unnoticeable if the occasional scroll wheel delta gets dropped, and so the bug hasn't resulted in any reports (beyond this one).

Finally, I would agree it would be useful to plumb an animation completion signal, as otherwise any browser test at present has no way to synchronize, making it difficult (or impossible) to write a browser test for scroll animation that won't be potentially flakey.

I should add, for example, that the current bug I've been fixing with OOPIF scroll bubbling has been due to the logic in the animation path, so having a browser test that does use the animation path is important.

SitePerProcessBrowserTest.ScrollBubblingFromOOPIFTest is extremely flaky on this CQ bot:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng

Could you please disable it until this is fixed?

flackr@, feel free to reassign if you know a better owner.

re #7 ... the CL to re-enable the test should have waited until another CL (to fix the DCHECK issue) landed ... I've reverted it.

I spent a bit more time investigating this, and here's what I've found:

The problem seems to be that, when the test happens to hit ScrollAnimationUpdateTarget, it adds a delta so small that the animation curve assigns it a zero duration. Since the existing animation is already at its end, the next call to AnimationPlayer::MarkFinishedAnimations() sets the run state on the animation to 'finished', and the animation is deleted even though it has unconsumed scroll delta.

Some of the points of interest in the code:

1) MarkFinishedAnimations() calls Animation::IsFinishedAt(), which (in the case of our test) indicates the animation is finished based on the value returned by curve_->Duration().

2) In ScrollOffsetAnimationCurve::UpdateTarget() our test hits the branch 

if ((total_animation_duration_ - last_retarget_).is_zero())

and sets total_animation_duration_ to zero, since the new delta is very small.

3) ScrollOffsetAnimationCurve::UpdateTarget() is called from ScrollOffsetAnimationsImpl::ScrollAnimationUpdateTarget(). Since UpdateTarget() doesn't return any sort of status, the caller has no idea the duration is zero (or at least doesn't attempt to check). ScrollAnimationUpdateTarget() exits from the final return statement, which is always true, even though (in our case) the curve duration is zero.

I don't know that the proper way to fix this is, but it seems wrong to consider the update successful when both (i) the curve duration hasn't changed, and (ii) the animation is terminating based solely on its duration, even when the curve has unconsumed scroll delta.

I've uploaded an *experimental* cl at:

https://codereview.chromium.org/2840883003

This is *not* meant to be a fix for the problem, but rather shows that if we detect the lack of change in the curve duration and use it to return false, and then use that in turn to force creation of a new animation, the test at least completes successfully.

That's interesting.  When SOAI::ScrollAnimationUpdateTarget runs, is the animation already marked finished?  If so, we should probably return false without touching it (just as if there were no animation).

If not, then there's something funny about the ordering of input and animation updates.

I'm guessing this isn't P1, as this bug hasn't seen recent activity. Dropping to P2. sunyunjia@, please upgrade priority if this is something we want to get done soon. Thanks!