* SUMMARY Make it possible to debug injected scripts when the Debug UI is enabled.
Created attachment 267660 [details] [Patch] Proposed Fix
Comment on attachment 267660 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267660&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:67 > + this._excludedScripts = []; This should be renamed _inspectorDebugScripts, since they aren't initially excluded if the Inspector loads in debug UI mode.
Comment on attachment 267660 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267660&action=review r- for the open questions, but this patch does look good. One more iteration and this should be good! > Source/WebInspectorUI/UserInterface/Base/Main.js:447 > + if (WebInspector.showDebugUISetting) { > + WebInspector.showDebugUISetting.addEventListener(WebInspector.Setting.Event.Changed, () => { > + this.notifications.dispatchEventToListeners(WebInspector.Notification.DebugUIEnabledDidChange); > + }); > + } > + } Why not just have Debug/Bootstrap.js do this? Seems weird to have Main.js do it. > Source/WebInspectorUI/UserInterface/Base/Main.js:1405 > +WebInspector.isInternalScript = function(url) > +{ > + if (!url) > + return false; > + > + return url === "__WebInspectorInternal__"; > +} Nit: Early return here won't actually matter. > Source/WebInspectorUI/UserInterface/Base/Object.js:212 > + DebugUIEnabledDidChange: "debug-ui-enabled-did-change" Nit: Add the trailing comma, it makes future diffs nicer. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:67 > + this._excludedScripts = []; This name can be improved, right now it doesn't indicate that it is purely for debug UI purposes. How about: this._excludedDebugScripts or even better this._debugScripts, since they may not be excluded. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-491 > - // Exclude the case where the call frame is in the inspector code. > - if (sourceCodeLocation.sourceCode.url && sourceCodeLocation.sourceCode.url.startsWith("__WebInspector")) > - continue; I don't think this is right. Debug Scripts are always included in DebuggerManager's script maps, even in Non-Debug mode. So we should get a sourceCodeLocation with a URL that startsWith "__WebInspector". Seems we might still want this code but with a WebInspector.isDebugUIEnabled() check. To test, just type this into the console: js> function foo() { debugger; } foo() > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:534 > + if (WebInspector.isInternalScript(script.url)) > + return; This can be done above the construction of the WebInspector.Script, and could save us a bunch of unnecessary work! > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:514 > - if (!sourceCode || sourceCode.url.startsWith("__WebInspector")) { > + if (!sourceCode) { This still seems relevant. In Non-Debug UI we probably do not want to link users into InjectedScriptSource code. Would that even work!? To test this, you could add a log into InjectedScriptSource.js (inspectedGlobalObject.console.log("test")) and ensure in Non-Debug it doesn't have a location link, but in Debug it does. Seems we would again want a WebInspector.isDebugUIEnabled check here. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:423 > + console.assert(scriptTreeElement.parent); I don't think this assert wins us anything. If it fails the next line will give us an exception with the same information. > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:317 > + scriptTreeElement.parent.removeChild(scriptTreeElement); I don't think this assert wins us anything. If it fails the next line will give us an exception with the same information.
Comment on attachment 267660 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267660&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-491 >> - continue; > > I don't think this is right. Debug Scripts are always included in DebuggerManager's script maps, even in Non-Debug mode. So we should get a sourceCodeLocation with a URL that startsWith "__WebInspector". > > Seems we might still want this code but with a WebInspector.isDebugUIEnabled() check. > > To test, just type this into the console: > js> function foo() { debugger; } foo() Nice catch! In non-debug UI mode the test should cause us to pause in the debugger, with Call Stack -> No Call Frames. Do we care about leaving the inspector call frames visible in the following situation: 1. Enable debug UI mode 2. js> function foo() { debugger; } foo() 3. Disable debug UI mode => Injected script call frames still visible
(In reply to comment #4) > Comment on attachment 267660 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267660&action=review > > >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-491 > >> - continue; > > > > I don't think this is right. Debug Scripts are always included in DebuggerManager's script maps, even in Non-Debug mode. So we should get a sourceCodeLocation with a URL that startsWith "__WebInspector". > > > > Seems we might still want this code but with a WebInspector.isDebugUIEnabled() check. > > > > To test, just type this into the console: > > js> function foo() { debugger; } foo() > > Nice catch! In non-debug UI mode the test should cause us to pause in the > debugger, with Call Stack -> No Call Frames. Do we care about leaving the > inspector call frames visible in the following situation: > > 1. Enable debug UI mode > 2. js> function foo() { debugger; } foo() > 3. Disable debug UI mode > => Injected script call frames still visible No I don't care about leaving them around. We want enough debug UI that things are useful, but not enough that it adds complexity to the code base.
> > js> function foo() { debugger; } foo() > > Nice catch! In non-debug UI mode the test should cause us to pause in the > debugger, with Call Stack -> No Call Frames. This is interesting to me. Why isn't there a "foo" call frame? This might be exposing an issue with isInternalScript(...), since all of this will be included in an internal script from the console: > with ((this && (this.console ? this.console.__commandLineAPI : this.__commandLineAPI)) || {}) { > function foo() { debugger; }; foo(); > //# sourceURL=__WebInspectorInternal__ > > }
Created attachment 267666 [details] [Patch] Proposed Fix
Comment on attachment 267666 [details] [Patch] Proposed Fix Attachment 267666 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/577894 Number of test failures exceeded the failure limit.
Created attachment 267669 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 267666 [details] [Patch] Proposed Fix Attachment 267666 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/577884 Number of test failures exceeded the failure limit.
Created attachment 267670 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 267666 [details] [Patch] Proposed Fix Attachment 267666 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/577899 Number of test failures exceeded the failure limit.
Created attachment 267673 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 267666 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=267666&action=review r- because this broke tests and 1 more open question. Suggestions below on how to address them. > Source/WebInspectorUI/UserInterface/Base/Main.js:1389 > + return WebInspector.showDebugUISetting && WebInspector.showDebugUISetting.value Style: Semicolon. > Source/WebInspectorUI/UserInterface/Base/Main.js:1400 > +WebInspector.isInternalScript = function(url) > +{ > + return url === "__WebInspectorInternal__"; > +} > + > +WebInspector.isDebugScript = function(url) > +{ > + return url && url.startsWith("__WebInspector"); > +} You may need to re-implement these in Test.js, or share the code somehow. Otherwise, LayoutTests (as we've seen on the EWS bots) will fail because of code using them. You may even want to drop the "WebInspector." prefix and just put them in Utilities.js alongside "appendWebInspectorSourceURL()"! > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-323 > - if (script.url && script.url.startsWith("__WebInspector")) > - continue; This part still needs to exist with WebInspector.isDebugUIEnabled(). Otherwise I think Internal scripts can show in the Debugger tab when the inspector first opens. Steps to reproduce: 1. Inspect <body> of about:blank (so Elements tab) 2. Evaluate 1+1 in the console 3. Close inspector 4. Open inspector (should still be Elements tab) 5. Show Debugger tab 6. Ensure sidebar is showing all scripts => I think this would show the Debug Scripts in the Debugger's sidebar > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:401 > if (!script.url) > return; There is a FIXME above this that I think can be eliminated now, or at least updated. We should not be told about any JSC internal scripts at this point. That said, we probably should keep this code for compatibility, maybe even with a COMPATIBILITY(iOS 9): Backends could send the frontend built-in code. But certainly the part about WebInspector internals can be removed, as we control that.
Created attachment 267680 [details] [Patch] Proposed Fix
Comment on attachment 267680 [details] [Patch] Proposed Fix r=me!
Comment on attachment 267680 [details] [Patch] Proposed Fix Clearing flags on attachment: 267680 Committed r194311: <http://trac.webkit.org/changeset/194311>
All reviewed patches have been landed. Closing bug.