Bug 87009 – Web Inspector: introduce a helper for HeapSnapshot post-processing tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

We have many post processing steps but the test coverage is poor.
The helper will simplify the snapshot mocking process.

It will have two public classes InspectorTest.HeapSnapshotBuilder and InspectorTest.HeapNode and node/edge type constants.

the sample:

    var builder = new InspectorTest.HeapSnapshotBuilder();

    var debuggerNode = new InspectorTest.HeapNode("Debugger");
    builder.rootNode.linkNode(debuggerNode, InspectorTest.HeapEdge.element);

    var windowNode = new InspectorTest.HeapNode("Window");
    builder.rootNode.linkNode(windowNode, InspectorTest.HeapEdge.shortcut);

    debuggerNode.linkNode(windowNode, InspectorTest.HeapEdge.property, "windowProperty");

    return builder.generateSnapshot();

[Ilya Tikhonovsky]

Created attachment 143007 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 143007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143007&action=review

> LayoutTests/inspector/profiler/heap-snapshot-test.js:384
> +    this._strings2id = {};

_strings2id -> _string2id

> LayoutTests/inspector/profiler/heap-snapshot-test.js:423
> +                            InspectorTest.HeapNode.hidden,

We should make sure the types here are ordered as their values above. Please either use
 node_types[this._nodeTypes[n]] = n
to fill this array or use this._nodeTypes[n] = node_types.indexOf(n) to initialize type constants.

> LayoutTests/inspector/profiler/heap-snapshot-test.js:496
> +        this._strings2id[string] = this._nextStringId++;

Can this._strings.length be used as id?

> LayoutTests/inspector/profiler/heap-snapshot-test.js:510
> +InspectorTest.HeapNode.hidden = "hidden";

Should be InspectorTest.HeapNode.Type = {
"hidden": "hidden",...
}

> LayoutTests/inspector/profiler/heap-snapshot-test.js:532
> +        if (nameOrIndex in this._edges)

this._edges is an array of InspectorTest.HeapEdge instances while nameOrIndex is a string or number. The condition looks wrong.

> LayoutTests/inspector/profiler/heap-snapshot-test.js:550
> +        rawSnapshot.nodes.push(this._ordinal * 2 + 1);    // id

You should add a comment explaining why the ids should be odd.

> LayoutTests/inspector/profiler/heap-snapshot-test.js:580
> +InspectorTest.HeapEdge.context = "context";

InspectorTest.HeapEdge.Type

> LayoutTests/inspector/profiler/heap-snapshot-test.js:587
> +InspectorTest.createFlagsTestSnapshot = function(instanceCount)

Consider putting this into the test file as it is used only once.

[Ilya Tikhonovsky]

Created attachment 143029 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 143030 [details]
Patch

[Ilya Tikhonovsky]

View in context: https://bugs.webkit.org/attachment.cgi?id=143007&action=review

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:384
>> +    this._strings2id = {};
> 
> _strings2id -> _string2id

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:423
>> +                            InspectorTest.HeapNode.hidden,
> 
> We should make sure the types here are ordered as their values above. Please either use
>  node_types[this._nodeTypes[n]] = n
> to fill this array or use this._nodeTypes[n] = node_types.indexOf(n) to initialize type constants.

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:496
>> +        this._strings2id[string] = this._nextStringId++;
> 
> Can this._strings.length be used as id?

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:510
>> +InspectorTest.HeapNode.hidden = "hidden";
> 
> Should be InspectorTest.HeapNode.Type = {
> "hidden": "hidden",...
> }

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:532
>> +        if (nameOrIndex in this._edges)
> 
> this._edges is an array of InspectorTest.HeapEdge instances while nameOrIndex is a string or number. The condition looks wrong.

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:550
>> +        rawSnapshot.nodes.push(this._ordinal * 2 + 1);    // id
> 
> You should add a comment explaining why the ids should be odd.

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:580
>> +InspectorTest.HeapEdge.context = "context";
> 
> InspectorTest.HeapEdge.Type

done

>> LayoutTests/inspector/profiler/heap-snapshot-test.js:587
>> +InspectorTest.createFlagsTestSnapshot = function(instanceCount)
> 
> Consider putting this into the test file as it is used only once.

done

[Yury Semikhatsky]

Comment on attachment 143030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143030&action=review

> LayoutTests/inspector/profiler/heap-snapshot-test.js:385
> +    this._size = size || 0;

_size => _selfSize?

> LayoutTests/inspector/profiler/heap-snapshot-test.js:434
> +        rawSnapshot.nodes.push(this._ordinal * 2 + 1);    // id has to be odd for the consistency with backend implementation.

Please state explicitly that we differentiate between JS nodes and DOM groups by the oddity of the id.

> LayoutTests/inspector/profiler/heap-snapshot-test.js:447
> +    this._node = node;

_node => _targetNode

> LayoutTests/inspector/profiler/heap-snapshot.html:135
> +	    debuggerNode.linkNode(debuggerOwnedNode, InspectorTest.HeapEdge.Type.element);

Wrong alignment.

[Ilya Tikhonovsky]

Committed r117780: <http://trac.webkit.org/changeset/117780>