DevTools: Add snapshot command log on the frontend.

Add snapshot command log on the frontend.

Review URL: https://codereview.chromium.org/319603007

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

[malch, 2014-06-05 05:14:59]

Please take a look.

[caseq, 2014-06-05 06:00:52]

https://codereview.chromium.org/319603007/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorLayerTreeAgent.h (right):

https://codereview.chromium.org/319603007/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorLayerTreeAgent.h:81: virtual void
snapshotCommandLog(ErrorString*, const String& snapshotId, const int* fromStep,
const int* toStep, RefPtr<TypeBuilder::Array<JSONObject> >&) OVERRIDE;
we can select necessary step range on the front-end instead.

[malch, 2014-06-05 07:36:45]

https://codereview.chromium.org/319603007/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorLayerTreeAgent.h (right):

https://codereview.chromium.org/319603007/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorLayerTreeAgent.h:81: virtual void
snapshotCommandLog(ErrorString*, const String& snapshotId, const int* fromStep,
const int* toStep, RefPtr<TypeBuilder::Array<JSONObject> >&) OVERRIDE;
On 2014/06/05 06:00:53, caseq wrote:
> we can select necessary step range on the front-end instead.

Done.

[caseq, 2014-06-05 11:14:24]

Mostly looks good, but let's not abuse Panel.

https://codereview.chromium.org/319603007/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/layers/PaintProfilerView.js (right):

https://codereview.chromium.org/319603007/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:35: * @extends
{WebInspector.PanelWithSidebarTree}
You can't make it a panel, since it's not really a panel. Please make it a split
view instead.

https://codereview.chromium.org/319603007/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:261: var logItem =
(this.representedObject);
remove redundant brackets

https://codereview.chromium.org/319603007/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContextRecorder.cpp (left):

https://codereview.chromium.org/319603007/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContextRecorder.cpp:167: class
LoggingSnapshotPlayer : public SnapshotPlayer {
Let's move that into a separate patch.

[malch, 2014-06-05 12:41:09]

https://codereview.chromium.org/319603007/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/layers/PaintProfilerView.js (right):

https://codereview.chromium.org/319603007/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:35: * @extends
{WebInspector.PanelWithSidebarTree}
On 2014/06/05 11:14:24, caseq wrote:
> You can't make it a panel, since it's not really a panel. Please make it a
split
> view instead.

Done.

https://codereview.chromium.org/319603007/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:261: var logItem =
(this.representedObject);
On 2014/06/05 11:14:24, caseq wrote:
> remove redundant brackets

Done.

https://codereview.chromium.org/319603007/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContextRecorder.cpp (left):

https://codereview.chromium.org/319603007/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContextRecorder.cpp:167: class
LoggingSnapshotPlayer : public SnapshotPlayer {
On 2014/06/05 11:14:24, caseq wrote:
> Let's move that into a separate patch.

Done.

[caseq, 2014-06-05 16:25:39]

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/layers/PaintProfilerView.js (right):

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:175: function
onProfileDone(snapshot, profiles)
You don't need to pass snapshot, it's already available as this._snapshot.
Another option to implement this is to start both profiling and getting log
independently and use a barrier, i.e.

snapshot.profile(barrier.createCallback(onProfileDone.bind(this)))
snapshot.commandLog(barrier.createCallback(onCommandLogDone.bind(this)))
barrier.callWhenDone(this._update.bind(this))

This is slightly more readable and faster.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:198: * @param {!Element}
sidebarElement
remove this.
rather than passing parent element to the constructor, we normally just call
show(parent) at the creation site (i.e. in the client code).

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:207: this._sidebarElement
= this.element;
Why do you need it? Just use this.element.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:208:
this._sidebarElement.classList.add("sidebar", "outline-disclosure");
Not sure if we need "sidebar" class here.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:292: title.textContent =
logItem.method;
let's build complete text first, then assign it to title.textContent.

[malch, 2014-06-09 05:59:45]

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/layers/PaintProfilerView.js (right):

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:175: function
onProfileDone(snapshot, profiles)
On 2014/06/05 16:25:40, caseq wrote:
> You don't need to pass snapshot, it's already available as this._snapshot.
> Another option to implement this is to start both profiling and getting log
> independently and use a barrier, i.e.
> 
> snapshot.profile(barrier.createCallback(onProfileDone.bind(this)))
> snapshot.commandLog(barrier.createCallback(onCommandLogDone.bind(this)))
> barrier.callWhenDone(this._update.bind(this))
> 
> This is slightly more readable and faster.

Done.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:198: * @param {!Element}
sidebarElement
On 2014/06/05 16:25:40, caseq wrote:
> remove this.
> rather than passing parent element to the constructor, we normally just call
> show(parent) at the creation site (i.e. in the client code).

Done.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:207: this._sidebarElement
= this.element;
On 2014/06/05 16:25:40, caseq wrote:
> Why do you need it? Just use this.element.

Done.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:208:
this._sidebarElement.classList.add("sidebar", "outline-disclosure");
On 2014/06/05 16:25:40, caseq wrote:
> Not sure if we need "sidebar" class here.

Done.

https://codereview.chromium.org/319603007/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:292: title.textContent =
logItem.method;
On 2014/06/05 16:25:40, caseq wrote:
> let's build complete text first, then assign it to title.textContent.

Done.

[caseq, 2014-06-09 06:13:09]

lgtm

https://codereview.chromium.org/319603007/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/layers/PaintProfilerView.js (right):

https://codereview.chromium.org/319603007/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:201:
WebInspector.LogTreeView = function()
nit: let's give it a more specific name, like PaintProfilerCommandLogView

[malch, 2014-06-09 06:20:45]

https://codereview.chromium.org/319603007/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/layers/PaintProfilerView.js (right):

https://codereview.chromium.org/319603007/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/layers/PaintProfilerView.js:201:
WebInspector.LogTreeView = function()
On 2014/06/09 06:13:08, caseq wrote:
> nit: let's give it a more specific name, like PaintProfilerCommandLogView

Done.

[caseq, 2014-06-09 06:23:50]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-06-09 06:24:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/malch@chromium.org/319603007/80001

[commit-bot: I haz the power, 2014-06-09 07:33:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-06-09 08:15:25]

Change committed as 175780