Bug 94222 – Web Inspector: build Elements, Resources, Timeline, Audits and Console panels lazily.

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

[Pavel Feldman]

There is no need to construct these panels on inspector startup - we can do that lazily.
This change introduces the concept of PanelDescriptor that is sufficient for the panel
representation before it has been selected. It also makes selected panels build lazily.

The next step is to migrate rest of the panels and load the panel code lazily as well.
That should speed up startup time significantly.

[Pavel Feldman]

Created attachment 158826 [details]
[Patch] I did not run tests yet.

[Gyuyoung Kim]

Comment on attachment 158826 [details]
[Patch] I did not run tests yet.

Attachment 158826 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13514420

[Pavel Feldman]

Created attachment 158849 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 158849 [details]
Patch

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

Having to use string literals everywhere we used to have WebInspector.panels.panelName looks quite unfortunate -- at the very least, it's not compiler-friendly. How about using stuff like WebInspector.panelDescriptros.timeline.panel()? It's longer, but that serves right those that have to access panel directly. Or perhaps you can just keep descriptors, not panels in WebInspector.panels[]?

> Source/WebCore/inspector/front-end/DOMStorage.js:105
> +        WebInspector.panel("resources").addDOMStorage(domStorage);

use events for that?

> Source/WebCore/inspector/front-end/DOMStorage.js:113
> +        WebInspector.panel("resources").domStorageUpdated(storageId);

ditto

> Source/WebCore/inspector/front-end/Database.js:143
> +        WebInspector.panel("resources").addDatabase(database);

ditto

> Source/WebCore/inspector/front-end/ExtensionServer.js:582
> +            function ()

s/ //

> Source/WebCore/inspector/front-end/Panel.js:250
> +    this._panel = lazyInit ? null : this.panel();

if (!lazyInit)
    this.panel();
Assigning value returned by this.panel() to this._panel() is a bit confusing given we already assign this._panel in this.panel().

> Source/WebCore/inspector/front-end/ResourcesPanel.js:100
> +    if (WebInspector.resourceTreeModel.cachedResourcesLoaded())
> +        this._cachedResourcesLoaded();

So one is a function that returns a boolean and another is a notification?.. Well.. Something must be wrong about our naming conventions ;)

> Source/WebCore/inspector/front-end/Toolbar.js:79
> +        toolbarItem.panelDescriptor = panelDescriptor;

s/panelDescriptor/__panelDescriptor/? Actually, do you need it?

> Source/WebCore/inspector/front-end/inspector.js:510
> +    var panelDescriptors = this._panelDescriptors();
> +    for (var i = 0; i < panelDescriptors.length; ++i)
> +        WebInspector.inspectorView.addPanel(panelDescriptors[i]);

this._panelDescriptors().forEach(WebInspector.inspectorView.addPanel, WebInspector.inspectorView)?

> Source/WebCore/inspector/front-end/inspector.js:991
> +    if (WebInspector._showAnchorLocationInPanel(anchor, this.panels.scripts))
> +        return true;
> +    if (WebInspector._showAnchorLocationInPanel(anchor, this.panel("resources")))
> +        return true;
> +    if (WebInspector._showAnchorLocationInPanel(anchor, this.panels.network))
> +        return true;

I'd use getters for scripts and network just for consistency. Then, perhaps, one day these will be lazily created as well?

[Vsevolod Vlasov]

Comment on attachment 158826 [details]
[Patch] I did not run tests yet.

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

> Source/WebCore/inspector/front-end/InspectorView.js:65
> +        this._panelOrder.push(panelDescriptor.name());

I'd store panelDescriptors, not their names.

> Source/WebCore/inspector/front-end/inspector.js:88
> +            this._nodeSearchButton = new WebInspector.StatusBarButton(WebInspector.UIString("Select an element in the page to inspect it."), "node-search-status-bar-item");

This button should work even if Elements Panel was never opened.

[Vsevolod Vlasov]

Comment on attachment 158849 [details]
Patch

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

> Source/WebCore/inspector/front-end/Toolbar.js:85
> +                this._lastSelectedItem.removeStyleClass("toggled-on");

Isn't lastSelectedItem listening for PanelSelected event already? Seems redundant.

> Source/WebCore/inspector/front-end/Toolbar.js:89
> +            this._lastSelectedItem.addStyleClass("toggled-on");

ditto.

[Pavel Feldman]

Comment on attachment 158849 [details]
Patch

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

>> Source/WebCore/inspector/front-end/Toolbar.js:79
>> +        toolbarItem.panelDescriptor = panelDescriptor;
> 
> s/panelDescriptor/__panelDescriptor/? Actually, do you need it?

Yes, you were using it for overflow.

>> Source/WebCore/inspector/front-end/Toolbar.js:85
>> +                this._lastSelectedItem.removeStyleClass("toggled-on");
> 
> Isn't lastSelectedItem listening for PanelSelected event already? Seems redundant.

This is redundant code, forgot to remove, thanks.

[Pavel Feldman]

> I'd store panelDescriptors, not their names.

It is only used to lookup names. Placing descriptors there would kill its purpose.

> > Source/WebCore/inspector/front-end/inspector.js:88
> > +            this._nodeSearchButton = new WebInspector.StatusBarButton(WebInspector.UIString("Select an element in the page to inspect it."), "node-search-status-bar-item");
> 
> This button should work even if Elements Panel was never opened.

Good catch. Fixing.

[Vsevolod Vlasov]

Comment on attachment 158849 [details]
Patch

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

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:92
> +    cachedResourcesLoaded: function()

Please annotate

[Pavel Feldman]

Created attachment 159041 [details]
Patch

[Pavel Feldman]

> > Source/WebCore/inspector/front-end/DOMStorage.js:105
> > +        WebInspector.panel("resources").addDOMStorage(domStorage);
> 
> use events for that?

We should have a model for that, but I am afraid it is outside of the scope of this change.

> s/ //

Fixed!

> > Source/WebCore/inspector/front-end/Panel.js:250
> > +    this._panel = lazyInit ? null : this.panel();
> 
> if (!lazyInit)
>     this.panel();
> Assigning value returned by this.panel() to this._panel() is a bit confusing given we already assign this._panel in this.panel().
> 

I'm changing it in a follw-up patch;

> > Source/WebCore/inspector/front-end/ResourcesPanel.js:100
> > +    if (WebInspector.resourceTreeModel.cachedResourcesLoaded())
> > +        this._cachedResourcesLoaded();
> 
> So one is a function that returns a boolean and another is a notification?.. Well.. Something must be wrong about our naming conventions ;)

Start a thread on webkit-dev ?

> 
> > Source/WebCore/inspector/front-end/Toolbar.js:79
> > +        toolbarItem.panelDescriptor = panelDescriptor;
> 
> s/panelDescriptor/__panelDescriptor/? Actually, do you need it?
> 

> 
> this._panelDescriptors().forEach(WebInspector.inspectorView.addPanel, WebInspector.inspectorView)?
>

This is not compiler-friendly.
 
> 
> I'd use getters for scripts and network just for consistency. Then, perhaps, one day these will be lazily created as well?

The whole point is to name them differently so that we knew which ones are already migrated to the lazy loading.

[Pavel Feldman]

Committed r125871: <http://trac.webkit.org/changeset/125871>

[Andrey Kosyakov]

Comment on attachment 159041 [details]
Patch

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

> Source/WebCore/inspector/front-end/inspector.js:48
> +        var allDescriptors = [elements, resources, network, scripts, timeline, profiles, audits, console];

one

> Source/WebCore/inspector/front-end/inspector.js:58
> +        var allDescriptors = [elements, resources, network, scripts, timeline, profiles, audits, console];

two

[Andrey Kosyakov]

Comment on attachment 159041 [details]
Patch

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

> Source/WebCore/inspector/front-end/inspector.js:91
> +        if (!WebInspector.WorkerManager.isWorkerFrontend()) {
> +            this._nodeSearchButton = new WebInspector.StatusBarButton(WebInspector.UIString("Select an element in the page to inspect it."), "node-search-status-bar-item");
> +            this._nodeSearchButton.addEventListener("click", this._toggleSearchingForNode, this);
> +            mainStatusBar.insertBefore(this._nodeSearchButton.element, bottomStatusBarContainer);
> +        }

So what if elements is a hidden panel?