Bug 73086

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

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.

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

Created attachment 117247 [details] Patch

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

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.

Created attachment 118559 [details] Patch

Created attachment 118564 [details] Patch

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).

Created attachment 118607 [details] Patch

(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.

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).

Created attachment 118998 [details] Patch

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