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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=194448

Attachment 361533: [PATCH] Proposed Fix

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




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

r=me

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
snapshot");

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
> +	  
this._importButtonNavigationItem.addEventListener(WI.ButtonNavigationItem.Event
.Clicked, this._importButtonNavigationItemClicked, this);

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

> 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
%s.%s.%s").format(...values);

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

Why?

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

>
Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshotWorker.js:
37
> +	   this._importedSnapshots = [];

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