Bug 68155 – Web Inspector: refactor ConsoleView, Drawer, ConsolePanel trio.

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

[Pavel Feldman]

1. ConsoleView, ConsolePanel and Drawer are all Views with custom show/hide implementations and weird DOM element ownership.
2. We have a number of cases when switching console from full panel to drawer mode and back result in UI glitches.
For the sake of code clarity and no glitches, I'd like to sacrifice the "grow into full screen" console animation.

[Pavel Feldman]

Created attachment 107481 [details]
Patch

[Pavel Feldman]

Created attachment 107487 [details]
[Patch] with fixed drawer status bar style.

[Andrey Kosyakov]

Comment on attachment 107487 [details]
[Patch] with fixed drawer status bar style.

View in context: https://bugs.webkit.org/attachment.cgi?id=107487&action=review

> Source/WebCore/inspector/front-end/ConsolePanel.js:57
> +        this._view.show(this.element);

passing parent element to show() is redundant here.

> Source/WebCore/inspector/front-end/Drawer.js:173
> +    resize: function()

shouldn't this propagate resize to the child view?

> Source/WebCore/inspector/front-end/inspector.js:219
> +        this._toggleConsoleButton.toggled = !this._toggleConsoleButton.toggled;

I think the way we used to maintain console button state previously was better: this gave us somewhat better encapsulation, less logic in inspector.js and a clear assertion that the state of the button matches that of the view. Now we have this logic spread over 3 methods of inspector.js.

[Pavel Feldman]

(In reply to comment #3)
> (From update of attachment 107487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107487&action=review
> 
> > Source/WebCore/inspector/front-end/ConsolePanel.js:57
> > +        this._view.show(this.element);
> 
> passing parent element to show() is redundant here.

Done.

> 
> > Source/WebCore/inspector/front-end/Drawer.js:173
> > +    resize: function()
> 
> shouldn't this propagate resize to the child view?

Done.

> 
> > Source/WebCore/inspector/front-end/inspector.js:219
> > +        this._toggleConsoleButton.toggled = !this._toggleConsoleButton.toggled;
> 
> I think the way we used to maintain console button state previously was better: this gave us somewhat better encapsulation, less logic in inspector.js and a clear assertion that the state of the button matches that of the view. Now we have this logic spread over 3 methods of inspector.js.

ConsoleView should not host a button that toggles drawer. This button is a glue, and our glue lives in the inspector.js.

[Pavel Feldman]

Created attachment 107492 [details]
[Patch] Review comments addressed

[Andrey Kosyakov]

Comment on attachment 107492 [details]
[Patch] Review comments addressed

LGTM

[Pavel Feldman]

Created attachment 107882 [details]
[Patch] rebaselined.

[Tony Gentilcore]

Comment on attachment 107882 [details]
[Patch] rebaselined.

View in context: https://bugs.webkit.org/attachment.cgi?id=107882&action=review

> Source/WebCore/ChangeLog:10
> +        screen" console animation.

The one line description suggestions this is just a refactor, but it is actually a behavior change. Maybe there is a better one-line description?

> Source/WebCore/inspector/front-end/ConsolePanel.js:31
>      WebInspector.Panel.call(this, "console");

While you are here, why not add an /**@constructor*/ annotation to this ConsolePanel ctor. Ditto for ConsoleView below.

> Source/WebCore/inspector/front-end/Drawer.js:63
> +        this.immediatelyFinishAnimation();

Don't quite get why these are still necessary. Should the animation bits be ripped out more completely or do some animations still exist?

[Pavel Feldman]

Comment on attachment 107882 [details]
[Patch] rebaselined.

View in context: https://bugs.webkit.org/attachment.cgi?id=107882&action=review

Thanks for your review!

>> Source/WebCore/ChangeLog:10
>> +        screen" console animation.
> 
> The one line description suggestions this is just a refactor, but it is actually a behavior change. Maybe there is a better one-line description?

Done.

>> Source/WebCore/inspector/front-end/ConsolePanel.js:31
>>      WebInspector.Panel.call(this, "console");
> 
> While you are here, why not add an /**@constructor*/ annotation to this ConsolePanel ctor. Ditto for ConsoleView below.

I haven't added them to the compilation yet. I do it file by file as a part of the separate effort. Thanks for noticing though!

>> Source/WebCore/inspector/front-end/Drawer.js:63
>> +        this.immediatelyFinishAnimation();
> 
> Don't quite get why these are still necessary. Should the animation bits be ripped out more completely or do some animations still exist?

Drawer is still animating. It is just that transition between visible drawer and console panel does not animate anymore.

[Pavel Feldman]

Committed r95536: <http://trac.webkit.org/changeset/95536>

[Csaba Osztrogonác]

It broke a test on the SL and the Qt bot:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r95540%20%2833226%29/inspector/debugger/script-formatter-pretty-diff.html

Could you check it, please?

[Pavel Feldman]

Looking

[Pavel Feldman]

(In reply to comment #12)
> Looking

Fix landed as r95557. Sorry for trouble - this test was disabled on chromium :(

[Csaba Osztrogonác]

(In reply to comment #13)
> (In reply to comment #12)
> > Looking
> 
> Fix landed as r95557. Sorry for trouble - this test was disabled on chromium :(

Not problem, thanks for the quick fix.

[Yury Semikhatsky]

Comment on attachment 107882 [details]
[Patch] rebaselined.

View in context: https://bugs.webkit.org/attachment.cgi?id=107882&action=review

> Source/WebCore/inspector/front-end/ConsolePanel.js:51
> +            WebInspector.drawer.hide(true);

ConsolePanel should not directly depend on the Drawer.