Bug 73086 – Web Inspector: Add scripts navigator sidebar to scripts panel.

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

[Vsevolod Vlasov]

Created attachment 116523 [details]
First mock

Add scripts navigator sidebar to scripts panel.

[Timothy Hatcher]

What is Snippets in the sidebar? I assume Content Scripts are from Chrome/Safari Extensions?

[Vsevolod Vlasov]

Yes, content scripts are scripts for extensions, they can be already seen in the scripts panel file select element. They are shown in the end of the select, highlighted in blue.

Snippets is the new feature I am currently working on. It should allow developer to create/evaluate/debug a set of persisted javascript snippets that are used frequently. Something similar to firebug's multiline console.

[Vsevolod Vlasov]

Note: Snippets are not going to be the part of this bug, I just added them for a mockup.

[Vsevolod Vlasov]

Created attachment 117247 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117247 [details]
Patch

Attachment 117247 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10709475

New failing tests:
inspector/debugger/scripts-panel.html
inspector/debugger/script-formatter-breakpoints.html

[Pavel Feldman]

Comment on attachment 117247 [details]
Patch

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

You should not scale the domain image.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:280
> +        this.navigator.selectTab(uiSourceCode.isContentScript ? "contentScripts" : "scripts");

Please use constants for the tab names.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:294
> +        uiSourceCode._scriptTreeElement = scriptTreeElement;

You should not bind properties to foreign objects.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:316
> +    _getOrCreateFolderTreeElement: function(isContentScript, domain, folderName)

I'd rather have it in a separate class.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:762
> +        var treeElement = uiSourceCode._scriptTreeElement;

This should be encapsulated into the navigator view.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1227
> +        var navigator = new WebInspector.TabbedPane();

Navigator is perfect candidate for extraction.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1344
> +    TreeOutline.call(this, element);

I'd put all this management logic here.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1386
> +   getScriptTreeElements: function()

no "get" prefix on getters in WebKit.

[Vsevolod Vlasov]

Created attachment 118559 [details]
Patch

[Vsevolod Vlasov]

Created attachment 118564 [details]
Patch

[Pavel Feldman]

Comment on attachment 118564 [details]
Patch

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

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:38
> +    this._panel = panel;

This dependency is not nice. You use it for a single static method and a "select" command. It is clear that scripts panel should listen to this navigator instead and that the static method should move here (or into the debugger presentation model).

[Vsevolod Vlasov]

Created attachment 118607 [details]
Patch

[Vsevolod Vlasov]

(In reply to comment #9)
> (From update of attachment 118564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118564&action=review
> 
> > Source/WebCore/inspector/front-end/ScriptsNavigator.js:38
> > +    this._panel = panel;
> 
> This dependency is not nice. You use it for a single static method and a "select" command. It is clear that scripts panel should listen to this navigator instead and that the static method should move here (or into the debugger presentation model).
Added event. Moved folderAndDisplayName method to UISourceCode. It can not be moved to navigator, since it is used in other places.

[Pavel Feldman]

Comment on attachment 118607 [details]
Patch

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

A bunch of nits, otherwise looks good.

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:40
> +    var scriptsView = new WebInspector.View();

Nit: it is almost never a good idea to create a non-specialized view. If client code is not interested in the View's events / styling, it should proceed with the DOM elements.

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:45
> +    this.appendTab(WebInspector.ScriptsNavigator.ScriptsTab, WebInspector.UIString("Scripts"), scriptsView);

(i.e. we could create this view in the tabbed pane instead. Tabbed pane is a UI component, not a view after all). No need to address in this change.

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:74
> +        var names = uiSourceCode.folderAndDisplayName();

This is going to be hard to compile. It once was a private hack in the ScriptsPanel, but now that it is official, we should create a type for it or use separate getters.

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:111
> +                    delete this._folderTreeElements[treeElement.identifier];

What is treeElement.identifier? Should it event be exposed in treeoutline.js?

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:120
> +    showScriptFoldersSettingChanged: function()

This should be a private listener that listens to the same property as ScriptsPanel. The one in the panel will vanish together with the drop box, right?

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:129
> +    reset: function()

This should also be event-driven (WebInspector.DebuggerPresentationModel.Events.DebuggerReset). You might need to emit additional DebuggerEnabled / DebuggerDisabled events there.

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:315
> +        WebInspector.NavigatorTreeOutline.prototype.appendChild.call(this, treeElement);

You should call into the same private functions from both: outline and here. In fact, i'd take it further and parametrize treeoutline with the comparator. Both resources panel and scripts would use one.

> Source/WebCore/inspector/front-end/ScriptsNavigator.js:399
> +    },

trailing coma

> Source/WebCore/inspector/front-end/ScriptsPanel.js:63
> +    const initialNavigatorWidth = 225;

You should do:
if (Preferences.useScriptNavigator) {
...
}

> Source/WebCore/inspector/front-end/ScriptsPanel.js:255
> +        this._navigator.showScriptFoldersSettingChanged();

Being event-drived would allow you not to make if (this._navigator) checks here. It would be a self-contained auxiliary component.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:265
> +            this._navigator.revealUISourceCode(uiSourceCode);

This should be done from within the sorting changed handler.

> Source/WebCore/inspector/front-end/UISourceCode.js:126
> +    folderAndDisplayName: function()

You should annotate the return type or cache intermediate results here and provide two getters (I like the latter more).

[Vsevolod Vlasov]

Created attachment 118998 [details]
Patch

[Vsevolod Vlasov]

Committed r102671: <http://trac.webkit.org/changeset/102671>