| Summary: | Web Inspector: introduce a helper for HeapSnapshot post-processing tests. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ilya Tikhonovsky <loislo> | ||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 87089 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 143007 [details]
Patch
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. Created attachment 143029 [details]
Patch
Created attachment 143030 [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 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 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. Committed r117780: <http://trac.webkit.org/changeset/117780> |
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();