Summary: | Web Inspector: Scope chain shouldn't show empty Closure sections | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, hi, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, saam, timothy, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||
Attachments: |
|
I just tried this with the given reduction and I didn't see any empty closure sections. Can you still reproduce it? Created attachment 288491 [details]
[HTML] Reduction
Created attachment 288492 [details]
[Image] Empty Closure section
Still relevant with the updated reduction.
Created attachment 288495 [details]
Patch
Comment on attachment 288495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288495&action=review > Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:164 > Nit: newlines aren't necessary between consecutive one-line getters. > Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:170 > What about _hasPropertyEntries instead? I think it's less ambiguous. > Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:171 > + get expanded() { return this._expanded; } Nit: add a space since the next statement is a method, not a property. > Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:292 > + for (let propertyDescriptor of properties) { I think we should resist the urge to refactor nearby code like this, as it makes it more time consuming to scan diffs. Based on our discussion over IRC, we should try deferring the creation of sections until properties are available. Ideally we shouldn't create DetailsSections that are just going to be removed right away. Comment on attachment 288495 [details]
Patch
r- since the goal is now to postpone the creation of DetailsSections so empty sections aren't created.
Created attachment 288504 [details]
Patch
Comment on attachment 288504 [details] Patch Attachment 288504 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2049844 New failing tests: fast/scrolling/ios/scroll-events-back-forward.html Created attachment 288511 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 288504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288504&action=review > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:249 > + Promise.all(updatePromises) > + .then((updateEvents) => { Err, I think the style here would be to indent the .then, and use .catch for a global function we use to show the uncaught exception reporter for errors: Promise.all([...]) .then(() => { ... }) .catch(handlePromiseException); > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:254 > + detailsSections.remove(detailsSection); > + detailsSection.element.remove(); > + return; > + } Does this mean the sidebar "flashes" removing this section? (I haven't tested). Comment on attachment 288504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288504&action=review >> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:249 >> + .then((updateEvents) => { > > Err, I think the style here would be to indent the .then, and use .catch for a global function we use to show the uncaught exception reporter for errors: > > Promise.all([...]) > .then(() => { > ... > }) > .catch(handlePromiseException); Brian and I have been talking about this, and we are not indenting .then() inside LayoutTests. I think we should keep it consistent. >> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:254 >> + } > > Does this mean the sidebar "flashes" removing this section? (I haven't tested). I have not seen any flashes. I could make it so that each DetailsSection starts out hidden, just in case. Another option would be to rework the entire function so that it only resolves the returned promise when every ObjectTreeView for each scope has been updated. Comment on attachment 288504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288504&action=review >>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:254 >>> + } >> >> Does this mean the sidebar "flashes" removing this section? (I haven't tested). > > I have not seen any flashes. I could make it so that each DetailsSection starts out hidden, just in case. Another option would be to rework the entire function so that it only resolves the returned promise when every ObjectTreeView for each scope has been updated. If possible we shouldn't add a section to the DOM at all until a promise resolves. Then we could add it or drop it. Created attachment 288754 [details]
Patch
So I tried reworking _generateCallFramesSection to only resolve once every scope has updated all of its properties (thus only creating DetailsSection instances if a scope has at least one property), but it causes a noticeable delay to the rendering of the scope chain. This is most likely due to the sheer number of calls to resolve PropertyDescriptor values for RemoteObject. I think that a current "middle-ground" solution may be to have placeholder elements be added to the DOM (basically blank <div>) that we can remove if there are no properties or replace with a DetailsSection.
Created attachment 288769 [details]
Patch
Comment on attachment 288769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288769&action=review > Source/WebInspectorUI/ChangeLog:30 > + * UserInterface/Views/ScopeChainDetailsSidebarPanel.js: > + (WebInspector.ScopeChainDetailsSidebarPanel.prototype._generateCallFramesSection): > + Changed logic to not resolve until every scope has updated all of its objects. > + DetailsSection elements are not created unless that section has scopes with properties. > + Also added sorting to the entries in each scope (it still keeps "this" at the top). I think this is the wrong approach. I think the backend should just tell the frontend "this scope is empty" so we don't have to do back and forth messages and delay updating the UI. Especially because these are very costly messages (get a list of all objects and their previews). Likewise I don't want us to delay updating the UI checking if the Global scope (a huge list of values) is empty or not. I think this would be very easy to do, but it is non-obvious: 1. Debugger.json - Update Debugger.Scope object type in the protocol to include an "isEmpty" property 2. InjectedScriptSource.js - InjectedScript.CallFrameProxy._createScopeJson would check if "object" is empty or not 3. Have the frontend handle this (DebuggerManager.prototype._scopeChainNodeFromPayload, WebInspector.ScopeChainNode, etc) Let me know if you want to tackle this! Created attachment 289187 [details]
Patch
Comment on attachment 289187 [details]
Patch
Could DebuggerManager just drop empty payloads? If _scopeChainNodeFromPayload returned null for payloads marked as empty, _scopeChainFromPayload could ignore them and the `isEmpty` flag wouldn't need to leak into Model/View code.
(In reply to comment #19) > Comment on attachment 289187 [details] > Patch > > Could DebuggerManager just drop empty payloads? If > _scopeChainNodeFromPayload returned null for payloads marked as empty, > _scopeChainFromPayload could ignore them and the `isEmpty` flag wouldn't > need to leak into Model/View code. I don't think that would be a good idea. The frontend should have complete information when determining what to show. And an isEmpty Closure scope could be promoted to the Local scope which has `this` stuff into it. If empty scopes were dropped by DebuggerManager we would have chosen he wrong scope to make "Local". Comment on attachment 289187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289187&action=review r- because we should be able to write a test for this. The patch itself looks great though. See https://bugs.webkit.org/show_bug.cgi?id=162113 for an example patch in this exact area. > Source/JavaScriptCore/inspector/InjectedScriptSource.js:1339 > + if (!Object.keys(object).length) Object.keys is slightly wasteful and user overridable (which is a known issue with our injected script source right now). We should just write our own "isEmptyObject" function at the top with other utilities that cannot be defeated by code on the page: function isEmptyObject(object) { for (let key in object) return false; return true; } > Source/JavaScriptCore/inspector/protocol/Debugger.json:88 > + { "name": "isEmpty", "type": "boolean", "optional": true, "description": "Whether the object has any properties." } What do you think about just naming this "empty"? We seem to use "is" sparingly, and I think empty would be clear. > Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:51 > + get isEmpty() { return this._isEmpty; } Likewise this would be "empty" and would read better. Created attachment 292589 [details]
Patch
Comment on attachment 292589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292589&action=review r=me, this is way nicer and the test output is a nice improvement! > Source/JavaScriptCore/inspector/protocol/Debugger.json:88 > + { "name": "empty", "type": "boolean", "optional": true, "description": "Whether the object has any properties." } "Whether the object has any properties" could be "Whether the scope has any variables". I believe that is the intent of this property, it just manifests as the object having no properties. > Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:172 > + if (scope.type !== WebInspector.ScopeChainNode.Type.Local && scope.empty) I would flip these. The empty check first is the intent, the Local Scope is a condition. if (scope.empty && scope.type !== Local) Created attachment 292653 [details]
Patch
Comment on attachment 292653 [details] Patch Rejecting attachment 292653 [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', 'validate-changelog', '--check-oops', '--non-interactive', 292653, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2360808 Created attachment 292654 [details]
Patch
Oops. Forgot the "Oops". (hehehe)
Comment on attachment 292654 [details] Patch Clearing flags on attachment: 292654 Committed r207784: <http://trac.webkit.org/changeset/207784> All reviewed patches have been landed. Closing bug. |
Created attachment 267472 [details] [Screenshot] Empty Closure section * SUMMARY Scope chain shouldn't show empty Closure sections. We don't display other scope types when they aren't present (With scopes). * 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) => An empty closure section appears at the bottom (see attached screenshot)