Can you be more specific about where the duplicate logic is occurring? cc::LayerTreeHost::BeginMainFrame is a one line function to client_->BeginMainFrame(args), and following the calls down I am unable to find duplicated logic (so far).

cc::LayerTreeHost::BeginMainFrame
-> LayerTreeHostClient::BeginMainFrame
-> RenderWidgetCompositor::BeginMainFrame
-> RenderWidget::BeginMainFrame
-> WebViewImpl::BeginFrame
-> PageWidgetDelegate::Animate
-> PageAnimator::ServiceScriptedAnimations
-> DocumentAnimations::UpdateAnimationTimingForAnimationFrame
-> DocumentTimeline::ServiceAnimations

DocumentTimeline::ServiceAnimations does the same thing as AnimateLayers (ticking animation state
for all animations, including composited ones).

1) DocumentTimeline::ServiceAnimations invokes blink::Animation::Update. This does not tick
   cc::AnimationPlayers that each blink::Animation may own.
2) cc::LayerTreeHost::AnimateLayers invokes cc::AnimationPlayer::Tick on main thread which
   actually ticks those cc::AnimationPlayer.

So the code is not redunant but the actual result i.e., updating the underlying layer property is
which I think is what #1 is referring to.

But I am not really sure why we actually need to tick cc::AnimationPlayers on main thread side at
all given that the layer is going to be updated by (1)!?

One thing to watch for is to make sure if we remove (2) we wouldn't cause redundant commits during
animations. Here is what can potentially happen (2) updates corresponding Layer::Input via
Layer::On{Property}Animated while (1) updates them through style update that eventually reach the
layer via Layer::Set{Property}. The latter is a no-op if the properties are the same otherwise it
will request a commit. So if we don't update cc::AnimationPlayers on main thread we may start
causing an unnecessary commit.

"So the code is not redundant but the actual result i.e., updating the underlying
layer property is which I think is what #1 is referring to."

Right, that is what I was referring to. cc::AnimationPlayer::Tick is never called,
but it seems there is no reason to call it because it has no effect.

In addition to the concern noted on #4, there is the possibility that we fail
to end animations at the right times. This was ajuma's concern raised in the
code review of the CL in #0 (expressed as "do we have tests for these conditions?")

My concern in #4 "removing it causing unnecessary commit" can be safely ignored. Looking more
closely at this, that possibility is prevented as part of
CompositedLayerMapping::UpdateGraphicsLayerGeometry which takes into account if there is a
active composited animation for the given property before updating it.


As for the possibility of failing to end animations on time, here are some thoughts:
 1. At the moment, notification for start/end/abort of a composited animation come from impl
    thread via a post task [2]. Ticking cc::AnimationPlayers on main side does not produce such
    notifications.
 2. There is a few different types of composited animations in Blink each with a slightly
    different lifecycle. For example blink::Animation does not seem to rely on impl notification
    to end its animation but blink::LinkHighlightImpl and blink::ScrollAnimator seem to use them
    in some capacity. I am not sure how if they have enough end-to-end tests coverage though.
    Perhaps ajuma@ can shed some light.

[1] https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/compositing/
CompositedLayerMapping.cpp?dr=Ss&l=1066

[2] See AnimationHost::SetAnimationEvents

[mac bug triage] Removing from mac triage queue.

I can own this. I will attempt to figure out if this is safe to do and will make the change.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a046fd2fec98b5e37b4186da5f50c674804a6e47

commit a046fd2fec98b5e37b4186da5f50c674804a6e47
Author: Peter Mayo <petermayo@chromium.org>
Date: Mon Jun 18 17:58:43 2018

[BlinkPropertyTrees] Shortcut AnimateLayers on main

These can cause animations to attempt to update elements which are no longer in
the property tree. It's all duplicate work to what will happen on CC and be
pushed back.

BUG=762717

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I92188f6a61b73274eb9ef1a4608585ae64e56683
Reviewed-on: https://chromium-review.googlesource.com/1091504
Commit-Queue: Peter Mayo <petermayo@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568065}
[modify] https://crrev.com/a046fd2fec98b5e37b4186da5f50c674804a6e47/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/a046fd2fec98b5e37b4186da5f50c674804a6e47/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees

A while back I look into whether removing this is safe to do.
I was mainly concerned about the dependency between
impl and main instances of an animation when it is moving from finished 
to deleted state. In particular we have logic that in some cases
we do not delete an animation model if it is waiting for a 'finished'
event [1]. So in theory if we are ever waiting for such an event
on the impl side we will end up with zombie animation models that
are never deleted.


However I have managed to convince myself that this dependency is not
an issue. In particular, only the main instance of an animation ever
waits for receiving 'finished' which is going to be produced by the
impl side even if we don't tick animations on main. See [2].

Just wanted to share in case it can be helpful.


[1] https://codesearch.chromium.org/chromium/src/cc/animation/keyframe_effect.cc?sq=package:chromium&dr=CSs&g=0&l=969
[2] https://codesearch.chromium.org/chromium/src/cc/animation/keyframe_effect.cc?sq=package:chromium&dr=CSs&g=0&l=23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/802f3a1ec6083df1b2c9d78f2f0e26fbfedd2e3e

commit 802f3a1ec6083df1b2c9d78f2f0e26fbfedd2e3e
Author: Peter Mayo <petermayo@chromium.org>
Date: Mon Jun 25 16:19:17 2018

[BlinkGenPropertyTrees] Add missed fixed test

It appears this was missed in https://chromium-review.googlesource.com/1091504
because it was listed separately.

TBR=flackr@chromium.org
BUG=762717

Change-Id: Id0fce202ec87376003b96e448c22f3e9ed7e0de2
Reviewed-on: https://chromium-review.googlesource.com/1113500
Reviewed-by: Peter Mayo <petermayo@chromium.org>
Commit-Queue: Peter Mayo <petermayo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570054}
[modify] https://crrev.com/802f3a1ec6083df1b2c9d78f2f0e26fbfedd2e3e/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees

I've discovered that we cannot simply remove LayerTreeHost::AnimateLayers, this is responsible for detecting animations which no longer affect pending elements during the commit, see analysis in  issue 888260  comment #5: https://bugs.chromium.org/p/chromium/issues/detail?id=888260#c5

We must still at least detect completed animations and mark them as such.

I put up a patch for this which with some trivial changes passes all tests: https://chromium-review.googlesource.com/c/chromium/src/+/1279193

But, I realized we may still need this for the ui compositor which allows querying the current transform / opacity of a ui::Layer (and I think does not duplicate the animation locally):
https://cs.chromium.org/chromium/src/ui/compositor/layer.h?dr=CSs&q=function:ui::Layer::transform&sq=package:chromium&g=0&l=151

There are many callers of this in ui which I believe may attempt to read the current position of composited animations.