Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1698913003:
Avoid style recalc during element.animate() when unnecessary (Closed)
|
Created:
4 years, 10 months ago by alancutter (OOO until 2018) Modified:
4 years, 10 months ago Reviewers:
dstockwell CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@_renameToSampledEffect Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid style recalc during element.animate() when unnecessary
BUG=587257
Committed: https://crrev.com/568ac9cd92b0b88a2b1eb2591041445323c043ee
Cr-Commit-Position: refs/heads/master@{#375875}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Fix tests #Patch Set 4 : Added test case #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (9 generated)
Description was changed from ========== Avoid style recalc during element.animate() when unnecessary BUG= ========== to ========== Avoid style recalc during element.animate() when unnecessary BUG=587257 ==========
alancutter@chromium.org changed reviewers: + dstockwell@chromium.org
Without patch: http://i.imgur.com/2OzfMG4.png With patch: http://i.imgur.com/ehzGSB6.png
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698913003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698913003/20001
Add a layout test? Should be able to use internals.updateStyleAndReturnAffectedElementCount()
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/02/17 at 02:07:48, dstockwell wrote: > Add a layout test? Should be able to use internals.updateStyleAndReturnAffectedElementCount() I'm not sure how internals.updateStyleAndReturnAffectedElementCount() would help test this. The change here is that update style doesn't get called during element.animate().
On 2016/02/17 at 05:06:05, alancutter wrote: > On 2016/02/17 at 02:07:48, dstockwell wrote: > > Add a layout test? Should be able to use internals.updateStyleAndReturnAffectedElementCount() > > I'm not sure how internals.updateStyleAndReturnAffectedElementCount() would help test this. The change here is that update style doesn't get called during element.animate(). if element.animate() updates style then a subsequent internals.updateStyleAndReturnAffectedElementCount() would return a smaller number
On 2016/02/17 at 05:06:05, alancutter wrote:
> On 2016/02/17 at 02:07:48, dstockwell wrote:
> > Add a layout test? Should be able to use
internals.updateStyleAndReturnAffectedElementCount()
>
> I'm not sure how internals.updateStyleAndReturnAffectedElementCount() would
help test this. The change here is that update style doesn't get called during
element.animate().
Unfortunately calling element.animate() always results in the element being
affected.
I attempted using
<!DOCTYPE html>
<div id="target"></div>
<script>
getComputedStyle(target).color;
target.style.color = 'blue';
target.animate(null);
console.log(internals.updateStyleAndReturnAffectedElementCount());
</script>
as a test case but it returns 1 before and after the patch.
On 2016/02/17 at 05:13:20, alancutter wrote: > On 2016/02/17 at 05:06:05, alancutter wrote: > > On 2016/02/17 at 02:07:48, dstockwell wrote: > > > Add a layout test? Should be able to use internals.updateStyleAndReturnAffectedElementCount() > > > > I'm not sure how internals.updateStyleAndReturnAffectedElementCount() would help test this. The change here is that update style doesn't get called during element.animate(). > > Unfortunately calling element.animate() always results in the element being affected. > > I attempted using > <!DOCTYPE html> > <div id="target"></div> > <script> > getComputedStyle(target).color; > target.style.color = 'blue'; > target.animate(null); > console.log(internals.updateStyleAndReturnAffectedElementCount()); > </script> > as a test case but it returns 1 before and after the patch. If you invalidate another element before calling element.animate() I think updateStyleAndReturnAffectedElementCount would return 1 before and 2 after this patch.
On 2016/02/17 at 05:22:42, dstockwell wrote: > On 2016/02/17 at 05:13:20, alancutter wrote: > > On 2016/02/17 at 05:06:05, alancutter wrote: > > > On 2016/02/17 at 02:07:48, dstockwell wrote: > > > > Add a layout test? Should be able to use internals.updateStyleAndReturnAffectedElementCount() > > > > > > I'm not sure how internals.updateStyleAndReturnAffectedElementCount() would help test this. The change here is that update style doesn't get called during element.animate(). > > > > Unfortunately calling element.animate() always results in the element being affected. > > > > I attempted using > > <!DOCTYPE html> > > <div id="target"></div> > > <script> > > getComputedStyle(target).color; > > target.style.color = 'blue'; > > target.animate(null); > > console.log(internals.updateStyleAndReturnAffectedElementCount()); > > </script> > > as a test case but it returns 1 before and after the patch. > > If you invalidate another element before calling element.animate() I think updateStyleAndReturnAffectedElementCount would return 1 before and 2 after this patch. Thanks, done.
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698913003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698913003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alancutter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dstockwell@chromium.org Link to the patchset: https://codereview.chromium.org/1698913003/#ps60001 (title: "Added test case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698913003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698913003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Avoid style recalc during element.animate() when unnecessary BUG=587257 ========== to ========== Avoid style recalc during element.animate() when unnecessary BUG=587257 Committed: https://crrev.com/568ac9cd92b0b88a2b1eb2591041445323c043ee Cr-Commit-Position: refs/heads/master@{#375875} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/568ac9cd92b0b88a2b1eb2591041445323c043ee Cr-Commit-Position: refs/heads/master@{#375875} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
