Bug 94222 - Web Inspector: build Elements, Resources, Timeline, Audits and Console panels lazily.
Summary: Web Inspector: build Elements, Resources, Timeline, Audits and Console panels...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-16 08:04 PDT by Pavel Feldman
Modified: 2012-08-17 02:16 PDT (History)
10 users (show)

See Also:


Attachments
[Patch] I did not run tests yet. (51.50 KB, patch)
2012-08-16 08:18 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (70.63 KB, patch)
2012-08-16 10:08 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (74.43 KB, patch)
2012-08-17 01:17 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2012-08-16 08:04:20 PDT
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.
Comment 1 Pavel Feldman 2012-08-16 08:18:51 PDT
Created attachment 158826 [details]
[Patch] I did not run tests yet.
Comment 2 Gyuyoung Kim 2012-08-16 09:09:18 PDT
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
Comment 3 Pavel Feldman 2012-08-16 10:08:34 PDT
Created attachment 158849 [details]
Patch
Comment 4 Andrey Kosyakov 2012-08-16 11:50:16 PDT
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?
Comment 5 Vsevolod Vlasov 2012-08-17 00:20:01 PDT
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.
Comment 6 Vsevolod Vlasov 2012-08-17 00:20:10 PDT
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.
Comment 7 Pavel Feldman 2012-08-17 00:24:39 PDT
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.
Comment 8 Pavel Feldman 2012-08-17 00:25:22 PDT
> 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.
Comment 9 Vsevolod Vlasov 2012-08-17 00:30:08 PDT
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
Comment 10 Pavel Feldman 2012-08-17 01:17:33 PDT
Created attachment 159041 [details]
Patch
Comment 11 Pavel Feldman 2012-08-17 02:03:28 PDT
> > 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.
Comment 12 Pavel Feldman 2012-08-17 02:09:30 PDT
Committed r125871: <http://trac.webkit.org/changeset/125871>
Comment 13 Andrey Kosyakov 2012-08-17 02:13:04 PDT
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
Comment 14 Andrey Kosyakov 2012-08-17 02:14:46 PDT
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?