[webkit-reviews] review granted: [Bug 87642] Web Inspector: REGRESSION: load heap snapshot doesn't work : [Attachment 144469] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 87642: Web Inspector: REGRESSION: load heap snapshot doesn't work
https://bugs.webkit.org/show_bug.cgi?id=87642

Attachment 144469: Patch
https://bugs.webkit.org/attachment.cgi?id=144469&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144469&action=review


> Source/WebCore/ChangeLog:7
> +	   It declares the API for HSLoader, HSLoaderProxy and
HSSaveToFileReceiver.

Please don't abbreviate, use HeapSnapshotLoader etc.

> Source/WebCore/inspector/front-end/HeapSnapshotLoader.js:52
> +	   this._reset();

It looks as if you were going to reuse the object after dispose.

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:839
> +    this._loader = null;

_loader -> _receiver

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:936
> +	   this.sidebarElement.subtitle =
Number.bytesToString(this._snapshotProxy.totalSize);

This will fail if _snapshotProxy is null.

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:1016
> +	   this._file = file;

All file loading stuff can be extracted into a separate entity.

> LayoutTests/inspector/profiler/heap-snapshot-loader.html:42
> +		   maxJSObjectId: 6

Can you extract this value from the snapshot mock?