Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1471193008:
Devtools Animations: Maintain playback rate on navigation (Closed)
|
Created:
5 years ago by samli Modified:
5 years ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, Eric Willigers, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, rjwright, sergeyv+blink_chromium.org, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevtools Animations: Maintain playback rate on navigation
This change allows slowdown of on-load animations and does not affect
the playback rate when devtools is closed.
BUG=447083
Committed: https://crrev.com/07835d0da6848630e58505a7e0ac4f560978d726
Cr-Commit-Position: refs/heads/master@{#361834}
Committed: https://crrev.com/4d69a570d0b2df9e37605d82126ca390b71a5300
Cr-Commit-Position: refs/heads/master@{#363697}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Messages
Total messages: 48 (15 generated)
samli@chromium.org changed reviewers: + dstockwell@chromium.org
lgtm
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471193008/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471193008/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Devtools Animations: Maintain playback rate on navigation This change allows slowdown of on-load animations and does not affect the playback rate when devtools is closed. BUG=447083 ========== to ========== Devtools Animations: Maintain playback rate on navigation This change allows slowdown of on-load animations and does not affect the playback rate when devtools is closed. BUG=447083 Committed: https://crrev.com/07835d0da6848630e58505a7e0ac4f560978d726 Cr-Commit-Position: refs/heads/master@{#361834} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/07835d0da6848630e58505a7e0ac4f560978d726 Cr-Commit-Position: refs/heads/master@{#361834}
Message was sent while issue was closed.
Why is it handled on the front-end side? Does not sound right. @dstockwell: I am not sure you should l-g-t-m inspector changes.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1477113002/ by pfeldman@chromium.org. The reason for reverting is: Reverting while we are figuring out if this is the right thing to do..
Message was sent while issue was closed.
@samli: please make sure inspector contributors are in the reviewers list of your changes.
On 2015/11/26 at 08:13:31, pfeldman wrote: > Why is it handled on the front-end side? Does not sound right. I want playback rate to be tied to the front-end lifetime. I think I've got an implementation now, but front-end seems simpler. Going to add a simple test, PTAL.
samli@chromium.org changed reviewers: + pfeldman@chromium.org - dstockwell@chromium.org
You can't implement this on the front-end due to the race. While your navigation event goes over the wire, animations might take place. Implementing it on the agent side is the only solution. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:54: if (m_state->getBoolean(AnimationAgentState::animationAgentEnabled)) { You should restore the playback rate here, not in enable. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:64: double playbackRate = m_state->getDouble(AnimationAgentState::animationAgentPlaybackRate); State should be clean on enable, this code belongs to ::restore. Restore is pretty much dudCommitLoad that happened during the cross-process navigation. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:65: if (playbackRate != 1 && playbackRate != 0) why don't you restore 1 and 0? https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:73: m_state->setBoolean(AnimationAgentState::animationAgentEnabled, false); You need to clear the rate here as well. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:82: setPlaybackRate(nullptr, 1); Nothing should happen upon front-end disconnect - you should do all the cleanup from within disable. After your agent has been disabled, it should leave no side effects.
samli@chromium.org changed reviewers: + dstockwell@chromium.org
https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:64: double playbackRate = m_state->getDouble(AnimationAgentState::animationAgentPlaybackRate); On 2015/11/27 at 03:53:30, pfeldman wrote: > State should be clean on enable, this code belongs to ::restore. Restore is pretty much dudCommitLoad that happened during the cross-process navigation. Oh, didn't realise restore() is called during the navigation. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:65: if (playbackRate != 1 && playbackRate != 0) On 2015/11/27 at 03:53:30, pfeldman wrote: > why don't you restore 1 and 0? No need to restore 1, already 1. If its not in the state yet, playback rate is 0, which it should never be. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:73: m_state->setBoolean(AnimationAgentState::animationAgentEnabled, false); On 2015/11/27 at 03:53:30, pfeldman wrote: > You need to clear the rate here as well. Done. https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:82: setPlaybackRate(nullptr, 1); On 2015/11/27 at 03:53:30, pfeldman wrote: > Nothing should happen upon front-end disconnect - you should do all the cleanup from within disable. After your agent has been disabled, it should leave no side effects. Done.
Not sure how to test the restore(). I've tried things like Page.reload, Page.navigate, but they couldn't get them to work. On 2015/11/27 at 03:53:30, pfeldman wrote: > You can't implement this on the front-end due to the race. While your navigation event goes over the wire, animations might take place. Implementing it on the agent side is the only solution. Not a critical operation to race animations. There's no dependencies here. This is a UX feature. Unlike something like network throttling, this doesn't need to be accurate. > > https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): > > https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:54: if (m_state->getBoolean(AnimationAgentState::animationAgentEnabled)) { > You should restore the playback rate here, not in enable. > > https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:64: double playbackRate = m_state->getDouble(AnimationAgentState::animationAgentPlaybackRate); > State should be clean on enable, this code belongs to ::restore. Restore is pretty much dudCommitLoad that happened during the cross-process navigation. > > https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:65: if (playbackRate != 1 && playbackRate != 0) > why don't you restore 1 and 0? > > https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:73: m_state->setBoolean(AnimationAgentState::animationAgentEnabled, false); > You need to clear the rate here as well. > > https://codereview.chromium.org/1471193008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:82: setPlaybackRate(nullptr, 1); > Nothing should happen upon front-end disconnect - you should do all the cleanup from within disable. After your agent has been disabled, it should leave no side effects.
https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:58: if (playbackRate != 1 && playbackRate != 0) So to make sure I understand, when I call setPlaybackRate(0) over the remote debugging protocol and then perform the navigation, what rate is supposed to be set?
https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:58: if (playbackRate != 1 && playbackRate != 0) On 2015/11/27 at 04:40:45, pfeldman wrote: > So to make sure I understand, when I call setPlaybackRate(0) over the remote debugging protocol and then perform the navigation, what rate is supposed to be set? 1.
On 2015/11/27 07:17:05, samli wrote: > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp > (right): > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:58: if > (playbackRate != 1 && playbackRate != 0) > On 2015/11/27 at 04:40:45, pfeldman wrote: > > So to make sure I understand, when I call setPlaybackRate(0) over the remote > debugging protocol and then perform the navigation, what rate is supposed to be > set? > > 1. I don't think that is expected!
On 2015/11/30 at 19:45:25, pfeldman wrote: > On 2015/11/27 07:17:05, samli wrote: > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp > > (right): > > > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:58: if > > (playbackRate != 1 && playbackRate != 0) > > On 2015/11/27 at 04:40:45, pfeldman wrote: > > > So to make sure I understand, when I call setPlaybackRate(0) over the remote > > debugging protocol and then perform the navigation, what rate is supposed to be > > set? > > > > 1. > > I don't think that is expected! We can remove the check if you'd like, but global paused on start is a dangerous state since animations will never start, advance or finish. Any script waiting on end events will never trigger.
On 2015/11/30 22:52:49, samli wrote: > On 2015/11/30 at 19:45:25, pfeldman wrote: > > On 2015/11/27 07:17:05, samli wrote: > > > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp > > > (right): > > > > > > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:58: if > > > (playbackRate != 1 && playbackRate != 0) > > > On 2015/11/27 at 04:40:45, pfeldman wrote: > > > > So to make sure I understand, when I call setPlaybackRate(0) over the > remote > > > debugging protocol and then perform the navigation, what rate is supposed to > be > > > set? > > > > > > 1. > > > > I don't think that is expected! > > We can remove the check if you'd like, but global paused on start is a dangerous > state since animations will never start, advance or finish. Any script waiting > on end events will never trigger. This is not me, this is protocol. If you set something via the protocol, it should be set. If animations agent is specific enough that it resets its state upon navigation, it could do this on the backend, but would need to notify the front-end about the changes.
On 2015/12/01 at 23:01:29, pfeldman wrote: > On 2015/11/30 22:52:49, samli wrote: > > On 2015/11/30 at 19:45:25, pfeldman wrote: > > > On 2015/11/27 07:17:05, samli wrote: > > > > > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp > > > > (right): > > > > > > > > > > https://codereview.chromium.org/1471193008/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:58: if > > > > (playbackRate != 1 && playbackRate != 0) > > > > On 2015/11/27 at 04:40:45, pfeldman wrote: > > > > > So to make sure I understand, when I call setPlaybackRate(0) over the > > remote > > > > debugging protocol and then perform the navigation, what rate is supposed to > > be > > > > set? > > > > > > > > 1. > > > > > > I don't think that is expected! > > > > We can remove the check if you'd like, but global paused on start is a dangerous > > state since animations will never start, advance or finish. Any script waiting > > on end events will never trigger. > > This is not me, this is protocol. If you set something via the protocol, it should be set. If animations agent is specific enough that it resets its state upon navigation, it could do this on the backend, but would need to notify the front-end about the changes. Ok, so I still thinks it makes the most sense to implement this on the front-end and I don't see why we can't. It's essentially a visual effect, the race you described between navigation/animations starting is not an issue. Can I just do it on the front-end?
> Ok, so I still thinks it makes the most sense to implement this on the front-end > and I don't see why we can't. It's essentially a visual effect, the race you > described between navigation/animations starting is not an issue. Can I just do > it on the front-end? Because it introduces a race which results in the undefined behavior. For example, I wouldn't be able to slow down the animation that happens on load in case of debugging over usb.
PTAL.
lgtm
https://codereview.chromium.org/1471193008/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/1471193008/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:73: m_state->setBoolean(AnimationAgentState::animationAgentEnabled, false); Don't land it just yet - you want to clear the state here.
The CQ bit was checked by samli@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/1471193008/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471193008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471193008/80001
https://codereview.chromium.org/1471193008/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/1471193008/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:73: m_state->setBoolean(AnimationAgentState::animationAgentEnabled, false); On 2015/12/02 at 05:25:05, pfeldman wrote: > Don't land it just yet - you want to clear the state here. Already is cleared in setPlaybackRate().
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471193008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471193008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by samli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dstockwell@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1471193008/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471193008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471193008/100001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1471193008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471193008/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Devtools Animations: Maintain playback rate on navigation This change allows slowdown of on-load animations and does not affect the playback rate when devtools is closed. BUG=447083 Committed: https://crrev.com/07835d0da6848630e58505a7e0ac4f560978d726 Cr-Commit-Position: refs/heads/master@{#361834} ========== to ========== Devtools Animations: Maintain playback rate on navigation This change allows slowdown of on-load animations and does not affect the playback rate when devtools is closed. BUG=447083 Committed: https://crrev.com/07835d0da6848630e58505a7e0ac4f560978d726 Cr-Commit-Position: refs/heads/master@{#361834} Committed: https://crrev.com/4d69a570d0b2df9e37605d82126ca390b71a5300 Cr-Commit-Position: refs/heads/master@{#363697} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4d69a570d0b2df9e37605d82126ca390b71a5300 Cr-Commit-Position: refs/heads/master@{#363697} |
