[webkit-reviews] review granted: [Bug 194448] Web Inspector: Import / Export Heap Snapshots : [Attachment 361533] [PATCH] Proposed Fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 8 14:35:10 PST 2019

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 194448: Web Inspector: Import / Export Heap Snapshots

Attachment 361533: [PATCH] Proposed Fix


--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 361533
  --> https://bugs.webkit.org/attachment.cgi?id=361533
[PATCH] Proposed Fix

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


Can we test this functionality?  I'd imagine it would go something like this:
1. take a heap snapshot
2. export it
3. test that the exported string from #2 matches the expected format/structure
(to some degree of accuracy)
4. import the exported string from #2 (this may require a bit of refactoring so
that you wouldn't have to call `WI.FileUtilities.importText`)
5. check that the imported heap snapshot from #4 and the original heap snapshot
from #1 match (to some degree of accuracy)

> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotProxy.js:86
> +    set snapshotStringData(x)

NIT: is `x` really the best name? 😂

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:62
> +	   this._importButtonNavigationItem = new
WI.ButtonNavigationItem("import", WI.UIString("Import"), "Images/Import.svg",
15, 15);

What about drag/drop?  I think we'd want to limit it to only working when the
"Heap Allocations" view is shown, but it could also work anywhere in the
"Timelines" tab and forcibly show the "Heap Allocations" view when a heap
snapshot is imported.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:63
> +	   this._importButtonNavigationItem.toolTip = WI.UIString("Import

Our usual style is to have the tooltip match the text shown.  As such, this
should also be `WI.UIString("Import")`.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:65
> +	  
.Clicked, this._importButtonNavigationItemClicked, this);

NIT: I prefer to prefix all event listeners with `handle`, so this would be

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:408
> +	   WI.FileUtilities.importText(function(result) {

NIT: arrow function?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:414
> +		   const timestamp = NaN;

Style: add a newline before this.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotContentView.js:115
> +	   let filename = WI.UIString("Heap Snapshot %s-%s-%s at

If only <https://webkit.org/b/194279> has landed, so that you'd have this
functionality on `WI.FileUtilities` :(

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:-150
> -    // Protected


> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:82
> +	   this._imported = imported;

Should we add assertions for any functions that don't make sense when imported
(e.g. `updateDeadNodesAndGatherCollectionData`)?

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:89
> +	   console.assert(!type || (type === "Inspector" || type ===
"GCDebugging"), "Expect an Inspector / GCDebugging Heap Snapshot");

NIT: are the parenthesis necessary, or more for clarity when reading?

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:91
> +	   this.nodeFieldCount = nodeFieldCount;

This value isn't used outside this class, so I'd make it "private" (e.g.
`_nodeFieldCount`).  The static functions are also "part" of this class, so I
think it's fine for them to access this value.

NIT: please put this in an `else`, so we aren't assigning twice in the
"GCDebugging" case.

> +	   this._importedSnapshots = [];

Is there actually any benefit to keeping a list of imported snapshots?

More information about the webkit-reviews mailing list