Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527623002:
add has_element_in_any_list check in ElementAnimations::UpdateActivation (Closed)
|
Created:
4 years, 1 month ago by junchao.han Modified:
4 years ago CC:
chromium-reviews, cc-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd has_element_in_any_list check in ElementAnimations::UpdateActivation
add has_element_in_any_list check in ElementAnimations::UpdateActivation during
PushProperities process. This solves a corner case if dom for animation be
removed before animation starts. See bug for details.
BUG=668086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/7e190bb23bc08d98e305c4e2b367acf932a2d800
Cr-Commit-Position: refs/heads/master@{#434862}
Patch Set 1 #Patch Set 2 : Deactivate ElementAnimations from AnimationHost if no element in any list #
Total comments: 6
Patch Set 3 : move has_element_in_any_list check to ::UpdateActivation #
Total comments: 2
Patch Set 4 : update check condition #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Deactivate ElementAnimations from AnimationHost if no element in any list Deactivate from animation_host_ if there is no element in any list in function ElementAnimations::Animation. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 ========== to ========== Deactivate ElementAnimations from AnimationHost if no element in any list Deactivate from animation_host_ if there is no element in any list in function ElementAnimations::Animation. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Deactivate ElementAnimations from AnimationHost if no element in any list Deactivate from animation_host_ if there is no element in any list in function ElementAnimations::Animation. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Deactivate ElementAnimations from AnimationHost if no element in any list Deactivate from animation_host_ if there is no element in any list in function ElementAnimations::Animation. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
junchao.han@intel.com changed reviewers: + ajuma@chromium.org, loyso@chromium.org, vollick@chromium.org
On 2016/11/23 11:09:29, junchao.han wrote: > mailto:junchao.han@intel.com changed reviewers: > + mailto:ajuma@chromium.org, mailto:loyso@chromium.org, mailto:vollick@chromium.org PTAL
unit test, please?
On 2016/11/24 03:24:38, loyso wrote: > unit test, please? sure. I will add a unit test case based on Octane2 scenario.
On 2016/11/24 07:53:19, junchao.han wrote: > On 2016/11/24 03:24:38, loyso wrote: > > unit test, please? > > sure. I will add a unit test case based on Octane2 scenario. unit test added. PTAL
Nice bug! Thanks for working on this! https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... File cc/animation/element_animations.cc (right): https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... cc/animation/element_animations.cc:163: animation_host_->DidDeactivateElementAnimations(this); if you call host_->UnregisterElement, ElementAnimations::ElementUnregistered should be called. I belive, DidDeactivate is called in ElementAnimations::ElementUnregistered for the main thread. For the impl thread: On PushProperties, ElementAnimations::UpdateActivation is called. It looks like we need to fix ElementAnimations::UpdateActivation (add the has_element_in_any_list() check there). ::UpdateActivation is the actual command, ::Animate is a regular general tick. What do you think?
https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... File cc/animation/element_animations_unittest.cc (right): https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... cc/animation/element_animations_unittest.cc:3651: EXPECT_EQ(1u, host_impl_->active_element_animations_for_testing().size()); This should be 0 since we don't have any impl-side layers, I think.
https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... File cc/animation/element_animations_unittest.cc (right): https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... cc/animation/element_animations_unittest.cc:3646: host_->UnregisterElement(element_id_, ElementListType::ACTIVE); Please, introduce void AnimationTimelinesTest::DestroyTestMainLayer(){ client_.UnregisterElement(element_id_, ElementListType::ACTIVE); } next to void AnimationTimelinesTest::CreateTestMainLayer() to unregister main-thread test layer and mimic real-world scenario. (And call it here). We need to unregister that TestLayer for real.
https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... File cc/animation/element_animations.cc (right): https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... cc/animation/element_animations.cc:163: animation_host_->DidDeactivateElementAnimations(this); On 2016/11/27 23:36:55, loyso wrote: > if you call host_->UnregisterElement, ElementAnimations::ElementUnregistered > should be called. I belive, DidDeactivate is called in > ElementAnimations::ElementUnregistered for the main thread. > > For the impl thread: On PushProperties, ElementAnimations::UpdateActivation is > called. > > It looks like we need to fix ElementAnimations::UpdateActivation (add the > has_element_in_any_list() check there). ::UpdateActivation is the actual > command, ::Animate is a regular general tick. > > What do you think? Thanks for the comments. Not DidActivateElementAnimations in PushProperties is a better way than DidDeactivateElementAnimations in the general tick. A new patch has been upload and PTAL. The new patch checks has_element_in_any_list in UpdateActivation. However this patch will cause following two cc_unittests failed. I will take a look. LayerTreeHostAnimationTestAnimatedLayerRemovedAndAdded.RunMultiThread_DelegatingRenderer LayerTreeHostAnimationTestAnimatedLayerRemovedAndAdded.RunSingleThread_DelegatingRenderer https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... File cc/animation/element_animations_unittest.cc (right): https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... cc/animation/element_animations_unittest.cc:3646: host_->UnregisterElement(element_id_, ElementListType::ACTIVE); On 2016/11/28 00:05:53, loyso wrote: > Please, introduce > void AnimationTimelinesTest::DestroyTestMainLayer(){ > client_.UnregisterElement(element_id_, ElementListType::ACTIVE); > } > > next to > void AnimationTimelinesTest::CreateTestMainLayer() > to unregister main-thread test layer and mimic real-world scenario. > (And call it here). > > We need to unregister that TestLayer for real. Done. https://codereview.chromium.org/2527623002/diff/20001/cc/animation/element_an... cc/animation/element_animations_unittest.cc:3651: EXPECT_EQ(1u, host_impl_->active_element_animations_for_testing().size()); On 2016/11/27 23:38:53, loyso wrote: > This should be 0 since we don't have any impl-side layers, I think. Done.
Cool! Looks very good. Wondering about those tests. They needs some tweaks, maybe. https://codereview.chromium.org/2527623002/diff/40001/cc/animation/element_an... File cc/animation/element_animations.cc (right): https://codereview.chromium.org/2527623002/diff/40001/cc/animation/element_an... cc/animation/element_animations.cc:376: if (is_active_ && (!was_active || force) && has_element_in_any_list()) { Sohuldn't it be simple with a semantics similar to: if (!has_element_in_any_list()) { animation_host_->DidDeactivateElementAnimations(this); return; } at the beginning of the function?
Description was changed from ========== Deactivate ElementAnimations from AnimationHost if no element in any list Deactivate from animation_host_ if there is no element in any list in function ElementAnimations::Animation. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== add has_element_in_any_list check in ElementAnimations::UpdateActivation add has_element_in_any_list check in ElementAnimations::UpdateActivation during PushProperities process. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2527623002/diff/40001/cc/animation/element_an... File cc/animation/element_animations.cc (right): https://codereview.chromium.org/2527623002/diff/40001/cc/animation/element_an... cc/animation/element_animations.cc:376: if (is_active_ && (!was_active || force) && has_element_in_any_list()) { On 2016/11/28 04:27:04, loyso wrote: > Sohuldn't it be simple with a semantics similar to: > if (!has_element_in_any_list()) { > animation_host_->DidDeactivateElementAnimations(this); return; } > at the beginning of the function? for 2 unit tests failed in patch set 3, UpdateActivation(ActivationType::FORCE) is called. So the corner case only apply to (is_active_ && !was_active && has_element_in_any_list()). A new patch has been uploaded. PTAL
On 2016/11/28 07:27:45, junchao.han wrote: > https://codereview.chromium.org/2527623002/diff/40001/cc/animation/element_an... > File cc/animation/element_animations.cc (right): > > https://codereview.chromium.org/2527623002/diff/40001/cc/animation/element_an... > cc/animation/element_animations.cc:376: if (is_active_ && (!was_active || force) > && has_element_in_any_list()) { > On 2016/11/28 04:27:04, loyso wrote: > > Sohuldn't it be simple with a semantics similar to: > > if (!has_element_in_any_list()) { > > animation_host_->DidDeactivateElementAnimations(this); return; } > > at the beginning of the function? > > for 2 unit tests failed in patch set 3, UpdateActivation(ActivationType::FORCE) > is called. So the corner case only apply to (is_active_ && !was_active && > has_element_in_any_list()). A new patch has been uploaded. PTAL All cc_unitttests including new added one have been passed.
The CQ bit was checked by loyso@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by junchao.han@intel.com
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": 60001, "attempt_start_ts": 1480388301203110,
"parent_rev": "ecbdb70dceb0966ffa6b76191895508ae04fb3c2", "commit_rev":
"9f6d61d8f05b8b0062b4e27152e44786ea91b7e7"}
Message was sent while issue was closed.
Description was changed from ========== add has_element_in_any_list check in ElementAnimations::UpdateActivation add has_element_in_any_list check in ElementAnimations::UpdateActivation during PushProperities process. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== add has_element_in_any_list check in ElementAnimations::UpdateActivation add has_element_in_any_list check in ElementAnimations::UpdateActivation during PushProperities process. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== add has_element_in_any_list check in ElementAnimations::UpdateActivation add has_element_in_any_list check in ElementAnimations::UpdateActivation during PushProperities process. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== add has_element_in_any_list check in ElementAnimations::UpdateActivation add has_element_in_any_list check in ElementAnimations::UpdateActivation during PushProperities process. This solves a corner case if dom for animation be removed before animation starts. See bug for details. BUG=668086 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/7e190bb23bc08d98e305c4e2b367acf932a2d800 Cr-Commit-Position: refs/heads/master@{#434862} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7e190bb23bc08d98e305c4e2b367acf932a2d800 Cr-Commit-Position: refs/heads/master@{#434862} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
