New issue
Advanced search Search tips

Issue 764405 link

Starred by 1 user
Status: Archived
Owner:
smcgruer@chromium.org
Closed: Apr 2018
Cc:
majidvp@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Task



Sign in to add a comment

cc/animation - cleanup needs_push_properties/SetNeedsPushProperties logic

Project Member Reported by smcgruer@chromium.org, Sep 12 2017 
There are various bits of available cleanup here:

1. We double-check needs_push_properties() in the PushPropertiesTo chain, we can remove the check before calling and get rid of the needs_push_properties() method (or keep it around only for tests).

2. When a particular class' SetNeedsPushProperties method informs a parent in the tree, it currently does that every time. We can change it so that we only inform the parent when needs_push_properties_ actually changes from false to true.

Comment 1 by smcgruer@chromium.org, Oct 10 2017 

It turns out that after other refactorings #2 is no longer relevant. AnimationTimeline::SetNeedsPushProperties is the only place that still has such a situation, and it is actually necessary as the underlying AnimationHost may change.

#1 is being worked on in https://chromium-review.googlesource.com/c/chromium/src/+/663415, though it has already reduced in scope due to other refactorings.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 10 2017 

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

commit ed314e20b76c4c65f1a6a85a86b1ef3b1cc32815
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Tue Oct 10 23:06:22 2017

Remove useless check of needs_push_properties() in AnimationHost

Both AnimationTimeline::PushPropertiesTo and
ElementAnimations::PushPropertiesTo already guard against their own
flags internally. The implementation of the methods were left in place
as multiple tests use the method to ensure that we are correctly
handling needs_push_properties_.

Bug:  764405 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iffa778ad14c8a11752b1a3c0a075da46e9fc38f0
Reviewed-on: https://chromium-review.googlesource.com/663415
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507815}
[modify] https://crrev.com/ed314e20b76c4c65f1a6a85a86b1ef3b1cc32815/cc/animation/animation_host.cc

Comment 3 by smcgruer@chromium.org, Apr 25 2018

Status: Archived (was: Assigned)

Comment 4 by smcgruer@chromium.org, Apr 25 2018 

(Archived because I think this is fixed, but I'm not 100% sure and it's really not that important.)
►

Sign in to add a comment