Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(364)

Issue 73643004: Web Animations: Extract an API for servicing animations (Closed)

Created:
7 years, 1 month ago by dstockwell
Modified:
7 years, 1 month ago
CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), zoltan1, eae+blinkwatch, dglazkov+blink, leviw+renderwatch, blink-layers+watch_chromium.org, dstockwell, Timothy Loh, apavlov+blink_chromium.org, adamk+blink_chromium.org, jchaffraix+rendering, darktears, bemjb+rendering_chromium.org, Steve Block, dino_apple.com, Eric Willigers
Visibility:
Public.

Description

Web Animations: Extract an API for servicing animations This is mostly a code shuffle making animation servicing less intrusive on the rest of the system. Also: * If necessary, waits for a compositing update before starting animations on the compositor (otherwise the appropriate composited layers might not have been created). * Forces a layout (and compositing update) after servicing frame callbacks when running against the test proxy. * Addresses concerns raised in r161892 about adding too much logic to Document. * Avoids checking whether there is an animation that could benefit from compositing in the CSSAnimationUpdate as we know that the update will have been applied by the time the compositor update takes place. Note: These changes are guarded by the WebAnimationsCSS runtime flag. BUG=298211, 321460 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162401

Patch Set 1 : #

Patch Set 2 : Avoid timing out in layout tests waiting for compositing update. #

Patch Set 3 : --no-find-copies #

Patch Set 4 : Switch to references for non-null params. #

Total comments: 12

Patch Set 5 : Address comments. Move after-composite callback to PageWidgetDelegate. #

Patch Set 6 : Remove unnecessary frame request. #

Total comments: 6

Patch Set 7 : Address comments and update expectations. #

Patch Set 8 : Fix nit. #

Patch Set 9 : Fix build. #

Patch Set 10 : Rebase and avoid killing inspector/layer-compositing-reasons. #

Patch Set 11 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+242 lines, -103 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/inspector/layer-compositing-reasons.html View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/animation/ActiveAnimations.cpp View 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/animation/Animation.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/animation/Animation.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -2 lines 0 comments Download
A Source/core/animation/DocumentAnimations.h View 1 2 3 1 chunk +55 lines, -0 lines 0 comments Download
A Source/core/animation/DocumentAnimations.cpp View 1 2 3 4 1 chunk +113 lines, -0 lines 0 comments Download
M Source/core/animation/css/CSSAnimations.h View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/animation/css/CSSAnimations.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +2 lines, -29 lines 0 comments Download
M Source/core/animation/css/CSSPendingAnimations.h View 1 2 3 4 1 chunk +4 lines, -3 lines 0 comments Download
M Source/core/animation/css/CSSPendingAnimations.cpp View 1 2 3 4 5 6 7 8 1 chunk +42 lines, -18 lines 0 comments Download
M Source/core/core.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 2 chunks +2 lines, -10 lines 0 comments Download
M Source/core/dom/Document.h View 1 chunk +0 lines, -3 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +2 lines, -24 lines 0 comments Download
M Source/core/frame/FrameView.cpp View 1 2 3 2 chunks +2 lines, -7 lines 0 comments Download
M Source/core/rendering/RenderLayerCompositor.cpp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/testing/runner/WebTestProxy.cpp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/PageWidgetDelegate.cpp View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
dstockwell
7 years, 1 month ago (2013-11-19 23:09:49 UTC) #1
Steve Block
https://codereview.chromium.org/73643004/diff/110001/Source/core/animation/ActiveAnimations.cpp File Source/core/animation/ActiveAnimations.cpp (left): https://codereview.chromium.org/73643004/diff/110001/Source/core/animation/ActiveAnimations.cpp#oldcode52 Source/core/animation/ActiveAnimations.cpp:52: return shouldCompositeForActiveAnimation || activeAnimations->cssAnimations().shouldCompositeForPendingAnimations(renderViewInCompositingMode); I don't understand why we ...
7 years, 1 month ago (2013-11-20 00:36:15 UTC) #2
dstockwell
https://codereview.chromium.org/73643004/diff/110001/Source/core/animation/ActiveAnimations.cpp File Source/core/animation/ActiveAnimations.cpp (left): https://codereview.chromium.org/73643004/diff/110001/Source/core/animation/ActiveAnimations.cpp#oldcode52 Source/core/animation/ActiveAnimations.cpp:52: return shouldCompositeForActiveAnimation || activeAnimations->cssAnimations().shouldCompositeForPendingAnimations(renderViewInCompositingMode); On 2013/11/20 00:36:16, Steve Block ...
7 years, 1 month ago (2013-11-20 00:49:15 UTC) #3
dstockwell
PTAL, made this robust to running under content_shell --dump-render-tree
7 years, 1 month ago (2013-11-20 03:00:20 UTC) #4
shans
LGTMy word that's a lot of code you're removing https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp File Source/core/animation/css/CSSPendingAnimations.cpp (right): https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp#newcode64 Source/core/animation/css/CSSPendingAnimations.cpp:64: ...
7 years, 1 month ago (2013-11-20 03:51:31 UTC) #5
dstockwell
https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp File Source/core/animation/css/CSSPendingAnimations.cpp (right): https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp#newcode64 Source/core/animation/css/CSSPendingAnimations.cpp:64: player->setStartTime(player->timeline().currentTime()); On 2013/11/20 03:51:31, shans wrote: > setStartTime(m_pending[i].second); Done. ...
7 years, 1 month ago (2013-11-20 04:42:30 UTC) #6
Steve Block
lgtm https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp File Source/core/animation/css/CSSPendingAnimations.cpp (right): https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp#newcode58 Source/core/animation/css/CSSPendingAnimations.cpp:58: } No need for braces
7 years, 1 month ago (2013-11-20 04:42:51 UTC) #7
dstockwell
https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp File Source/core/animation/css/CSSPendingAnimations.cpp (right): https://codereview.chromium.org/73643004/diff/190001/Source/core/animation/css/CSSPendingAnimations.cpp#newcode58 Source/core/animation/css/CSSPendingAnimations.cpp:58: } On 2013/11/20 04:42:52, Steve Block wrote: > No ...
7 years, 1 month ago (2013-11-20 04:47:08 UTC) #8
dstockwell
+jochen for Source/web
7 years, 1 month ago (2013-11-20 05:04:33 UTC) #9
abarth-chromium
On 2013/11/20 05:04:33, dstockwell wrote: > +jochen for Source/web rsLGTM for the one-line change to ...
7 years, 1 month ago (2013-11-20 05:35:41 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dstockwell@chromium.org/73643004/270002
7 years, 1 month ago (2013-11-20 05:48:55 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dstockwell@chromium.org/73643004/560001
7 years, 1 month ago (2013-11-20 19:43:49 UTC) #12
commit-bot: I haz the power
7 years, 1 month ago (2013-11-20 20:54:56 UTC) #13
Message was sent while issue was closed.
Change committed as 162401

Powered by Google App Engine
This is Rietveld 408576698