Bug 112586 – Web Inspector: Switch Drawer animation from JavaScript to CSS transitions.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Dmitry Zvorygin]

This simplifies code, and gives smoother animation for free.

[Dmitry Zvorygin]

Created attachment 193598 [details]
Patch

[Vsevolod Vlasov]

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 ?

[Dmitry Zvorygin]

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.

[Dmitry Zvorygin]

Created attachment 193794 [details]
Patch

[Alexander Pavlov (apavlov)]

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?

[Dmitry Zvorygin]

Created attachment 193802 [details]
Patch

[Vsevolod Vlasov]

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?

[Dmitry Zvorygin]

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.

[Vsevolod Vlasov]

(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 :)

[Dmitry Zvorygin]

Created attachment 194012 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194012 [details]
Patch

Clearing flags on attachment: 194012

Committed r146326: <http://trac.webkit.org/changeset/146326>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.