Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570723002:
For SPv2, move updateAnimations() to follow scrolling and paint invalidation. (Closed)
DescriptionFor SPv2, move updateAnimations() to follow scrolling and paint invalidation.
BUG=666986
Committed: https://crrev.com/d01445abcc37e8db7b6912815a3c9584a6762f93
Cr-Commit-Position: refs/heads/master@{#438971}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move updateAnimations to follow updatePaintProperties. #
Total comments: 1
Messages
Total messages: 15 (5 generated)
wkorman@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
For discussion following our in person conversation about this last week. SPv2 tests look to still pass with this change. I believe it resided where it did as it followed naturally from its previous callsite within line 2874, PaintLayerCompositor::updateIfNeededRecursive(), which ends up calling it here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... IIRC Chris had made the case to move it as he felt animation update should happen post-scrolling, but I could be mis-remembering. The compositing logic in this area of FrameView is just skipped in SPv2, so having it where it is vs. here seems like an either-or situation currently. The implementation of updateAnimations() essentially just kicks off animations timer-wise, requests a frame, and requests subsequent tick via animation timeline. I don't see that having it happen before or after scrollContentsIfNeededRecursive() is called makes a difference, but I haven't looked deeply. We'd also discussed moving scheduleAnimation() and/or scheduleNextService() from DocumentAnimations::updateAnimations() out to callsites, but on review it seems better where it is. DocumentAnimations seems like it provides higher level primitives in form of named methods that encapsulate steps needed for interacting with animation infra on a per-frame basis. That seems worthwhile, though it doesn't really need a containing class. Just a namespace looks like it would be sufficient.
https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2901: DocumentAnimations::updateAnimations(layoutView()->document()); It needs to be after updatePaintProperties, if animations will live on property tree nodes.
On 2016/12/13 at 00:13:53, chrishtr wrote: > https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:2901: DocumentAnimations::updateAnimations(layoutView()->document()); > It needs to be after updatePaintProperties, if animations will live on property tree nodes. Per conversation from yesterday, go ahead and do this or not?
On 2016/12/13 at 00:13:53, chrishtr wrote: > https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:2901: DocumentAnimations::updateAnimations(layoutView()->document()); > It needs to be after updatePaintProperties, if animations will live on property tree nodes. Per conversation from yesterday, go ahead and do this or not?
On 2016/12/15 at 19:20:20, chrishtr wrote: > On 2016/12/13 at 00:13:53, chrishtr wrote: > > https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > > > https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/frame/FrameView.cpp:2901: DocumentAnimations::updateAnimations(layoutView()->document()); > > It needs to be after updatePaintProperties, if animations will live on property tree nodes. > > Per conversation from yesterday, go ahead and do this or not? I'm writing an email right now that relates.
https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2570723002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2901: DocumentAnimations::updateAnimations(layoutView()->document()); On 2016/12/13 at 00:13:53, chrishtr wrote: > It needs to be after updatePaintProperties, if animations will live on property tree nodes. Indeed. Done. https://codereview.chromium.org/2570723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2570723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2906: DocumentAnimations::updateAnimations(layoutView()->document()); Two notes: 1. This moves some work that used to be captured within tracing for ("devtools.timeline", "UpdateLayerTree") out of that tracing block for SPv2. I think that's fine given overarching SPv2 compositing architecture change but wanted to note before we potentially land this. 2. I am not sure whether it is safe to move updateAnimations() to follow scrollContentsIfNeededRecursive(). We can try it and see unless you've existing insight. It looks like it may be ok since FrameView::scrollContentsSlowPath() looks mostly legacy as all of the is-composited checks there will be skipped in SPv2, leaving only some paint invalidation, which shouldn't affect animations; and, FrameView::frameRectsChanged() which is also therein also looks animation-safe.
lgtm
The CQ bit was checked by wkorman@chromium.org
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": 20001, "attempt_start_ts": 1481836687706210,
"parent_rev": "316363b425ffb807b5d09dab0db71997609098c7", "commit_rev":
"a113b89a3e3aba26e6d89f92bf25ab6be0e29d32"}
Message was sent while issue was closed.
Description was changed from ========== For SPv2, move updateAnimations() to follow scrolling and paint invalidation. BUG=666986 ========== to ========== For SPv2, move updateAnimations() to follow scrolling and paint invalidation. BUG=666986 Review-Url: https://codereview.chromium.org/2570723002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== For SPv2, move updateAnimations() to follow scrolling and paint invalidation. BUG=666986 Review-Url: https://codereview.chromium.org/2570723002 ========== to ========== For SPv2, move updateAnimations() to follow scrolling and paint invalidation. BUG=666986 Committed: https://crrev.com/d01445abcc37e8db7b6912815a3c9584a6762f93 Cr-Commit-Position: refs/heads/master@{#438971} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d01445abcc37e8db7b6912815a3c9584a6762f93 Cr-Commit-Position: refs/heads/master@{#438971} |
