Bug 108183 – Web Inspector: Create a container class for SidebarPane instances

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

[Vladislav Kaznacheev]

Although WebInspector.SidebarPane has been recently converted to inherit from WebInspector.View there are still some things that are wrong with it:
1. Some inheritors of WebInspector.SidebarPane do lazy initialization in some strange places (such as expand method), not in wasShown as a decent View should.
2. The client code directly inserts SidebarPane instances into DOM, then these instances change their appearance in expand/collapse methods.

There is a better way:
1. SidebarPane instances should live in a dedicated container (SidebarPaneStack).
2. SidebarPaneStack should take care of displaying pane title text, title buttons and collapse/expand behavior (calling show/detach).

This should make for a cleaner code and simplify further refactoring (replacing SidebarPaneStack with a special kind of a tabbed pane when the sidebar width > height).

[Vladislav Kaznacheev]

Created attachment 185770 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 185772 [details]
Patch

[Pavel Feldman]

Comment on attachment 185772 [details]
Patch

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

Overall looks good. We'll need to go through the sidebars to check whether it regressed anything UI-wise.

> Source/WebCore/ChangeLog:9
> +        SidebarPane's are inserted into DOM lazily and can belong to more than one container.

SidebarPanes are...

> Source/WebCore/inspector/front-end/ElementsPanel.js:89
> +    this.sidebarPanes.styles.setShowCallback(this.updateStyles.bind(this));

Might be a good time to introduce WebInspector.View.Events.WasShown event. Could be a part of separate change though.

> Source/WebCore/inspector/front-end/SidebarPane.js:38
> +//    this.titleElement = document.createElement("div");

Remove this line.

> Source/WebCore/inspector/front-end/SidebarPane.js:39
> +    this.titleElement = document.createDocumentFragment();

this._auxTitleFragment. Btw, what is the point of using fragment here?

> Source/WebCore/inspector/front-end/SidebarPane.js:43
> +    this._expanded = false;

It is weird that we still have this._expanded.

> Source/WebCore/inspector/front-end/SidebarPane.js:91
> +    setExpandCallback: function(callback)

Please annotate these

> Source/WebCore/inspector/front-end/SidebarPane.js:132
> +    addPane: function(pane)

Here and below: please annotate.

> Source/WebCore/inspector/front-end/inspector.css:1788
> +.pane-title + .pane-title, .pane:not(.visible) + .pane-title, .pane-title:first-of-type {

We'll need to test this thoroughly since there is a lot of minor features associated with the titles.

[Vladislav Kaznacheev]

Comment on attachment 185772 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        SidebarPane's are inserted into DOM lazily and can belong to more than one container.
> 
> SidebarPanes are...

Done

>> Source/WebCore/inspector/front-end/ElementsPanel.js:89
>> +    this.sidebarPanes.styles.setShowCallback(this.updateStyles.bind(this));
> 
> Might be a good time to introduce WebInspector.View.Events.WasShown event. Could be a part of separate change though.

Will file a bug for that.

>> Source/WebCore/inspector/front-end/SidebarPane.js:39
>> +    this.titleElement = document.createDocumentFragment();
> 
> this._auxTitleFragment. Btw, what is the point of using fragment here?

Removed the comment. The fragment is used here because the 5 of the SidebarPane inheritors actively use this.titleElement.appendChild and I did not want to change their code.

>> Source/WebCore/inspector/front-end/SidebarPane.js:43
>> +    this._expanded = false;
> 
> It is weird that we still have this._expanded.

Got rid of _expanded. The expand state is now stored only in SidebarPaneStack

>> Source/WebCore/inspector/front-end/SidebarPane.js:91
>> +    setExpandCallback: function(callback)
> 
> Please annotate these

Done

>> Source/WebCore/inspector/front-end/SidebarPane.js:132
>> +    addPane: function(pane)
> 
> Here and below: please annotate.

Done

>> Source/WebCore/inspector/front-end/inspector.css:1788
>> +.pane-title + .pane-title, .pane:not(.visible) + .pane-title, .pane-title:first-of-type {
> 
> We'll need to test this thoroughly since there is a lot of minor features associated with the titles.

Oh, I have and I will again.

[Vladislav Kaznacheev]

Created attachment 186021 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 186041 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 186050 [details]
Patch

[Build Bot]

Comment on attachment 186050 [details]
Patch

Attachment 186050 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16297939

New failing tests:
inspector/extensions/extensions-events.html

[Build Bot]

Comment on attachment 186050 [details]
Patch

Attachment 186050 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16293938

New failing tests:
inspector/extensions/extensions-events.html

[Pavel Feldman]

Comment on attachment 186050 [details]
Patch

Please resolve tests issues prior to landing.

[Vladislav Kaznacheev]

Created attachment 186388 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186388 [details]
Patch

Clearing flags on attachment: 186388

Committed r141777: <http://trac.webkit.org/changeset/141777>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.