Web Inspector: Scope chain shows too many scopes for functions (`let` and `var` in...
authorjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Jan 2016 01:44:08 +0000 (01:44 +0000)
committerjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Jan 2016 01:44:08 +0000 (01:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=152349

Reviewed by Timothy Hatcher.

Source/WebInspectorUI:

* Localizations/en.lproj/localizedStrings.js:
New "Closure Variables (%s)" string for including the function name alongside
the closure scope section.

* UserInterface/Models/ScopeChainNode.js:
(WebInspector.ScopeChainNode):
(WebInspector.ScopeChainNode.prototype.get objects):
(WebInspector.ScopeChainNode.prototype.get object): Deleted.
Allow a ScopeChainNode to hold onto multiple RemoteObjects (for multiple JSScopes).

* UserInterface/Controllers/DebuggerManager.js:
(WebInspector.DebuggerManager.prototype._scopeChainNodeFromPayload):
Simplify the legacy case and convert "Local" to "Closure". The UI will now
determine and display "Local" where appropriate in a backwards compatible way.

* UserInterface/Views/ScopeChainDetailsSidebarPanel.js:
(WebInspector.ScopeChainDetailsSidebarPanel.prototype._generateCallFramesSection):
Associate ScopeChain nodes with CallFrame frames so that we can better combine
variable sections and better label closure sections.

* UserInterface/Views/ScopeChainDetailsSidebarPanel.css:
(.scope-chain .details-section > .content > .group > .row.properties:not(.empty)):
(.scope-chain .details-section > .content > .group > .row.properties:not(.empty):last-child):
Styles for two object trees property sections next to eachother.

LayoutTests:

* inspector/debugger/breakpoint-scope.html:
* inspector/model/scope-chain-node.html:
The ScopeChainNode model object changed, so update tests.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@194685 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/inspector/debugger/breakpoint-scope.html
LayoutTests/inspector/model/scope-chain-node.html
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js
Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js
Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js
Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.css
Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js

index 9a7e004..929067d 100644 (file)
@@ -1,3 +1,14 @@
+2016-01-06  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: Scope chain shows too many scopes for functions (`let` and `var` in the same function are two scopes)
+        https://bugs.webkit.org/show_bug.cgi?id=152349
+
+        Reviewed by Timothy Hatcher.
+
+        * inspector/debugger/breakpoint-scope.html:
+        * inspector/model/scope-chain-node.html:
+        The ScopeChainNode model object changed, so update tests.
+
 2016-01-06  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [GTK] css-multicol-1 tests are failing
index 96f35b8..76a26bd 100644 (file)
@@ -57,7 +57,7 @@ function test()
         for (var scope of scopeChain) {
             scopeTypes.push(scope.type);
             if (scope.type !== WebInspector.ScopeChainNode.Type.Global) {
-                scope.object.getAllPropertyDescriptors(function(properties) {
+                scope.objects[0].getAllPropertyDescriptors(function(properties) {
                     InspectorTest.log(scopeTypes[scopeTypeIndex++] + " properties:");
                     for (var propertyDescriptor of properties)
                         InspectorTest.log("    " + propertyDescriptor.name);
index dbaabaa..37ec318 100644 (file)
@@ -30,7 +30,7 @@ function test()
                 if (scope.type === WebInspector.ScopeChainNode.Type.Global)
                     resolve({scope, propertyDescriptors: []});
                 else {
-                    scope.object.getDisplayablePropertyDescriptors((propertyDescriptors) => {
+                    scope.objects[0].getDisplayablePropertyDescriptors((propertyDescriptors) => {
                         resolve({scope, propertyDescriptors});
                     });
                 }
index 9f8d813..24366f6 100644 (file)
@@ -1,3 +1,35 @@
+2016-01-06  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: Scope chain shows too many scopes for functions (`let` and `var` in the same function are two scopes)
+        https://bugs.webkit.org/show_bug.cgi?id=152349
+
+        Reviewed by Timothy Hatcher.
+
+        * Localizations/en.lproj/localizedStrings.js:
+        New "Closure Variables (%s)" string for including the function name alongside
+        the closure scope section.
+
+        * UserInterface/Models/ScopeChainNode.js:
+        (WebInspector.ScopeChainNode):
+        (WebInspector.ScopeChainNode.prototype.get objects):
+        (WebInspector.ScopeChainNode.prototype.get object): Deleted.
+        Allow a ScopeChainNode to hold onto multiple RemoteObjects (for multiple JSScopes).
+
+        * UserInterface/Controllers/DebuggerManager.js:
+        (WebInspector.DebuggerManager.prototype._scopeChainNodeFromPayload):
+        Simplify the legacy case and convert "Local" to "Closure". The UI will now
+        determine and display "Local" where appropriate in a backwards compatible way.
+
+        * UserInterface/Views/ScopeChainDetailsSidebarPanel.js:
+        (WebInspector.ScopeChainDetailsSidebarPanel.prototype._generateCallFramesSection):
+        Associate ScopeChain nodes with CallFrame frames so that we can better combine
+        variable sections and better label closure sections.
+
+        * UserInterface/Views/ScopeChainDetailsSidebarPanel.css:
+        (.scope-chain .details-section > .content > .group > .row.properties:not(.empty)):
+        (.scope-chain .details-section > .content > .group > .row.properties:not(.empty):last-child):
+        Styles for two object trees property sections next to eachother.
+
 2016-01-06  Devin Rousso  <dcrousso+webkit@gmail.com>
 
         Web Inspector: Add basic Animation section to Visual Sidebar
index 2811851..d1e981e 100644 (file)
Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ
index aca2d84..427adb0 100644 (file)
@@ -623,8 +623,9 @@ WebInspector.DebuggerManager = class DebuggerManager extends WebInspector.Object
 
         // COMPATIBILITY (iOS 9): Debugger.ScopeType.Local used to be provided by the backend.
         // Newer backends no longer send this enum value, it should be computed by the frontend.
+        // Map this to "Closure" type. The frontend can recalculate this when needed.
         case DebuggerAgent.ScopeType.Local:
-            type = WebInspector.ScopeChainNode.Type.Local;
+            type = WebInspector.ScopeChainNode.Type.Closure;
             break;
 
         default:
@@ -632,7 +633,7 @@ WebInspector.DebuggerManager = class DebuggerManager extends WebInspector.Object
         }
 
         var object = WebInspector.RemoteObject.fromPayload(payload.object);
-        return new WebInspector.ScopeChainNode(type, object);
+        return new WebInspector.ScopeChainNode(type, [object]);
     }
 
     _pauseReasonFromPayload(payload)
index 79e0f4f..613a8d1 100644 (file)
 
 WebInspector.ScopeChainNode = class ScopeChainNode extends WebInspector.Object
 {
-    constructor(type, object)
+    constructor(type, objects)
     {
         super();
 
         console.assert(typeof type === "string");
-        console.assert(object instanceof WebInspector.RemoteObject);
+        console.assert(objects.every(function(x) { return x instanceof WebInspector.RemoteObject; }));
 
         if (type in WebInspector.ScopeChainNode.Type)
             type = WebInspector.ScopeChainNode.Type[type];
 
         this._type = type || null;
-        this._object = object || null;
+        this._objects = objects || [];
     }
 
     // Public
@@ -46,9 +46,9 @@ WebInspector.ScopeChainNode = class ScopeChainNode extends WebInspector.Object
         return this._type;
     }
 
-    get object()
+    get objects()
     {
-        return this._object;
+        return this._objects;
     }
 };
 
index 5b60c6d..8e59440 100644 (file)
     width: calc(100% - 2px);
     overflow: hidden;
 }
+
+.scope-chain .details-section > .content > .group > .row.properties:not(.empty) {
+    padding-top: 0px;
+    padding-bottom: 0px;
+}
+
+.scope-chain .details-section > .content > .group > .row.properties:not(.empty):last-child {
+    position: relative;
+    top: -2px;
+
+    padding-bottom: 2px;
+}
index 9463406..6e2a3d0 100644 (file)
@@ -164,79 +164,159 @@ WebInspector.ScopeChainDetailsSidebarPanel = class ScopeChainDetailsSidebarPanel
         for (let type in WebInspector.ScopeChainNode.Type)
             sectionCountByType.set(WebInspector.ScopeChainNode.Type[type], 0);
 
+        // Scopes list goes from top/local (1) to bottom/global (5)
+        // Call frames list goes from top/local (1) to bottom/global (2)
+        //   [scope1, scope2, scope3, scope4, scope5]
+        //   [CallFrame1, CallFrame2]
         let scopeChain = callFrame.scopeChain;
-        for (let scope of scopeChain) {
-            let title = null;
-            let extraPropertyDescriptor = null;
-            let collapsedByDefault = false;
-
-            let count = sectionCountByType.get(scope.type);
-            sectionCountByType.set(scope.type, ++count);
-
-            switch (scope.type) {
-                case WebInspector.ScopeChainNode.Type.Local:
-                    foundLocalScope = true;
-                    collapsedByDefault = false;
-
-                    title = WebInspector.UIString("Local Variables");
-
-                    if (callFrame.thisObject)
-                        extraPropertyDescriptor = new WebInspector.PropertyDescriptor({name: "this", value: callFrame.thisObject});
-                    break;
-
-                case WebInspector.ScopeChainNode.Type.Block:
-                    title = WebInspector.UIString("Block Variables");
-                    collapsedByDefault = false;
-                    break;
+        let callFrames = WebInspector.debuggerManager.callFrames;
+
+        // Group scopes with the call frame containing them.
+        // Creating a map that looks like:
+        //   CallFrame2 => [scope5, scope4]
+        //   CallFrame1 => [scope3, scope2, scope1]
+        let reversedScopeChain = scopeChain.slice().reverse();
+        let callFrameScopes = new Map;
+        let lastLength = 0;
+        for (let i = callFrames.length - 1; i >= 0; --i) {
+            let nextCallFrame = callFrames[i];
+            console.assert(nextCallFrame.scopeChain.length > lastLength);
+            callFrameScopes.set(nextCallFrame, reversedScopeChain.slice(lastLength, nextCallFrame.scopeChain.length));
+            lastLength = nextCallFrame.scopeChain.length;
+            if (nextCallFrame === callFrame) {
+                console.assert(lastLength === scopeChain.length);
+                break;
+            }
+        }
 
-                case WebInspector.ScopeChainNode.Type.Closure:
-                    title = WebInspector.UIString("Closure Variables");
-                    collapsedByDefault = false;
+        // Now that we have this map we can merge some of the scopes within an individual
+        // call frame. In particular, function call frames may have multiple top level
+        // closure scopes (one for `var`s one for `let`s) that can be combined to a
+        // single scope of variables.
+        // This modifies the Map, resulting in:
+        //   CallFrame2 => [scope4, scope5]
+        //   CallFrame1 => [scope1, scope2&3]
+        for (let [currentCallFrame, scopes] of callFrameScopes) {
+            let firstClosureScope = null;
+            for (let scope of scopes) {
+                // Reached a non-closure scope. Bail.
+                let isClosureScope = scope.type === WebInspector.ScopeChainNode.Type.Closure;
+                if (!isClosureScope && firstClosureScope)
                     break;
 
-                case WebInspector.ScopeChainNode.Type.Catch:
-                    title = WebInspector.UIString("Catch Variables");
-                    collapsedByDefault = false;
-                    break;
+                // Found first closure scope. Mark it so we can provide the function name later in the UI.
+                if (isClosureScope && !firstClosureScope) {
+                    firstClosureScope = scope;
+                    firstClosureScope[WebInspector.ScopeChainDetailsSidebarPanel.CallFrameBaseClosureScopeSymbol] = true;
+                    continue;
+                }
 
-                case WebInspector.ScopeChainNode.Type.FunctionName:
-                    title = WebInspector.UIString("Function Name Variable");
-                    collapsedByDefault = true;
+                // Found 2 sequential top level closure scopes. Merge and mark it so we can provide the function name later in the UI.
+                if (isClosureScope && firstClosureScope) {
+                    let type = currentCallFrame === callFrame ? WebInspector.ScopeChainNode.Type.Local : WebInspector.ScopeChainNode.Type.Closure;
+                    let objects = firstClosureScope.objects.concat(scope.objects);
+                    let merged = new WebInspector.ScopeChainNode(type, objects);
+                    merged[WebInspector.ScopeChainDetailsSidebarPanel.CallFrameBaseClosureScopeSymbol] = true;
+                    console.assert(objects.length === 2);
+
+                    let index = scopes.indexOf(firstClosureScope);
+                    scopes.splice(index, 1); // Remove one of them.
+                    scopes[index] = merged; // Replace the remaining with the merged.
                     break;
+                }
+            }
+            scopes.reverse();
+        }
 
-                case WebInspector.ScopeChainNode.Type.With:
-                    title = WebInspector.UIString("With Object Properties");
-                    collapsedByDefault = foundLocalScope;
-                    break;
+        // Now we can walk the list of call frames and their scopes.
+        // We walk in top -> down order:
+        //   CallFrame1 => [scope1, scope2&3]
+        //   CallFrame2 => [scope5, scope4]
+        for (let [call, scopes] of [...callFrameScopes.entries()].reverse()) {
+            for (let scope of scopes) {
+                let title = null;
+                let extraPropertyDescriptor = null;
+                let collapsedByDefault = false;
+
+                let count = sectionCountByType.get(scope.type);
+                sectionCountByType.set(scope.type, ++count);
+
+                switch (scope.type) {
+                    case WebInspector.ScopeChainNode.Type.Local:
+                        foundLocalScope = true;
+                        collapsedByDefault = false;
+                        title = WebInspector.UIString("Local Variables");
+                        if (call.thisObject)
+                            extraPropertyDescriptor = new WebInspector.PropertyDescriptor({name: "this", value: call.thisObject});
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.Closure:
+                        if (scope[WebInspector.ScopeChainDetailsSidebarPanel.CallFrameBaseClosureScopeSymbol] && call.functionName)
+                            title = WebInspector.UIString("Closure Variables (%s)").format(call.functionName);
+                        else
+                            title = WebInspector.UIString("Closure Variables");
+                        collapsedByDefault = false;
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.Block:
+                        title = WebInspector.UIString("Block Variables");
+                        collapsedByDefault = false;
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.Catch:
+                        title = WebInspector.UIString("Catch Variables");
+                        collapsedByDefault = false;
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.FunctionName:
+                        title = WebInspector.UIString("Function Name Variable");
+                        collapsedByDefault = true;
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.With:
+                        title = WebInspector.UIString("With Object Properties");
+                        collapsedByDefault = foundLocalScope;
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.Global:
+                        title = WebInspector.UIString("Global Variables");
+                        collapsedByDefault = true;
+                        break;
+
+                    case WebInspector.ScopeChainNode.Type.GlobalLexicalEnvironment:
+                        title = WebInspector.UIString("Global Lexical Environment");
+                        collapsedByDefault = true;
+                        break;
+                }
 
-                case WebInspector.ScopeChainNode.Type.Global:
-                    title = WebInspector.UIString("Global Variables");
-                    collapsedByDefault = true;
-                    break;
+                let detailsSectionIdentifier = scope.type + "-" + sectionCountByType.get(scope.type);
 
-                case WebInspector.ScopeChainNode.Type.GlobalLexicalEnvironment:
-                    title = WebInspector.UIString("Global Lexical Environment");
-                    collapsedByDefault = true;
-                    break;
-            }
+                // FIXME: This just puts two ObjectTreeViews next to eachother, but that means
+                // that properties are not nicely sorted between the two separate lists.
 
-            let detailsSectionIdentifier = scope.type + "-" + sectionCountByType.get(scope.type);
+                let rows = [];
+                for (let object of scope.objects) {
+                    let scopePropertyPath = WebInspector.PropertyPath.emptyPropertyPathForScope(object);
+                    let objectTree = new WebInspector.ObjectTreeView(object, WebInspector.ObjectTreeView.Mode.Properties, scopePropertyPath);
 
-            let scopePropertyPath = WebInspector.PropertyPath.emptyPropertyPathForScope(scope.object);
-            let objectTree = new WebInspector.ObjectTreeView(scope.object, WebInspector.ObjectTreeView.Mode.Properties, scopePropertyPath);
+                    objectTree.showOnlyProperties();
 
-            objectTree.showOnlyProperties();
+                    if (extraPropertyDescriptor) {
+                        objectTree.appendExtraPropertyDescriptor(extraPropertyDescriptor);
+                        extraPropertyDescriptor = null;
+                    }
 
-            if (extraPropertyDescriptor)
-                objectTree.appendExtraPropertyDescriptor(extraPropertyDescriptor);
+                    let treeOutline = objectTree.treeOutline;
+                    treeOutline.addEventListener(WebInspector.TreeOutline.Event.ElementAdded, this._treeElementAdded.bind(this, detailsSectionIdentifier), this);
+                    treeOutline.addEventListener(WebInspector.TreeOutline.Event.ElementDisclosureDidChanged, this._treeElementDisclosureDidChange.bind(this, detailsSectionIdentifier), this);
 
-            let treeOutline = objectTree.treeOutline;
-            treeOutline.addEventListener(WebInspector.TreeOutline.Event.ElementAdded, this._treeElementAdded.bind(this, detailsSectionIdentifier), this);
-            treeOutline.addEventListener(WebInspector.TreeOutline.Event.ElementDisclosureDidChanged, this._treeElementDisclosureDidChange.bind(this, detailsSectionIdentifier), this);
+                    rows.push(new WebInspector.DetailsSectionPropertiesRow(objectTree));
+                }
 
-            let detailsSection = new WebInspector.DetailsSection(detailsSectionIdentifier, title, null, null, collapsedByDefault);
-            detailsSection.groups[0].rows = [new WebInspector.DetailsSectionPropertiesRow(objectTree)];
-            detailsSections.push(detailsSection);
+                let detailsSection = new WebInspector.DetailsSection(detailsSectionIdentifier, title, null, null, collapsedByDefault);
+                detailsSection.groups[0].rows = rows;
+                detailsSections.push(detailsSection);
+            }
         }
 
         return Promise.resolve(detailsSections);
@@ -464,3 +544,4 @@ WebInspector.ScopeChainDetailsSidebarPanel = class ScopeChainDetailsSidebarPanel
 
 WebInspector.ScopeChainDetailsSidebarPanel._autoExpandProperties = new Set;
 WebInspector.ScopeChainDetailsSidebarPanel.WatchExpressionsObjectGroupName = "watch-expressions";
+WebInspector.ScopeChainDetailsSidebarPanel.CallFrameBaseClosureScopeSymbol = Symbol("scope-chain-call-frame-base-closure-scope");