Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1958283002:
DevTools: restore pause all that suspended all animations into the animations timeline. (Closed)
|
Created:
4 years, 7 months ago by pfeldman Modified:
4 years, 7 months 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: restore pause all that suspended all animations into the animations timeline.
BUG=608073
Committed: https://crrev.com/933e2b64178778ad63eadcf560129400c7f273d9
Cr-Commit-Position: refs/heads/master@{#392483}
Patch Set 1 : #Patch Set 2 : reflect button state after resume #Patch Set 3 : #
Total comments: 7
Patch Set 4 : review comments addressed #Messages
Total messages: 19 (7 generated)
pfeldman@chromium.org changed reviewers: + paulirish@chromium.org
Patchset #1 (id:1) has been deleted
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org
lgtm on the UI side of this. not sure about nuking _updateAnimationsPlaybackRate tho
This assumes single client :-) lgtm https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js (left): https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:263: WebInspector.AnimationModel.fromTarget(target).playbackRatePromise().then(syncPlaybackRate.bind(this)); Remove playbackRatePromise and getPlaybackRate from protocol. https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js (right): https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:131: this._pauseButton = new WebInspector.ToolbarButton(WebInspector.UIString("Pause all"), "pause-toolbar-item"); ToolbarToggle https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:209: this._pauseButton.setState(this._allPaused ? "on" : "off"); setToggled https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:211: this._pauseButton.setTitle(this._allPaused ? WebInspector.UIString("Resume all") : WebInspector.UIString("Pause all")) style: semicolon missing
https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js (right): https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:131: this._pauseButton = new WebInspector.ToolbarButton(WebInspector.UIString("Pause all"), "pause-toolbar-item"); On 2016/05/09 22:20:27, dgozman wrote: > ToolbarToggle Done. https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:209: this._pauseButton.setState(this._allPaused ? "on" : "off"); On 2016/05/09 22:20:27, dgozman wrote: > setToggled Done. https://codereview.chromium.org/1958283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:211: this._pauseButton.setTitle(this._allPaused ? WebInspector.UIString("Resume all") : WebInspector.UIString("Pause all")) On 2016/05/09 22:20:27, dgozman wrote: > style: semicolon missing Done.
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from paulirish@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1958283002/#ps80001 (title: "review comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958283002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958283002/80001
samli@chromium.org changed reviewers: + samli@chromium.org
BTW, this is broken and I'm a bit concerned about UX. * double pause button, with up to 3 pause signals which are different (global vs local), which isn't clear. * doesn't sync with scrubber I think the simplest thing to do would be to add a 0% playback rate, which will already work with the syncing of the scrubber.
On 2016/05/09 23:23:33, samli wrote: > BTW, this is broken and I'm a bit concerned about UX. > > * double pause button, with up to 3 pause signals which are different (global > vs local), which isn't clear. > * doesn't sync with scrubber > > I think the simplest thing to do would be to add a 0% playback rate, which will > already work with the syncing of the scrubber. I don't think people use scrubber much and 0% is not as intuitive as the "Pause"....
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: restore pause all that suspended all animations into the animations timeline. BUG=608073 ========== to ========== DevTools: restore pause all that suspended all animations into the animations timeline. BUG=608073 Committed: https://crrev.com/933e2b64178778ad63eadcf560129400c7f273d9 Cr-Commit-Position: refs/heads/master@{#392483} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/933e2b64178778ad63eadcf560129400c7f273d9 Cr-Commit-Position: refs/heads/master@{#392483}
Message was sent while issue was closed.
On 2016/05/09 at 23:32:58, pfeldman wrote: > On 2016/05/09 23:23:33, samli wrote: > > BTW, this is broken and I'm a bit concerned about UX. > > > > * double pause button, with up to 3 pause signals which are different (global > > vs local), which isn't clear. > > * doesn't sync with scrubber > > > > I think the simplest thing to do would be to add a 0% playback rate, which will > > already work with the syncing of the scrubber. > > I don't think people use scrubber much and 0% is not as intuitive as the "Pause".... But the animation still 'plays' when its paused..
Message was sent while issue was closed.
On 2016/05/09 at 23:32:58, pfeldman wrote: > On 2016/05/09 23:23:33, samli wrote: > > BTW, this is broken and I'm a bit concerned about UX. > > > > * double pause button, with up to 3 pause signals which are different (global > > vs local), which isn't clear. > > * doesn't sync with scrubber > > > > I think the simplest thing to do would be to add a 0% playback rate, which will > > already work with the syncing of the scrubber. > > I don't think people use scrubber much and 0% is not as intuitive as the "Pause".... But the animation still 'plays' when its paused.. |
