[DevTools] Send heap snapshot to the frontend immediatly when it is ready

Send heap snapshot to the frontend immediatly when it is ready

HeapProfiler.takeHeapSnapshot command is now handled in the following way:

Profiler starts taking snapshot immediately when the command arrives (as before) and if reportProgress param is true it will send HeapProfiler.reportHeapSnapshotProgress events. After that a series of snapshot chunks is sent to the frontend as HeapProfiler.addHeapSnapshotChunk events. When whole snapshot is sent the backend will sent response to HeapProfiler.takeHeapSnapshot command.

The snapshot is going to be deleted automatically after it has been sent to the front-end. This means that all commands that require snapshot uid parameter will be removed from the protocol as there will be no way to find snapshot by uid on the backend.

BUG=324769
R=alph@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166131

[alph, 2013-12-23 14:06:58]

lgtm

https://codereview.chromium.org/98273008/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorHeapProfilerAgent.cpp (right):

https://codereview.chromium.org/98273008/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorHeapProfilerAgent.cpp:276:
m_frontend->reportHeapSnapshotProgress(m_totalWork, m_totalWork, &finished);
Do you really need to add this parameter?
Can't it just assume the snapshotting is finished when workDone equals
totalWork?

https://codereview.chromium.org/98273008/diff/1/Source/devtools/front_end/Hea...
File Source/devtools/front_end/HeapSnapshotView.js (right):

https://codereview.chromium.org/98273008/diff/1/Source/devtools/front_end/Hea...
Source/devtools/front_end/HeapSnapshotView.js:1122: * @this
{WebInspector.HeapSnapshotProfileType}
WebInspector.TrackingHeapSnapshotProfileType

https://codereview.chromium.org/98273008/diff/1/Source/devtools/front_end/Hea...
Source/devtools/front_end/HeapSnapshotView.js:1124: function
didTakeHeapSnapshot(error)
It seems to have a bunch of similar code to its friend from
HeapSnapshotProfileType. Perhaps it's worth to extract the common part. wdyt?

https://codereview.chromium.org/98273008/diff/1/Source/devtools/front_end/Hea...
Source/devtools/front_end/HeapSnapshotView.js:1130:
WebInspector.log("didTakeHeapSnapshot " + this._profileBeingRecorded);
is that intended?

https://codereview.chromium.org/98273008/diff/1/Source/devtools/front_end/Hea...
Source/devtools/front_end/HeapSnapshotView.js:1134:
WebInspector.panels.profiles._showProfile(profile);
Event broadcasting would look more elegant here. wdyt?

[yurys, 2013-12-23 14:53:03]

@qyearsley, @kkania, @marja: this change and subsequent removal of commands like
HeapProfiler.getHeapSnapshot will require updating chromedriver, telemetry and
jsleakchecker.

[marja, 2014-01-07 08:47:30]

On 2013/12/23 14:53:03, yurys wrote:
> @qyearsley, @kkania, @marja: this change and subsequent removal of commands
like
> HeapProfiler.getHeapSnapshot will require updating chromedriver, telemetry and
> jsleakchecker.

I guess this change will necessarily break jsleakcheck for some versions; after
remote_inspector_client.py is updated, it'll then only work with the new
version, not working with old chrome versions any more. Oh well, it's not like
jsleakcheck is used that much anyway.

[yurys, 2014-01-10 12:58:31]

On 2014/01/07 08:47:30, marja wrote:
> On 2013/12/23 14:53:03, yurys wrote:
> > @qyearsley, @kkania, @marja: this change and subsequent removal of commands
> like
> > HeapProfiler.getHeapSnapshot will require updating chromedriver, telemetry
and
> > jsleakchecker.
> 
> I guess this change will necessarily break jsleakcheck for some versions;
after
> remote_inspector_client.py is updated, it'll then only work with the new
> version, not working with old chrome versions any more. Oh well, it's not like
> jsleakcheck is used that much anyway.

remote_inspector_client.py could decide on the supported protocol based on the
browser version and support both command sets for some time if we need to be
backward compatible for some time.

[marja, 2014-01-15 09:04:17]

Yeah, that would work. Btw, I'm not sure if there are some tests which run
against the reference build and use remote_inspector_client.py, which would
break if it cannot handle the reference build any more. If it's only
jsleakcheck, I won't be objecting the breakage.

[yurys, 2014-01-29 13:59:09]

PTAL

[kkania, 2014-01-29 16:45:30]

-kkania, +stgao

[qyearsley, 2014-01-29 23:21:38]

Question: Is this change in HeapProfiler.takeHeapSnapshot related to the removal
of Memory.getDOMCounters or its subsequent replacement by Timeline.start with
includeDomCounters=true?

Also: It looks like Telemetry is using HeapProfiler.takeHeapSnapshot and
HeapProfiler.getHeapSnapshot, but there are currently no Telemetry tests that
are using that functionality.
Link to the part of Telemetry that uses HeapProfiler:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te....

[stgao, 2014-01-30 00:18:05]

-kkania
+siddharthab

The change of devtools protocol for heap snapshot looks good to me.

Is there a plan for removal of Memory.getDOMCounters?
Is there an email thread of chromium bug that we could follow up?
I remember siddharthab@ worked on a CL to use this method.
(https://codereview.chromium.org/34773002/)

[yurys, 2014-01-30 08:44:30]

On 2014/01/29 23:21:38, qyearsley wrote:
> Question: Is this change in HeapProfiler.takeHeapSnapshot related to the
removal
> of Memory.getDOMCounters or its subsequent replacement by Timeline.start with
> includeDomCounters=true?
> 
No, this is a different story. This change affects only HeapProfiler domain.

> Also: It looks like Telemetry is using HeapProfiler.takeHeapSnapshot and
> HeapProfiler.getHeapSnapshot, but there are currently no Telemetry tests that
> are using that functionality.
> Link to the part of Telemetry that uses HeapProfiler:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te....

Correct. This is addressed in another patch:
https://codereview.chromium.org/141553008/

[yurys, 2014-01-30 09:02:55]

On 2014/01/30 00:18:05, Shuotao wrote:
> -kkania
> +siddharthab
> 
> The change of devtools protocol for heap snapshot looks good to me.
> 
> Is there a plan for removal of Memory.getDOMCounters?
We would like to drop this command and the whole Memory domain as it is not used
by the DevTools front-end. It is not a pressing issue though and there is no
ongoing work in that direction.

> Is there an email thread of chromium bug that we could follow up?
I've just filed a bug to track this: crbug.com/339386 (There seems to have been
a problem adding siddharthab@ to CC list).

> I remember siddharthab@ worked on a CL to use this method.
> (https://codereview.chromium.org/34773002/)

[commit-bot: I haz the power, 2014-01-30 09:10:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yurys@chromium.org/98273008/110001

[commit-bot: I haz the power, 2014-01-30 11:46:42]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 11:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yurys@chromium.org/98273008/130001

[commit-bot: I haz the power, 2014-01-30 14:46:32]

CQ bit was unchecked on CL. Ignoring.

[yurys, 2014-01-30 14:49:59]

Committed patchset #4 manually as r166131 (presubmit successful).