Summary: | Web Inspector: Debugger statement in console does not provide any call frames and debugger UI is confused | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||
Component: | Web Inspector | Assignee: | Timothy Hatcher <timothy> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bburg, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Local Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 156924 | ||||||
Attachments: |
|
Description
Timothy Hatcher
2016-04-22 10:56:32 PDT
Created attachment 277079 [details]
Patch
Comment on attachment 277079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277079&action=review > Source/WebInspectorUI/ChangeLog:6 > + Web Inspector: Debugger statement in console does not provide any call frames and debugger UI is confused > + > + https://bugs.webkit.org/show_bug.cgi?id=156919 > + rdar://problem/25857118 Unexpected empty line (changes git log --oneline when committed). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1260 > + if (string.includes("//# sourceURL")) > + return string; I worry about how expensive this could get, but at the moment I don't think we ever have very large strings. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1276 > +function isWebInspectorConsoleExpressionScript(url) Maybe "isWebInspectorConsoleEvaluationScript". s/Expression/Evaluation/ > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-583 > - if (isWebInspectorInternalScript(sourceURL)) > - return; > - I think this can be left in. Now that you have separated Console Evaluations from other Internal Scripts. I don't think we want to create and keep around a WebInspector.Script for every internal script. > Source/WebInspectorUI/UserInterface/Models/Script.js:103 > + return WebInspector.UIString("Console Expression %d").format(this._uniqueDisplayNameNumber); Nit: I like "Console Evaluation %d" instead of "Expression". Since you can evaluate more then just a single expression in the console, its technically a whole program evaluation. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:211 > + var treeElement = this.contentTreeOutline.findTreeElement(representedObject); Style: `let` > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:466 > + return treeElement; The other returns here should probably return `null` now. Comment on attachment 277079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277079&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:551 > + if (!this._activeCallFrame) { > + this._didResumeInternal(); > + return; > + } Maybe we should add an assert / warning for our purposes. This might catch something in the future where we expected to pause and didn't. Comment on attachment 277079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277079&action=review > Source/WebInspectorUI/UserInterface/Models/Script.js:53 > static resetUniqueDisplayNameNumbers() It doesn't look like this ever gets called the first time the inspector opens. Only after page loads. I think DebuggerManager should call this in its constructor. Follow up: https://trac.webkit.org/r199899 |