[webkit-reviews] review denied: [Bug 73086] Web Inspector: Add scripts navigator sidebar to scripts panel. : [Attachment 118607] 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 118607: Patch
https://bugs.webkit.org/attachment.cgi?id=118607&action=review

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