Communicate MT changes to impl-only scroll offset animations

Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

Note that in this CL, this path is only exercised from ScrollAnchor.cpp

BUG=594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f3a9e63578aff4d5c814a5bb374665a4a0812834
Cr-Commit-Position: refs/heads/master@{#394947}

[ymalik, 2016-05-06 00:03:48]

Description was changed from

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

BUG=https://crbug.com/594456
==========

to

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

BUG=https://crbug.com/594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-05-06 00:11:14]

Description was changed from

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

BUG=https://crbug.com/594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

BUG=https://crbug.com/594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-05-06 00:11:15]

ymalik@chromium.org changed reviewers:
+ ajuma@chromium.org, loyso@chromium.org, skobes@chromium.org

[ymalik, 2016-05-06 00:12:34]

Description was changed from

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

BUG=https://crbug.com/594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

Note that in this CL, this path is only exercised from ScrollAnchor.cpp

BUG=https://crbug.com/594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-05-06 01:04:39]

Big things first.

https://codereview.chromium.org/1950243005/diff/1/cc/animation/animation_host.h
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/1950243005/diff/1/cc/animation/animation_host...
cc/animation/animation_host.h:160: bool
HasScrollOffsetAnimationUpdatesForTesting() const;
It's better to have a general accessor:
ScrollOffsetAnimations& AnimationHost::scroll_offset_animations() const; with
these methods.

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.h (right):

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.h:45:
std::vector<ScrollOffsetAnimationUpdate> queue_;
using UpdatesQueue = std::vector<ScrollOffsetAnimationUpdate>;

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations_impl.cc (right):

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations_impl.cc:127:
ReattachScrollOffsetPlayerIfNeeded(element_id);
element_id == scroll_offset_animation_player_->element_id here. Do we need to
reattach? Also, is it a valid case when we keep spaming impl thread with these
udates? Can we replace element_id !=
scroll_offset_animation_player_->element_id() with a DCHECK?

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:9:
#include "cc/animation/animation_host.h"
You can use forward declaration here as well.

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/animation/CompositorAnimationTimeline.h
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/animation/CompositorAnimationTimeline.h:34:
std::unique_ptr<CompositorAnimationHost> m_compositorAnimationHost;
No need to store it, I think. Return it by value - CompositorAnimationHost
compositorAnimationHost();
That's just a wrapper.

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:271:
cc::ScrollOffsetAnimationUpdate update;
You directly use cc update type here (without wrapper). Not sure if it's
compliant with our architectural vision.
Can you change the intarface to
addImplOnlyScrollOffsetAnimationChange(adjustment); ? One function for one
update type.

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:139:
// They should probably be reworked via LayerTreeView.
Since you send the updates from ScrollingCoordinator, it should be easy to write
it correctly from the beginning.
You have a pointer to WLTV to memorize in
layerTreeViewInitialized(WebLayerTreeView&
layerTreeView)/willCloseLayerTreeView.

[ymalik, 2016-05-06 05:41:49]

https://codereview.chromium.org/1950243005/diff/1/cc/animation/animation_host.h
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/1950243005/diff/1/cc/animation/animation_host...
cc/animation/animation_host.h:160: bool
HasScrollOffsetAnimationUpdatesForTesting() const;
On 2016/05/06 01:04:38, loyso wrote:
> It's better to have a general accessor:
> ScrollOffsetAnimations& AnimationHost::scroll_offset_animations() const; with
> these methods.

Done.

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.h (right):

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.h:45:
std::vector<ScrollOffsetAnimationUpdate> queue_;
On 2016/05/06 01:04:38, loyso wrote:
> using UpdatesQueue = std::vector<ScrollOffsetAnimationUpdate>;

Done.

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations_impl.cc (right):

https://codereview.chromium.org/1950243005/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations_impl.cc:127:
ReattachScrollOffsetPlayerIfNeeded(element_id);
On 2016/05/06 01:04:38, loyso wrote:
> element_id == scroll_offset_animation_player_->element_id here. Do we need to
> reattach? Also, is it a valid case when we keep spaming impl thread with these
> udates? Can we replace element_id !=
> scroll_offset_animation_player_->element_id() with a DCHECK?

You're right, no need to reattach.

We can't replay it with a DCHECK in case the adjustment is applied to a layer
that we're not currently scrolling.

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:9:
#include "cc/animation/animation_host.h"
On 2016/05/06 01:04:38, loyso wrote:
> You can use forward declaration here as well.

Done.

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/animation/CompositorAnimationTimeline.h
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/animation/CompositorAnimationTimeline.h:34:
std::unique_ptr<CompositorAnimationHost> m_compositorAnimationHost;
On 2016/05/06 01:04:38, loyso wrote:
> No need to store it, I think. Return it by value - CompositorAnimationHost
> compositorAnimationHost();
> That's just a wrapper.

Done

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:271:
cc::ScrollOffsetAnimationUpdate update;
On 2016/05/06 01:04:38, loyso wrote:
> You directly use cc update type here (without wrapper). Not sure if it's
> compliant with our architectural vision.
> Can you change the intarface to
> addImplOnlyScrollOffsetAnimationChange(adjustment); ? One function for one
> update type.

Ah yes. So we would still need to save the list of adjustments locally, until we
can add the update (need to be in compositing clean). Also the interface would
also have the element_id?

I'll make that change after your feedback to the comment in
https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:139:
// They should probably be reworked via LayerTreeView.
On 2016/05/06 01:04:38, loyso wrote:
> Since you send the updates from ScrollingCoordinator, it should be easy to
write
> it correctly from the beginning.
> You have a pointer to WLTV to memorize in
> layerTreeViewInitialized(WebLayerTreeView&
> layerTreeView)/willCloseLayerTreeView.

It's not clear to me what that would look like. The updates are sent from
ScrollAnimator not ScrollingCoordinator? Where would we store the list of
updates? Note that we can only actually add the update when the
DocumentLifecycleState is CompositingClean. 

Unless there is a good short-term reason to not do this, is this reasonable for
now?

[ymalik, 2016-05-06 14:15:07]

Description was changed from

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

Note that in this CL, this path is only exercised from ScrollAnchor.cpp

BUG=https://crbug.com/594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

Note that in this CL, this path is only exercised from ScrollAnchor.cpp

BUG=594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[skobes, 2016-05-07 00:25:16]

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/animation_...
cc/animation/animation_host.cc:206: // beforethe sync below because it may add
new animations that will need to
"before the"

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:114:
SetInitialValue(ScrollOffsetWithDelta(GetValue(t), adjustment));
I don't think we want to call SetInitialValue here since that will reset
total_animation_duration_.  We should just apply the adjustment directly to
initial_value_.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.cc:37: case
ScrollOffsetAnimationUpdate::Type::SCROLL_OFFSET_CHANGED: {
{ } is not typically used around case blocks.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animations.h (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.h:14: // that may impact an impl-only
scroll offset animation. PushPropertiesTo is
The comment about PushPropertiesTo would make more sense on the declaration of
ScrollOffsetAnimations.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.h:21: ScrollOffsetAnimationUpdate();
Add a ctor with params to initialize the fields?

https://codereview.chromium.org/1950243005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:30: void
addImplOnlyScrollOffsetAnimationUpdate(cc::ScrollOffsetAnimationUpdate);
Couldn't this just be called updateImplOnlyScrollOffsetAnimation?

[loyso (OOO), 2016-05-09 01:02:58]

On 2016/05/06 05:41:49, ymalik1 wrote:
> It's not clear to me what that would look like. The updates are sent from
> ScrollAnimator not ScrollingCoordinator? Where would we store the list of
> updates? Note that we can only actually add the update when the
> DocumentLifecycleState is CompositingClean. 
No need to change the place where you store the updates.
The idea is to get CompositorAnimationHost from ScrollingCoordinator (from the
Page).

So instead of:
void ScrollAnimatorCompositorCoordinator::updateCompositorAnimations() {
...
        if (CompositorAnimationTimeline* timeline =
getScrollableArea()->compositorAnimationTimeline())
           
timeline->compositorAnimationHost().addImplOnlyScrollOffsetAnimationUpdate(update);

you write:
       
getScrollableArea()->compositorAnimationHost().addImplOnlyScrollOffsetAnimationUpdate(update);

Of course, it requires plumbing and several overrides (absolutely similar to 
ScrollableArea::compositorAnimationTimeline() plumbing, except that you
init/clear your CompositorAnimationHost wrapper
 in ScrollingCoordinator::layerTreeViewInitialized / willCloseLayerTreeView,
i.e. you obtain it from WebLayerTreeView)

> Unless there is a good short-term reason to not do this, is this reasonable
for
> now?
Obtaining CompositorAnimationHost from timeline (a child) is fundamentally wrong
(as you mentioned before). It works as a hack.
Now we can see a solution, how to plumb it from ScrollingCoordinator for all
ScrollAnimators correctly.

[loyso (OOO), 2016-05-09 01:38:11]

In an ideal world, introducing CompositorAnimationHost is a huge task on its own
(a separate CL?)
The idea is to expose cc::AnimationHost API for blink properly: It should be
small and simple 1:1 wrapper with all the methods gathered in one class.
In that CL one would:
1) Rename ScrollableArea::compositorAnimationTimeline() to
ScrollableArea::compositorAnimationHost()
2) Move attachCompositorAnimationTimeline / detachCompositorAnimationTimeline
methods from WebLayerTreeView to CompositorAnimationHost (so they become non
virtual).
Of course, it requires a couple of rewrites for other callers (Document.cpp
timeline attachment and LinkHighlightsImpl.cpp timeline attachment. It should be
obvious, though).
Having CompositorAnimationHost introduced properly is a critical impact which
opens a road to extend it easily.

[ymalik, 2016-05-10 19:40:59]

@loyso, spoke to you offline and concluded that I'll add the suggested changes
to CompositorAnimationHost in a follow up CL.

PTAL :)

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:271:
cc::ScrollOffsetAnimationUpdate update;
On 2016/05/06 05:41:49, ymalik1 wrote:
> On 2016/05/06 01:04:38, loyso wrote:
> > You directly use cc update type here (without wrapper). Not sure if it's
> > compliant with our architectural vision.
> > Can you change the intarface to
> > addImplOnlyScrollOffsetAnimationChange(adjustment); ? One function for one
> > update type.
> 
> Ah yes. So we would still need to save the list of adjustments locally, until
we
> can add the update (need to be in compositing clean). Also the interface would
> also have the element_id?
> 
> I'll make that change after your feedback to the comment in
>
https://codereview.chromium.org/1950243005/diff/1/third_party/WebKit/Source/p...

Done.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/animation_...
cc/animation/animation_host.cc:206: // beforethe sync below because it may add
new animations that will need to
On 2016/05/07 00:25:16, skobes wrote:
> "before the"

Done.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:114:
SetInitialValue(ScrollOffsetWithDelta(GetValue(t), adjustment));
On 2016/05/07 00:25:16, skobes wrote:
> I don't think we want to call SetInitialValue here since that will reset
> total_animation_duration_.  We should just apply the adjustment directly to
> initial_value_.

We are starting essentially aborting the previous animation and starting a new
animation with the adjustment. The initial value of the animation needs to be
where we currently are offsetted by the adjustment. Because the distance the new
animation covers is less than that of the old animation, shouldn't the total
duration also be adjusted?

I tried it without adjusting total_animation_duration_ and it didn't seem as
smooth as with it.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.cc:37: case
ScrollOffsetAnimationUpdate::Type::SCROLL_OFFSET_CHANGED: {
On 2016/05/07 00:25:16, skobes wrote:
> { } is not typically used around case blocks.

Done.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animations.h (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.h:14: // that may impact an impl-only
scroll offset animation. PushPropertiesTo is
On 2016/05/07 00:25:16, skobes wrote:
> The comment about PushPropertiesTo would make more sense on the declaration of
> ScrollOffsetAnimations.

Done.

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.h:21: ScrollOffsetAnimationUpdate();
On 2016/05/07 00:25:16, skobes wrote:
> Add a ctor with params to initialize the fields?

Done.

https://codereview.chromium.org/1950243005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:30: void
addImplOnlyScrollOffsetAnimationUpdate(cc::ScrollOffsetAnimationUpdate);
On 2016/05/07 00:25:16, skobes wrote:
> Couldn't this just be called updateImplOnlyScrollOffsetAnimation?

Done.

[loyso (OOO), 2016-05-11 06:25:45]

Thanks ymalik for major fixes!

Sorry for one-word comments - those are nits mostly. Could you fix them? Thank
you!

Also, CompositorAnimationHostTest.cpp would be good to have.
Just one test needed so far that the pointer becomes null if you detach timeline
from WebLayerTreeViewForTesting. See the example in
CompositorAnimationTimelineTest -
CompositorTimelineDeletionDetachesFromAnimationHost.

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
File cc/animation/animation_host.cc (left):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.cc:188: // Firstly, sync all players with impl
thread to create ElementAnimations.
Firstly, secondly - it meant the constraint on the order of passes. It was a bit
unclear (bad comments), but no need to delete it at all. I propose to extract
each pass as a separate private function with a name. And the comment that
passes can't be reordered needed.

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.cc:41: else
Code style: Brackets on multiline if else blocks, AFAIR. if {} else {}

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.h:35: struct ScrollOffsetAnimationUpdate;
Is it used in this header?

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.h:83: base::TimeTicks frame_monotonic_time =
base::TimeTicks());
Can we avoid the default value and make it explicit? Any rationale apart from
modifying too many tests? :)

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.cc:32: if (!animations)
DCHECK maybe?

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:26: //
crbug.com/610763.
Probably same bug link needed on
CompositorAnimationTimeline::compositorAnimationHost method.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:29:
CompositorAnimationHost(cc::AnimationHost*);
explicit

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:33: void
updateImplOnlyScrollOffsetAnimation(const gfx::Vector2dF& adjustment, int
elementId);
cc::ElementId

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:51:
if (!m_implOnlyAnimationAdjustments.empty()) {
no brackets needed.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:257:
int elementId = getScrollableArea()->layerForScrolling()->platformLayer()->id();
cc::ElementId

[loyso (OOO), 2016-05-11 06:28:59]

PS:

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/animation/CompositorAnimationTimeline.cpp
(right):

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationTimeline.cpp:35:
return CompositorAnimationHost(m_animationTimeline->animation_host());
Please notice, no DCHECK here.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:258:
timeline->compositorAnimationHost().updateImplOnlyScrollOffsetAnimation(
compositorAnimationHost() can be nullptr inside if layerTreeView closed. Do we
need to check it explicitly and don't crash? DCHECK otherwise?

[ymalik, 2016-05-12 22:31:26]

@loyso, skobes PTAL :)

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1950243005/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:114:
SetInitialValue(ScrollOffsetWithDelta(GetValue(t), adjustment));
On 2016/05/10 19:40:59, ymalik1 wrote:
> On 2016/05/07 00:25:16, skobes wrote:
> > I don't think we want to call SetInitialValue here since that will reset
> > total_animation_duration_.  We should just apply the adjustment directly to
> > initial_value_.
> 
> We are starting essentially aborting the previous animation and starting a new
> animation with the adjustment. The initial value of the animation needs to be
> where we currently are offsetted by the adjustment. Because the distance the
new
> animation covers is less than that of the old animation, shouldn't the total
> duration also be adjusted?
> 
> I tried it without adjusting total_animation_duration_ and it didn't seem as
> smooth as with it.

Spoke to you offline. And you're right that we shouldn't change the duration. I
was missing the fact that I need to copy over the start time from the previous
animation.

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
File cc/animation/animation_host.cc (left):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.cc:188: // Firstly, sync all players with impl
thread to create ElementAnimations.
On 2016/05/11 06:25:44, loyso wrote:
> Firstly, secondly - it meant the constraint on the order of passes. It was a
bit
> unclear (bad comments), but no need to delete it at all. I propose to extract
> each pass as a separate private function with a name. And the comment that
> passes can't be reordered needed.

I only see value in extracting things out if they're used from more than one
place. Coming up with good names is difficult and subjective to the reader's
interpretation. I like to PushPropertiesTo and PushPropertiesToImplThread
convention we have for these classes, and adding more functions makes it
confusing IMO. Updated comment to make order more clear.

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.cc:41: else
On 2016/05/11 06:25:44, loyso wrote:
> Code style: Brackets on multiline if else blocks, AFAIR. if {} else {}

Done.

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.h:35: struct ScrollOffsetAnimationUpdate;
On 2016/05/11 06:25:45, loyso wrote:
> Is it used in this header?

No. Thanks!

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/animation_...
cc/animation/animation_host.h:83: base::TimeTicks frame_monotonic_time =
base::TimeTicks());
On 2016/05/11 06:25:45, loyso wrote:
> Can we avoid the default value and make it explicit? Any rationale apart from
> modifying too many tests? :)

No other rationale. But you're right, this should probably not have a default
value. Done.

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/1950243005/diff/40001/cc/animation/scroll_off...
cc/animation/scroll_offset_animations.cc:32: if (!animations)
On 2016/05/11 06:25:45, loyso wrote:
> DCHECK maybe?

Done.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:26: //
crbug.com/610763.
On 2016/05/11 06:25:45, loyso wrote:
> Probably same bug link needed on
> CompositorAnimationTimeline::compositorAnimationHost method.

Done.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:29:
CompositorAnimationHost(cc::AnimationHost*);
On 2016/05/11 06:25:45, loyso wrote:
> explicit

Done.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:33: void
updateImplOnlyScrollOffsetAnimation(const gfx::Vector2dF& adjustment, int
elementId);
On 2016/05/11 06:25:45, loyso wrote:
> cc::ElementId

Done.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:51:
if (!m_implOnlyAnimationAdjustments.empty()) {
On 2016/05/11 06:25:45, loyso wrote:
> no brackets needed.

Done.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:257:
int elementId = getScrollableArea()->layerForScrolling()->platformLayer()->id();
On 2016/05/11 06:25:45, loyso wrote:
> cc::ElementId

Done.

https://codereview.chromium.org/1950243005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:258:
timeline->compositorAnimationHost().updateImplOnlyScrollOffsetAnimation(
On 2016/05/11 06:28:58, loyso wrote:
> compositorAnimationHost() can be nullptr inside if layerTreeView closed. Do we
> need to check it explicitly and don't crash? DCHECK otherwise?

compositorAnimatiomHost() is currently returned by value and won't be null.
updateImplOnlyScrollOffsetAnimation ensures that it's pointer to
cc::AnimationHost is not null, so it shouldn't crash. But I think when we fix up
CompositorAnimationHost, this will need to be updated.

[loyso (OOO), 2016-05-13 01:14:19]

Thanks for all those fixes! Could you run trybots to see if any
dchecks/failures? Also, one major question:

https://codereview.chromium.org/1950243005/diff/60001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1950243005/diff/60001/cc/animation/animation_...
cc/animation/animation_host.cc:209: // be pushed to the impl thread.
"it may add new animations that will need to be pushed to the impl thread."
I don't understand the comment. scroll_offset_animations_->PushPropertiesTo
doesn't create anything on MT which needs to be pushed.
scroll_offset_animations_->PushPropertiesTo may create a new animation which
would be added to impl-only animation player. Could you elaborate more, why this
needs to happen before the sync below?

[loyso (OOO), 2016-05-13 01:27:10]

https://codereview.chromium.org/1950243005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.cpp
(right):

https://codereview.chromium.org/1950243005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.cpp:22:
return;
This is clearly exceptional situation when the compositor (WLTV) has been closed
but someone still send updates.

https://codereview.chromium.org/1950243005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:31:
cc::AnimationHost* animationHost() const;
It exposes cc guts. It this accessor used in testing only?

[ymalik, 2016-05-13 03:11:04]

https://codereview.chromium.org/1950243005/diff/60001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1950243005/diff/60001/cc/animation/animation_...
cc/animation/animation_host.cc:209: // be pushed to the impl thread.
On 2016/05/13 01:14:19, loyso wrote:
> "it may add new animations that will need to be pushed to the impl thread."
> I don't understand the comment. scroll_offset_animations_->PushPropertiesTo
> doesn't create anything on MT which needs to be pushed.
> scroll_offset_animations_->PushPropertiesTo may create a new animation which
> would be added to impl-only animation player. Could you elaborate more, why
this
> needs to happen before the sync below?

You're absolutely right. This doesn't need to happen before the sync! Thanks.

https://codereview.chromium.org/1950243005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/1950243005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:31:
cc::AnimationHost* animationHost() const;
On 2016/05/13 01:27:09, loyso wrote:
> It exposes cc guts. It this accessor used in testing only?

Yes. CompositorAnimationTimeline also exposes cc::AnimationTimeline, but perhaps
thats not preferred? Do you suggest getting rid of it and adding the test as a
friend for testing?

[ymalik, 2016-05-13 22:34:31]

@loyso, @skobes, PTAL :)

[skobes, 2016-05-14 00:09:20]

lgtm

[loyso (OOO), 2016-05-16 02:50:37]

LGTM with a nit.

https://codereview.chromium.org/1950243005/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:262:
for (auto& update : m_implOnlyAnimationAdjustments) {
Nit: You could call .clear() only once at the end of this function, without any
premature returns.
Surround the main action with something like
if (getScrollableArea()->layerForScrolling() && timeline &&
!timeline->compositorAnimationHost().isNull())
for example (or similar)

[ajuma, 2016-05-16 13:41:57]

https://codereview.chromium.org/1950243005/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/1950243005/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_animation.cc:907:
EXPECT_EQ(gfx::ScrollOffset(750.f, 850.f), curve->target_value());
Do you need to take current_offset_ into account here? (It doesn't seem to be
used anywhere else.)

[ymalik, 2016-05-16 18:21:37]

@ajuma, PTAL :)

https://codereview.chromium.org/1950243005/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/1950243005/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_animation.cc:907:
EXPECT_EQ(gfx::ScrollOffset(750.f, 850.f), curve->target_value());
On 2016/05/16 13:41:57, ajuma wrote:
> Do you need to take current_offset_ into account here? (It doesn't seem to be
> used anywhere else.)

Thanks! current_offset_ should have been removed based on skobes' feedback on in
PS2 scroll_offset_animation_curve.cc.

https://codereview.chromium.org/1950243005/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:262:
for (auto& update : m_implOnlyAnimationAdjustments) {
On 2016/05/16 02:50:37, loyso wrote:
> Nit: You could call .clear() only once at the end of this function, without
any
> premature returns.
> Surround the main action with something like
> if (getScrollableArea()->layerForScrolling() && timeline &&
> !timeline->compositorAnimationHost().isNull())
> for example (or similar)

Done.

[ajuma, 2016-05-16 18:38:52]

Thanks, lgtm

[ymalik, 2016-05-16 18:47:18]

ymalik@chromium.org changed reviewers:
+ jbroman@chromium.org

[ymalik, 2016-05-16 18:47:19]

+jbroman for Source/platform

[jbroman, 2016-05-16 19:15:11]

Looks like platform/animation/ was already reviewed, so I'll focus on the rest
of platform/.

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:251:
if (getScrollableArea()->layerForScrolling() && timeline &&
!timeline->compositorAnimationHost().isNull()) {
nit: This is getting a little long. Suggest factoring out the layer into a
local:

GraphicsLayer* layer = getScrollableArea()->layerForScrolling();
if (layer && timeline && ...) {
}

(This should also make the long line inside somewhat shorter.)

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:255:
host.updateImplOnlyScrollOffsetAnimation(
Just checking: I assume that it's necessary to send these all separately, rather
than as one combined offset? This is specified in the doc too, but it's not
obvious to me why. Might be worth a comment somewhere, but up to you.

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:137:
std::vector<FloatSize> m_implOnlyAnimationAdjustments;
Please avoid STL containers in Blink. I would ordinarily suggest
Vector<FloatSize> instead, but since this is a garbage-collected type, it should
probably be HeapVector<FloatSize> (with the corresponding adjustment to the
trace method).

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:539:
EXPECT_EQ(2U, scrollAnimator->m_implOnlyAnimationAdjustments.size());
Accessing the private members of an object is unfortunate. I'd rather test the
client-visible behaviour if it's not too inconvenient (failing that, a
-ForTesting accessor -- though that ship may have sailed already with
m_runState).

Why is it important that the updates be cleared once they're pushed to the host?
Is it possible to test for whatever bad behaviour that would cause, or is that
difficult?

[ymalik, 2016-05-16 20:52:35]

@jbroman, PTAL :)

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:251:
if (getScrollableArea()->layerForScrolling() && timeline &&
!timeline->compositorAnimationHost().isNull()) {
On 2016/05/16 19:15:11, jbroman wrote:
> nit: This is getting a little long. Suggest factoring out the layer into a
> local:
> 
> GraphicsLayer* layer = getScrollableArea()->layerForScrolling();
> if (layer && timeline && ...) {
> }
> 
> (This should also make the long line inside somewhat shorter.)

Done.

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:255:
host.updateImplOnlyScrollOffsetAnimation(
On 2016/05/16 19:15:11, jbroman wrote:
> Just checking: I assume that it's necessary to send these all separately,
rather
> than as one combined offset? This is specified in the doc too, but it's not
> obvious to me why. Might be worth a comment somewhere, but up to you.

You're right that the updates don't need to be sent separately. Changed this to
be a non-vector.

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:137:
std::vector<FloatSize> m_implOnlyAnimationAdjustments;
On 2016/05/16 19:15:11, jbroman wrote:
> Please avoid STL containers in Blink. I would ordinarily suggest
> Vector<FloatSize> instead, but since this is a garbage-collected type, it
should
> probably be HeapVector<FloatSize> (with the corresponding adjustment to the
> trace method).

N/A - This actually doesn't need to be a vector at all.

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1950243005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:539:
EXPECT_EQ(2U, scrollAnimator->m_implOnlyAnimationAdjustments.size());
On 2016/05/16 19:15:11, jbroman wrote:
> Accessing the private members of an object is unfortunate. I'd rather test the
> client-visible behaviour if it's not too inconvenient (failing that, a
> -ForTesting accessor -- though that ship may have sailed already with
> m_runState).
> 
> Why is it important that the updates be cleared once they're pushed to the
host?
> Is it possible to test for whatever bad behaviour that would cause, or is that
> difficult?

The correct way to test the bad behavior would require a test suite that
connects the TestScrollAnimator to the scroll animation logic in CC, which is a
lot of work.

I added a ForTesting accessor and a TODO for m_runState.

[jbroman, 2016-05-16 21:01:48]

ok, platform lgtm

https://codereview.chromium.org/1950243005/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1950243005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:139:
// A list of updates that happened on the main thread that may change
nit: please update comment (FloatSize is not a list)

[ymalik, 2016-05-16 21:30:49]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-05-16 21:30:50]

The patchset sent to the CQ was uploaded after l-g-t-m from loyso@chromium.org,
skobes@chromium.org, ajuma@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1950243005/#ps160001
(title: "nit + rebase master")

[commit-bot: I haz the power, 2016-05-16 21:31:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/160001

[commit-bot: I haz the power, 2016-05-16 22:51:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-16 22:51:43]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2016-05-17 17:35:55]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-05-17 17:36:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/160001

[commit-bot: I haz the power, 2016-05-17 20:21:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 20:21:26]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2016-05-17 21:03:33]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-05-17 21:04:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/160001

[commit-bot: I haz the power, 2016-05-17 23:54:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 23:54:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2016-05-18 20:00:24]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-05-18 20:00:25]

The patchset sent to the CQ was uploaded after l-g-t-m from loyso@chromium.org,
ajuma@chromium.org, skobes@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1950243005/#ps180001
(title: "fix bot")

[commit-bot: I haz the power, 2016-05-18 20:01:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/180001

[commit-bot: I haz the power, 2016-05-18 22:53:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-18 22:53:11]

Failed to apply patch for
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:206
  Falling back to three-way merge...
  Applied patch to 'third_party/WebKit/Source/core/layout/ScrollAnchor.cpp' with
conflicts.
  U third_party/WebKit/Source/core/layout/ScrollAnchor.cpp

Patch:       third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
Index: third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
diff --git a/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
b/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
index
85fa551dd7ba7c73289aede917ec61ca80f87c2d..acf34f2ce58535de543d32b931663fefcd97a96c
100644
--- a/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
+++ b/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
@@ -206,6 +206,7 @@ void ScrollAnchor::restore()
         ScrollAnimatorBase* animator = m_scroller->existingScrollAnimator();
         if (!animator || !animator->hasRunningAnimation()) {
             m_scroller->setScrollPosition(desiredPos, AnchoringScroll);
+           
animator->updateImplOnlyScrollOffsetAnimation(FloatSize(adjustment));
         } else {
             // If in the middle of a scroll animation, stop the animation, make
             // the adjustment, and continue the animation on the pending delta.

[commit-bot: I haz the power, 2016-05-18 22:53:25]

The CQ bit was unchecked by commit-bot@chromium.org

[ymalik, 2016-05-18 23:40:59]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-05-18 23:40:59]

The patchset sent to the CQ was uploaded after l-g-t-m from loyso@chromium.org,
ajuma@chromium.org, skobes@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1950243005/#ps200001
(title: "rebase master")

[commit-bot: I haz the power, 2016-05-18 23:41:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/200001

[commit-bot: I haz the power, 2016-05-18 23:46:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-18 23:46:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ymalik, 2016-05-19 22:09:16]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 22:10:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/220001

[commit-bot: I haz the power, 2016-05-20 00:51:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-20 00:51:43]

Dry run: This issue passed the CQ dry run.

[ymalik, 2016-05-20 01:07:34]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-05-20 01:07:35]

The patchset sent to the CQ was uploaded after l-g-t-m from loyso@chromium.org,
ajuma@chromium.org, skobes@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1950243005/#ps220001
(title: "attempt to fix rebase issue")

[commit-bot: I haz the power, 2016-05-20 01:11:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950243005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950243005/220001

[commit-bot: I haz the power, 2016-05-20 01:32:25]

Description was changed from

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

Note that in this CL, this path is only exercised from ScrollAnchor.cpp

BUG=594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate MT changes to impl-only scroll offset animations

This CL adds cc::ScrollOffsetAnimations which is the MT version of
cc::ScrollOffsetAnimationsImpl.

The flow is as follows: The scroll offset is changed on the MT (e.g. scroll
anchoring adjustment) while there is an ongoing impl-only scroll offset
animation. ScrollAnimator::updateImplOnlyAnimationWithAdjustment is called,
which pushes the update to MT's cc::AnimationHost via
CompositorAnimationTimeline::CompositorAnimationHost::AnimationHost.

During commit, cc::ScrollOffsetAnimations::PushPropertiesTo calls the
relevant handler in response to the update. Once the updates are applied,
the list of updates is cleared.

This is described in http://bit.ly/1Y6RA6y.

Note that in this CL, this path is only exercised from ScrollAnchor.cpp

BUG=594456
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f3a9e63578aff4d5c814a5bb374665a4a0812834
Cr-Commit-Position: refs/heads/master@{#394947}
==========

[commit-bot: I haz the power, 2016-05-20 01:32:27]

Patchset 12 (id:??) landed as
https://crrev.com/f3a9e63578aff4d5c814a5bb374665a4a0812834
Cr-Commit-Position: refs/heads/master@{#394947}