Bug 156419 – Web Inspector: Should be able to expand Objects in Heap Allocations View to see exactly what it retains

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

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/25633863>

[Joseph Pecoraro]

Created attachment 276194 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 276195 [details]
[IMAGE] Expand Function - Shows FunctionExecutable

[Joseph Pecoraro]

Created attachment 276196 [details]
[IMAGE] Expand Object - Shows properties and previews

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Timothy Hatcher]

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?

[Timothy Hatcher]

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.

[Joseph Pecoraro]

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.

[Timothy Hatcher]

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.

[Joseph Pecoraro]

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

I went with just dropping any edge name for Internal edges.

[Joseph Pecoraro]

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