Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791513002:
Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking (Closed)
DescriptionReset is_ticking_ when calling AnimationPlayer::RemoveFromTicking
it_ticking_ isn't cleared in this path. This means that future
animations wont be started since UpdateTickingState will think that its
already ticking and wont add itself to the AnimationHost's ticking
players list.
I believe the omission in
https://crrev.com/e7592f02242026970af0a6425928e5271a52f79d was that
when the scrolling Element is unregistered the ElementAnimations will
be removed from AnimationHost. In contrast, the AnimationPlayer lives on.
That's why ElementAnimations didn't need to clear the active/ticking
flag but AnimationHost does.
BUG=704410
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2791513002
Cr-Commit-Position: refs/heads/master@{#460966}
Committed: https://chromium.googlesource.com/chromium/src/+/a064840eb26683687c53dc6fa6b5c29a2c418b5c
Patch Set 1 #
Messages
Total messages: 20 (11 generated)
Description was changed from ========== Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking it_ticking_ isn't cleared in this path. This means that future animations wont be started since UpdateTickingState will think that its already ticking and wont add itself to the AnimationHost's ticking players list. BUG=704410 ========== to ========== Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking it_ticking_ isn't cleared in this path. This means that future animations wont be started since UpdateTickingState will think that its already ticking and wont add itself to the AnimationHost's ticking players list. BUG=704410 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking it_ticking_ isn't cleared in this path. This means that future animations wont be started since UpdateTickingState will think that its already ticking and wont add itself to the AnimationHost's ticking players list. BUG=704410 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking it_ticking_ isn't cleared in this path. This means that future animations wont be started since UpdateTickingState will think that its already ticking and wont add itself to the AnimationHost's ticking players list. I believe the omission in https://crrev.com/e7592f02242026970af0a6425928e5271a52f79d was that when the scrolling Element is unregistered the ElementAnimations will be removed from AnimationHost. In contrast, the AnimationPlayer lives on. That's why ElementAnimations didn't need to clear the active/ticking flag but AnimationHost does. BUG=704410 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
rs lgtm
bokan@chromium.org changed reviewers: + aelias@chromium.org
aelias@: I narrowed it down to this. I think this may be safer compared to a the large multipatch revert in the other CL. I've asked some SYD animation folks to double check but unfortunately the only ones deeply familiar with cc/animation are ajuma@ and loyso@, both out. WDYT?
Seems legit at first glance. But if this is now targeted for 58 not 57, should we wait until Tuesday to let ajuma@ make an expert review?
No, we need a fix in M57 - though TPMs have decided to ship a fix to this issue in a second stable push. They'd like to get a fix/revert in tonights Canary which leaves about 1.5 hours to get something landed.
OK. It looks like this boolean is not used for anything else than activation signals so it seems relatively easy to understand and safe. lgtm
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1490920576907910, "parent_rev":
"473015ab5e49f5b70ed66215cc94c9af87ca91b7", "commit_rev":
"a064840eb26683687c53dc6fa6b5c29a2c418b5c"}
Message was sent while issue was closed.
Description was changed from ========== Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking it_ticking_ isn't cleared in this path. This means that future animations wont be started since UpdateTickingState will think that its already ticking and wont add itself to the AnimationHost's ticking players list. I believe the omission in https://crrev.com/e7592f02242026970af0a6425928e5271a52f79d was that when the scrolling Element is unregistered the ElementAnimations will be removed from AnimationHost. In contrast, the AnimationPlayer lives on. That's why ElementAnimations didn't need to clear the active/ticking flag but AnimationHost does. BUG=704410 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Reset is_ticking_ when calling AnimationPlayer::RemoveFromTicking it_ticking_ isn't cleared in this path. This means that future animations wont be started since UpdateTickingState will think that its already ticking and wont add itself to the AnimationHost's ticking players list. I believe the omission in https://crrev.com/e7592f02242026970af0a6425928e5271a52f79d was that when the scrolling Element is unregistered the ElementAnimations will be removed from AnimationHost. In contrast, the AnimationPlayer lives on. That's why ElementAnimations didn't need to clear the active/ticking flag but AnimationHost does. BUG=704410 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2791513002 Cr-Commit-Position: refs/heads/master@{#460966} Committed: https://chromium.googlesource.com/chromium/src/+/a064840eb26683687c53dc6fa6b5... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/a064840eb26683687c53dc6fa6b5...
Message was sent while issue was closed.
Drive-by request for followup unit test :) On Thu, Mar 30, 2017 at 8:34 PM, <aelias@chromium.org> wrote: > OK. It looks like this boolean is not used for anything else than > activation > signals so it seems relatively easy to understand and safe. lgtm > > https://codereview.chromium.org/2791513002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/03/31 14:50:36, danakj wrote: > Drive-by request for followup unit test :) Yup, we needed to get this into last night's canary cut so we had some urgency. As noted in the bug, I'll wait for ajuma@ to get back and verify this is the correct fix and I'll add a test then. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
