Bug 85595 – Web Inspector: save HeapSnapshot draft implementation

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

[Ilya Tikhonovsky]

We would like to save HeapSnapshots.
The simplest solution for now is to force the back-end to push the snapshot to the front-end second time
and persist it to the disk with help of save and append operations of the embedder.

[Ilya Tikhonovsky]

Created attachment 140185 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 140185 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2541
> +                        "name": "typeId",

Please put all properties in of an entry on one line as in the rest code.

> Source/WebCore/inspector/Inspector.json:2547
> +                        "name": "title",

This part of change may well be done in a separate CL.

> Source/WebCore/inspector/Inspector.json:2572
> +                        "optional": true

Add a description?

> Source/WebCore/inspector/Inspector.json:2577
> +                        "optional": true

Add a description?

> Source/WebCore/inspector/front-end/DebuggerModel.js:47
> +    this._scriptsWithSourceURL = {};

Look like a part of a different change. Please revert.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:149
> +        if (this._saveOperationInProgress) {

What if "save profile" is invoked before the profile load has completed?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:153
> +        if (!this.proxy)

Where does this proxy property come from?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:172
> +            this._saveToFile = fileName;

_saveToFile -> _fileName?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:525
> +    temporaryProfile: function(typeId)

temporaryProfile -> findTemporaryProfile

> Source/WebCore/inspector/front-end/ProfilesPanel.js:927
> +        if (!!temporaryProfile !== isProfiling) {

Prefer early return.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1072
> +        contextMenu.appendItem(WebInspector.UIString("Save profile"), profile.save.bind(profile));

localizedStrings.js change is missing

> Source/WebCore/inspector/front-end/inspector.js:1043
> +    WebInspector.saveCallbacks[url] = callback;

Can callback be null? Why saveCallbacks is not private? What should happen to the callback if save is invoked several times with same URL?

> Source/WebCore/inspector/front-end/inspector.js:1065
> +WebInspector.appendCallbacks = [];

Should be private

[WebKit Review Bot]

Comment on attachment 140185 [details]
Patch

Attachment 140185 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12634124

New failing tests:
inspector/profiler/heap-snapshot-comparison-expansion-preserved-when-sorting.html
inspector/profiler/heap-snapshot-summary-show-all.html
inspector/profiler/heap-snapshot-dominators-shown-node-count-preserved-when-sorting.html
inspector/profiler/heap-snapshot-dominators-expansion-preserved-when-sorting.html
http/tests/security/sandboxed-iframe-modify-self.html
fast/frames/cached-frame-counter.html
accessibility/aria-disabled.html
fast/loader/text-document-wrapping.html
inspector/profiler/heap-snapshot-summary-sorting.html
inspector/profiler/heap-snapshot-comparison-sorting.html
inspector/profiler/heap-snapshot-dominators-show-all.html
inspector/profiler/heap-snapshot-summary-expansion-preserved-when-sorting.html
inspector/profiler/heap-snapshot-dominators-show-next.html
inspector/profiler/heap-snapshot-comparison-shown-node-count-preserved-when-sorting.html
fast/canvas/webgl/shader-precision-format.html
inspector/profiler/heap-snapshot-comparison-show-next.html
inspector/profiler/heap-snapshot-comparison-show-all.html
inspector/profiler/heap-snapshot-containment-expansion-preserved-when-sorting.html
inspector/profiler/heap-snapshot-dominators-sorting.html
inspector/profiler/heap-snapshot-comparison-dom-groups-change.html
inspector/profiler/heap-snapshot-containment-sorting.html
inspector/profiler/heap-snapshot-containment-show-all.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
inspector/profiler/heap-snapshot-containment-show-next.html
inspector/profiler/heap-snapshot-summary-show-next.html
fast/loader/javascript-url-in-embed.html
inspector/profiler/heap-snapshot-summary-shown-node-count-preserved-when-sorting.html
inspector/profiler/heap-snapshot-containment-shown-node-count-preserved-when-sorting.html

[WebKit Review Bot]

Created attachment 140217 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ilya Tikhonovsky]

Created attachment 140384 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 140386 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 140386 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:794
> +            return;

We should invoke the callback here.

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:795
> +        if (!this.proxy) {

Why is the proxy field public?

> Source/WebCore/inspector/front-end/HeapSnapshotView.js:830
> +        else {

It should go on the previous line.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1108
> +        if (!profile.canSave || !profile.canSave())

Double check to be sure?

> Source/WebCore/inspector/front-end/inspector.js:1044
> +        WebInspector._saveCallbacks[url] = callback;

Please add and assert here if we cannot override existing callback.

[Ilya Tikhonovsky]

(In reply to comment #2)
> (From update of attachment 140185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140185&action=review
> 
> > Source/WebCore/inspector/Inspector.json:2541
> > +                        "name": "typeId",
> 
> Please put all properties in of an entry on one line as in the rest code.

done in separate patch

> 
> > Source/WebCore/inspector/Inspector.json:2547
> > +                        "name": "title",
> 
> This part of change may well be done in a separate CL.

done in separate patch

> 
> > Source/WebCore/inspector/Inspector.json:2572
> > +                        "optional": true
> 
> Add a description?

I'll do it in separate patch

> 
> > Source/WebCore/inspector/Inspector.json:2577
> > +                        "optional": true
> 
> Add a description?

I'll do it in separate patch

> 
> > Source/WebCore/inspector/front-end/DebuggerModel.js:47
> > +    this._scriptsWithSourceURL = {};
> 
> Look like a part of a different change. Please revert.

it was obsolete change.

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:149
> > +        if (this._saveOperationInProgress) {
> 
> What if "save profile" is invoked before the profile load has completed?

done

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:153
> > +        if (!this.proxy)
> 
> Where does this proxy property come from?

It was done in a method of ProfilesPanel. Now it is lazily initialized in HeapProfileHeader.load.

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:172
> > +            this._saveToFile = fileName;
> 
> _saveToFile -> _fileName?

done

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:525
> > +    temporaryProfile: function(typeId)
> 
> temporaryProfile -> findTemporaryProfile

done in separate patch

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:927
> > +        if (!!temporaryProfile !== isProfiling) {
> 
> Prefer early return.
done in separate patch

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:1072
> > +        contextMenu.appendItem(WebInspector.UIString("Save profile"), profile.save.bind(profile));
> 
> localizedStrings.js change is missing
> 
> > Source/WebCore/inspector/front-end/inspector.js:1043
> > +    WebInspector.saveCallbacks[url] = callback;
> 
> Can callback be null? Why saveCallbacks is not private? What should happen to the callback if save is invoked several times with same URL?

done

> 
> > Source/WebCore/inspector/front-end/inspector.js:1065
> > +WebInspector.appendCallbacks = [];
> 
> Should be private

done

[Ilya Tikhonovsky]

Committed r116218: <http://trac.webkit.org/changeset/116218>

[Ilya Tikhonovsky]

(In reply to comment #7)
> (From update of attachment 140386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140386&action=review
> 
> > Source/WebCore/inspector/front-end/HeapSnapshotView.js:794
> > +            return;
> 
> We should invoke the callback here.

done

> 
> > Source/WebCore/inspector/front-end/HeapSnapshotView.js:795
> > +        if (!this.proxy) {
> 
> Why is the proxy field public?

done

> 
> > Source/WebCore/inspector/front-end/HeapSnapshotView.js:830
> > +        else {
> 
> It should go on the previous line.

done 

> 
> > Source/WebCore/inspector/front-end/ProfilesPanel.js:1108
> > +        if (!profile.canSave || !profile.canSave())
> 
> Double check to be sure?

This function exists only in HeapProfileHeader. I'd change this in a separate patch.

> 
> > Source/WebCore/inspector/front-end/inspector.js:1044
> > +        WebInspector._saveCallbacks[url] = callback;
> 
> Please add and assert here if we cannot override existing callback.

Such assert will prevent the second save operation if the first one failed because we have no notifications for unsuccessful saves. I'll fix this in a separate patch.