[webkit-reviews] review denied: [Bug 73086] Web Inspector: Add scripts navigator sidebar to scripts panel. : [Attachment 117247] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 73086: Web Inspector: Add scripts navigator sidebar to scripts panel.
https://bugs.webkit.org/show_bug.cgi?id=73086

Attachment 117247: Patch
https://bugs.webkit.org/attachment.cgi?id=117247&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.