This simplifies code, and gives smoother animation for free.
Created attachment 193598 [details] Patch
Comment on attachment 193598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193598&action=review > Source/WebCore/inspector/front-end/Drawer.js:-104 > - function animationFinished() Why was this function removed? > Source/WebCore/inspector/front-end/Drawer.js:130 > + this.animationFinished = function() Please make it a named function function animationFinshed() { ... } this._animationFinished = animationFinished; > Source/WebCore/inspector/front-end/Drawer.js:141 > + if (this._viewStatusBar.style.opacity == 0 || animationType == WebInspector.Drawer.AnimationType.Immediately ) What does this condition check: this._viewStatusBar.style.opacity == 0 ?
Comment on attachment 193598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193598&action=review >> Source/WebCore/inspector/front-end/Drawer.js:-104 >> - function animationFinished() > > Why was this function removed? By mistake. Restored, although everything seemed to work fine without this function. >> Source/WebCore/inspector/front-end/Drawer.js:130 >> + this.animationFinished = function() > > Please make it a named function > function animationFinshed() > { > ... > } > this._animationFinished = animationFinished; Done. >> Source/WebCore/inspector/front-end/Drawer.js:141 >> + if (this._viewStatusBar.style.opacity == 0 || animationType == WebInspector.Drawer.AnimationType.Immediately ) > > What does this condition check: this._viewStatusBar.style.opacity == 0 ? Added comments.
Created attachment 193794 [details] Patch
Comment on attachment 193794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193794&action=review > Source/WebCore/inspector/front-end/Drawer.js:114 > + if (this._viewStatusBar.style.opacity == 1 || animationType == WebInspector.Drawer.AnimationType.Immediately ) We use === everywhere. Also, an extraneous whitespace before ')' > Source/WebCore/inspector/front-end/Drawer.js:156 > + if (this._viewStatusBar.style.opacity == 0 || animationType == WebInspector.Drawer.AnimationType.Immediately ) I seem to have seen this somewhere? :) Is the diff/patch correct?
Created attachment 193802 [details] Patch
Comment on attachment 193802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193802&action=review > Source/WebCore/inspector/front-end/Drawer.js:155 > + // So we have to finish animation manually. But when does this actually happen?
Comment on attachment 193802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193802&action=review >> Source/WebCore/inspector/front-end/Drawer.js:155 >> + // So we have to finish animation manually. > > But when does this actually happen? Honestly, I can't invent situation when opacity can unsynchronize with drawer state, but if it happens, we wouldn't receive webkitTransitionEnd, and thuis wouldn't perform important cleanup. I can remove opacity check assuming that such situation will newer happen.
(In reply to comment #8) > (From update of attachment 193802 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193802&action=review > > >> Source/WebCore/inspector/front-end/Drawer.js:155 > >> + // So we have to finish animation manually. > > > > But when does this actually happen? > > Honestly, I can't invent situation when opacity can unsynchronize with drawer state, but if it happens, we wouldn't receive webkitTransitionEnd, and thuis wouldn't perform important cleanup. > > I can remove opacity check assuming that such situation will newer happen. Sounds like a good time to use console.assert :)
Created attachment 194012 [details] Patch
Comment on attachment 194012 [details] Patch Clearing flags on attachment: 194012 Committed r146326: <http://trac.webkit.org/changeset/146326>
All reviewed patches have been landed. Closing bug.