Bug 152349 – Web Inspector: Scope chain shows too many scopes for functions (`let` and `var` in the same function are two scopes)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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

[Matt Baker]

Created attachment 267477 [details]
[Screenshot] Chrome devtools, expected behavior

Both `a` and `x` appear in locals section, no closures shown.

[Joseph Pecoraro]

Created attachment 267640 [details]
[PATCH] Work In Progress - Merge and Name Scopes

[Joseph Pecoraro]

Created attachment 267641 [details]
[IMAGE] Work In Progress - Merge and Name Scopes

[Devin Rousso]

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

[Matt Baker]

(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>);

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

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

[Matt Baker]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 268259 [details]
[PATCH] Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/24052011>

[Joseph Pecoraro]

Created attachment 268260 [details]
[IMAGE] After - Merged Scopes and Named Closures

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 268292 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

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.

[WebKit Commit Bot]

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

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/194685>