* SUMMARY Should be able to expand Objects in Heap Allocations View to see exactly what it retains The Retained Size of most objects can be made more useful when you can see exactly what objects the original object retains (and dominates).
<rdar://problem/25633863>
Created attachment 276194 [details] [PATCH] Proposed Fix
Created attachment 276195 [details] [IMAGE] Expand Function - Shows FunctionExecutable
Created attachment 276196 [details] [IMAGE] Expand Object - Shows properties and previews
Comment on attachment 276194 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=276194&action=review > Source/WebInspectorUI/UserInterface/Models/HeapSnapshotRootPath.js:57 > + case WebInspector.HeapSnapshotEdgeProxy.EdgeType.Internal: > + return emDash; This looks funny in the screenshots and could be confusing. I'd either show nothing (drop the "—:") or have it say "(internal)" or something.
Comment on attachment 276194 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=276194&action=review >> Source/WebInspectorUI/UserInterface/Models/HeapSnapshotRootPath.js:57 >> + return emDash; > > This looks funny in the screenshots and could be confusing. I'd either show nothing (drop the "—:") or have it say "(internal)" or something. It might be best to show nothing, since these internal things will usually say "Internal Object" in red at the end. Or drop that and move "(internal):" to the front.
Comment on attachment 276194 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=276194&action=review > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:135 > + nameElement.textContent = edgeText + ": " + this._node.className + " "; We now show this._node.className twice if there is a preview (see JSGlobalLexicalEnvironment). Should we drop the className from the preview in this case? > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:200 > + for (let i = 0; i < children.length; ++i) { Back to classic for loops for performance? > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:214 > + for (let i = 0; i < instances.length; ++i) Ditto?
Comment on attachment 276194 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=276194&action=review > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotClassDataGridNode.js:66 > + iconElement.classList.add("icon", WebInspector.HeapSnapshotClusterContentView.iconStyleClassNameForClassName(className)); Why do some internal types show [ ? ] for the icon while others show [ N ]? I think they should all be [ N ] unless there is a reason not distinguish them.
Comment on attachment 276194 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=276194&action=review >> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotClassDataGridNode.js:66 >> + iconElement.classList.add("icon", WebInspector.HeapSnapshotClusterContentView.iconStyleClassNameForClassName(className)); > > Why do some internal types show [ ? ] for the icon while others show [ N ]? I think they should all be [ N ] unless there is a reason not distinguish them. All internal types (JSC objects that are note tied to a particular global object) show [?]. There are a few JSC objects that show [N] that maybe you think should be [?]. Such as an JSLexicalEnvironment, InjectedScriptHost? >> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:135 >> + nameElement.textContent = edgeText + ": " + this._node.className + " "; > > We now show this._node.className twice if there is a preview (see JSGlobalLexicalEnvironment). Should we drop the className from the preview in this case? Currently our previews of lexical environments is poor. I'd rather we just improve them, or failing that eliminate them. >> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:200 >> + for (let i = 0; i < children.length; ++i) { > > Back to classic for loops for performance? This one is needed because we use `i` in the loop. >> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:214 >> + for (let i = 0; i < instances.length; ++i) > > Ditto? This one is needed because we use `i` in the loop.
Comment on attachment 276194 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=276194&action=review >>> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotClassDataGridNode.js:66 >>> + iconElement.classList.add("icon", WebInspector.HeapSnapshotClusterContentView.iconStyleClassNameForClassName(className)); >> >> Why do some internal types show [ ? ] for the icon while others show [ N ]? I think they should all be [ N ] unless there is a reason not distinguish them. > > All internal types (JSC objects that are note tied to a particular global object) show [?]. > There are a few JSC objects that show [N] that maybe you think should be [?]. Such as an JSLexicalEnvironment, InjectedScriptHost? I'm not sure having two icons is helpful. I think [ N ] is a better choice.
<http://trac.webkit.org/changeset/199379> I went with just dropping any edge name for Internal edges.
> > There are a few JSC objects that show [N] that maybe you think should be [?]. Such as an JSLexicalEnvironment, InjectedScriptHost? > > I'm not sure having two icons is helpful. I think [ N ] is a better choice. <https://webkit.org/b/156513> Web Inspector: Show the normal Native icon for all Internal objects in Heap Snapshots