New issue
Advanced search Search tips

Issue 860341 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Allow only one keyframe model per target property in cc::KeyframeEffect

Project Member Reported by yigu@chromium.org, Jul 4

Issue description

Currently in cc a keyframe effect may have the following keyframe model sequence: opacity, transform, opacity; but this is never true in reality. Adding 2 opacity animations for the same target should be done with 2 separate animations.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 6

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

commit 5be95125dc971bd30eb0e074b46549532ef0318b
Author: Yi Gu <yigu@chromium.org>
Date: Fri Jul 06 22:35:36 2018

[cc] Reorder impl side keyframe model add / remove logic

Currently we push new keyframe models on impl followed by removing any
completed ones. This is to make sure that we don't push the keyframe
models twice because if we do it the other way around, a finished main
thread keyframe model will push itself to impl if its impl side
counterpart got removed prior to the push.

Actually, for finished keyframe models there is no need to copy them
over to impl as we remove them right after the push. Besides, adding
new keyframe models then removing them violates the principle that a
keyframe effect should have only one keyframe model per property.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I50dc93c5c358b0d1711a3d5561a96ccd46698a3b
Bug:  860341 
Reviewed-on: https://chromium-review.googlesource.com/1127451
Commit-Queue: Yi Gu <yigu@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573095}
[modify] https://crrev.com/5be95125dc971bd30eb0e074b46549532ef0318b/cc/animation/element_animations_unittest.cc
[modify] https://crrev.com/5be95125dc971bd30eb0e074b46549532ef0318b/cc/animation/keyframe_effect.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 9

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

commit 248b42203b256d2b0e2d310c5dae115a06d72fdd
Author: Yi Gu <yigu@chromium.org>
Date: Mon Jul 09 23:01:48 2018

Add DCHECK to make sure that keyframe models in the same group don't animate the same property

Currently in cc a keyframe effect is allowed to have the following
keyframe model sequence: opacity, transform, opacity; but this is never
true in reality if they are in the same group, i.e., start together.
This patch is to forbid such behavior and update tests accordingly.

Bug:  860341 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I39e7765faf910ff4e9385b18b796baa394d0e660
Reviewed-on: https://chromium-review.googlesource.com/1129200
Reviewed-by: Ali Juma <ajuma@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573504}
[modify] https://crrev.com/248b42203b256d2b0e2d310c5dae115a06d72fdd/cc/animation/element_animations_unittest.cc
[modify] https://crrev.com/248b42203b256d2b0e2d310c5dae115a06d72fdd/cc/animation/keyframe_effect.cc

Status: Fixed (was: Started)

Sign in to add a comment