Created attachment 267476 [details] [Screenshot] Local and Closure sections * SUMMARY Scope chain never shows variables declared with `var` in Local Variables. * REDUCTION <script> (function f() { let a; var x; debugger; })(); </script> * STEPS TO REPRODUCE 1. Inspector -> Debugger tab 2. Inspect above test case, reload page 3. Open right-sidebar (Scope Chain panel) => Expected `x` to appear in Local Variables, instead it has it's own Closure section (see screenshot).
Created attachment 267477 [details] [Screenshot] Chrome devtools, expected behavior Both `a` and `x` appear in locals section, no closures shown.
Created attachment 267640 [details] [PATCH] Work In Progress - Merge and Name Scopes
Created attachment 267641 [details] [IMAGE] Work In Progress - Merge and Name Scopes
Comment on attachment 267640 [details] [PATCH] Work In Progress - Merge and Name Scopes View in context: https://bugs.webkit.org/attachment.cgi?id=267640&action=review > Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:33 > + console.assert(objects.every(function(x) { return x instanceof WebInspector.RemoteObject; })); Just for my own knowledge, would this work here: console.assert(objects.every(x => x instanceof WebInspector.RemoteObject)); > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:173 > + return result; Instead of using a loop, couldn't this be simplified as: return array.slice(startIndex, startIndex + num); > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:213 > + let isClosureScope = s.type === WebInspector.ScopeChainNode.Type.Closure; Would this be a good place to use const? > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:227 > + let objects = firstClosureScope.objects.concat(s.objects); So you mention a FIXME on line 307 that it just combines the two object views together and doesn't make it look pretty. In order to do that, could we do something like: objects.sort((a, b) => <compare by name or something else here>);
(In reply to comment #4) > Comment on attachment 267640 [details] > [PATCH] Work In Progress - Merge and Name Scopes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267640&action=review > > > Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:33 > > + console.assert(objects.every(function(x) { return x instanceof WebInspector.RemoteObject; })); > > Just for my own knowledge, would this work here: > console.assert(objects.every(x => x instanceof WebInspector.RemoteObject)); Yep! Try it in the console: var a = [1, 2, 3, 4]; a.every(x => typeof x === "number"); > > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:173 > > + return result; > > Instead of using a loop, couldn't this be simplified as: > return array.slice(startIndex, startIndex + num); > > > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:213 > > + let isClosureScope = s.type === WebInspector.ScopeChainNode.Type.Closure; > > Would this be a good place to use const? I used const in instances like this initially, but I think others prefer to restrict use to magic numbers, etc. I don't think we've settled on a coding style. > > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:227 > > + let objects = firstClosureScope.objects.concat(s.objects); > > So you mention a FIXME on line 307 that it just combines the two object > views together and doesn't make it look pretty. In order to do that, could > we do something like: > objects.sort((a, b) => <compare by name or something else here>);
Comment on attachment 267640 [details] [PATCH] Work In Progress - Merge and Name Scopes View in context: https://bugs.webkit.org/attachment.cgi?id=267640&action=review >>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:173 >>> + return result; >> >> Instead of using a loop, couldn't this be simplified as: >> return array.slice(startIndex, startIndex + num); > > I used const in instances like this initially, but I think others prefer to restrict use to magic numbers, etc. I don't think we've settled on a coding style. Ignore my comment. It was inserted into the wrong location. >> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:213 >> + let isClosureScope = s.type === WebInspector.ScopeChainNode.Type.Closure; > > Would this be a good place to use const? I used const in instances like this initially, but I think others prefer to restrict use to magic numbers, etc. I don't think we've settled on a coding style.
Comment on attachment 267640 [details] [PATCH] Work In Progress - Merge and Name Scopes View in context: https://bugs.webkit.org/attachment.cgi?id=267640&action=review > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:203 > + // call frame. In particular, function call frames may have multiple top level Nit: 'some of the scopes' > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:220 > + firstClosureScope.__isFirstClosure = true; Let's define a WebInspector.ScopeChainDetailsSidebarPanel.FirstClosureSymbol, since we'e been moving to symbols for external properties. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:229 > + merged.__isFirstClosure = true; Use a symbol. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:259 > + title = WebInspector.UIString("Local Variables (%s)").format(call.functionName); Since there is only one Local Variables scope I don't think we need to include the function name. The sidebar looks a little busy with the additional text, now that closure scopes are named. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:267 > + if (scope.__isFirstClosure && call.functionName) Use a symbol.
(In reply to comment #3) > Created attachment 267641 [details] > [IMAGE] Work In Progress - Merge and Name Scopes Does the change fix https://bugs.webkit.org/show_bug.cgi?id=152348? I don't see any empty Closure sections in the screenshot.
(In reply to comment #8) > (In reply to comment #3) > > Created attachment 267641 [details] > > [IMAGE] Work In Progress - Merge and Name Scopes > > Does the change fix https://bugs.webkit.org/show_bug.cgi?id=152348? I don't > see any empty Closure sections in the screenshot. Applied the patch and it doesn't. I'll fix it separately.
> > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:173 > > + return result; > > Instead of using a loop, couldn't this be simplified as: > return array.slice(startIndex, startIndex + num); Yep, I planned on moving to slice! > > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:213 > > + let isClosureScope = s.type === WebInspector.ScopeChainNode.Type.Closure; > > Would this be a good place to use const? I have been preferring const for variables we know will never change: const second = 1000; const minute = 60 * second; const placeholder = "\u1234"; I don't really know what to call these. Static constants? In this case its a local variable that doesn't change inside the block, but it could be `true` one iteration and `false` the next. Maybe I'm in the minority. > > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:227 > > + let objects = firstClosureScope.objects.concat(s.objects); > > So you mention a FIXME on line 307 that it just combines the two object > views together and doesn't make it look pretty. In order to do that, could > we do something like: > objects.sort((a, b) => <compare by name or something else here>); Nope, the issue here is not the objects themselves, but the descriptors fetched from the objects! The scope objects are just JavaScript objects with properties that are the variable names and values.
Created attachment 268259 [details] [PATCH] Proposed Fix
<rdar://problem/24052011>
Created attachment 268260 [details] [IMAGE] After - Merged Scopes and Named Closures
Comment on attachment 268259 [details] [PATCH] Proposed Fix Attachment 268259 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/651496 New failing tests: inspector/debugger/breakpoint-scope.html inspector/model/scope-chain-node.html
Created attachment 268262 [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 268259 [details] [PATCH] Proposed Fix Attachment 268259 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/651492 New failing tests: inspector/debugger/breakpoint-scope.html inspector/model/scope-chain-node.html
Created attachment 268263 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268259 [details] [PATCH] Proposed Fix Attachment 268259 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/651497 New failing tests: inspector/debugger/breakpoint-scope.html inspector/model/scope-chain-node.html
Created attachment 268264 [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
> New failing tests: > inspector/debugger/breakpoint-scope.html > inspector/model/scope-chain-node.html Oops, easy fix. These need to change `scope.object` to `scope.objects[0]` since in these instances there will only ever be one object for the scope. We only ever merge scopes ourselves for the UI.
Created attachment 268292 [details] [PATCH] Proposed Fix
Comment on attachment 268292 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=268292&action=review > Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:33 > + console.assert(objects.every(function(x) { return x instanceof WebInspector.RemoteObject; })); Could be an arrow function.
Comment on attachment 268292 [details] [PATCH] Proposed Fix Rejecting attachment 268292 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 268292, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: tor/debugger/breakpoint-scope.html patching file LayoutTests/inspector/model/scope-chain-node.html patching file Source/WebInspectorUI/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 283. Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/658328
<http://trac.webkit.org/changeset/194685>