[webkit-reviews] review denied: [Bug 73086] Web Inspector: Add scripts navigator sidebar to scripts panel. : [Attachment 118607] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Dec 11 09:16:54 PST 2011
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).
More information about the webkit-reviews
mailing list