Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1156953002:
Devtools: Replace split widget rAF animation with web animation (Closed)
|
Created:
5 years, 7 months ago by samli Modified:
5 years, 5 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, yurys+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevtools: Replace split widget rAF animation with web animation
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Review changes #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : Address review comments #
Total comments: 6
Patch Set 6 : #Messages
Total messages: 14 (2 generated)
samli@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:554: this._mainWidget.doResize(); It might be important to make this call. https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:559: this._mainWidget.doResize(); It is definitely important to make this call.
ptal. Latest change uses raf simply to make the mainWidget.doResize() call. I think the animation looks better and the code simpler. https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:554: this._mainWidget.doResize(); On 2015/05/26 06:01:14, pfeldman wrote: > It might be important to make this call. Done. https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:559: this._mainWidget.doResize(); On 2015/05/26 06:01:14, pfeldman wrote: > It is definitely important to make this call. In some cases, this is handled in the callback(). Even without any of these calls, the viewport resized correctly when the animation finished. Not all animate calls have a callback though.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:560: this.dispatchEventToListeners(WebInspector.SplitWidget.Events.SidebarSizeChanged, this.sidebarSize()); This one is gone. https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (right): https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:538: var player = this.contentElement.animate(keyframes, { duration: 150, easing: "cubic-bezier(0, 0, 0.2, 1)" }); Note that duration was 50 before. Accidental change?
https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:560: this.dispatchEventToListeners(WebInspector.SplitWidget.Events.SidebarSizeChanged, this.sidebarSize()); On 2015/05/26 11:46:32, dgozman wrote: > This one is gone. Done. https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (right): https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:538: var player = this.contentElement.animate(keyframes, { duration: 150, easing: "cubic-bezier(0, 0, 0.2, 1)" }); On 2015/05/26 11:46:32, dgozman wrote: > Note that duration was 50 before. Accidental change? Intentional. 100-150ms allows for user perception to see and understand the transition being made, while still being really fast compared to the 100ms window to respond to user input. 50ms means you have about 3 frames to work with, which usually means it looks janky/flickering. Especially true given that hiding a widget is quite a big state transition. The curve will still make this look really speedy but smooth. It's the standard material "leaving" easing.
https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:533: this._sidebarWidget.doResize(); This seems valuable. https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (right): https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:559: this._mainWidget.doResize(); This causes a lot of extra resizes, since |_cancelAnimation| is called from multiple places. Same goes for the event.
PTAL
Could you please explain how is this better? Code does not look much cleaner. Did you measure performance or something? https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:527: suppressUnused(this._mainElement.offsetWidth); I wonder why this is not needed anymore? https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (right): https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:517: this._mainWidget.doResize(); Looks like this call will be duplicated in animationFinished at the end.
Can we animate a transform so we don't have to layout on every frame?
On 2015/06/02 09:47:09, paulirish wrote: > Can we animate a transform so we don't have to layout on every frame? We have to layout because of things like CodeMirror or FlameChart redraw themselves when size changes.
Its hard to make a fair comparison about performance here. To me, it looks much better and is also more semantic. https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (left): https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:527: suppressUnused(this._mainElement.offsetWidth); On 2015/06/02 at 09:36:54, dgozman wrote: > I wonder why this is not needed anymore? Not sure why it would be. https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:527: suppressUnused(this._mainElement.offsetWidth); On 2015/06/02 at 09:36:54, dgozman wrote: > I wonder why this is not needed anymore? I don't know why it was needed in the first place. https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/SplitWidget.js (right): https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:517: this._mainWidget.doResize(); On 2015/06/02 at 09:36:54, dgozman wrote: > Looks like this call will be duplicated in animationFinished at the end. Fixed. https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/SplitWidget.js:517: this._mainWidget.doResize(); On 2015/06/02 at 09:36:54, dgozman wrote: > Looks like this call will be duplicated in animationFinished at the end. Done.
Closing it since I don't see why it is being done + it potentially regresses things. |
