DevTools: Add ability to go to paint record and paint profiler in Timeline from layers view.

Add ability to go to paint record and paint profiler in Timeline from layers view.

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

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

[malch, 2014-07-17 12:35:23]

Please take a look, thank you!

[caseq, 2014-07-17 13:12:22]

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/Layers3DView.js (right):

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:154:
setSelectPaintEventCallback: function(callback)
Why isn't this an event?

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:662:
contextMenu.appendItem("Paint event", this._selectPaintEventCallback.bind(this,
activeObject.traceEvent), false);
Use WebInspector.UIString() around UI strings (also, please fix the one above).
Also, it's an action -- there should be some verb. "Jump to paint event"?

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:707:
this._selectPaintEventCallback.call(this, object.traceEvent);
Why are we invoking the client callback with `this' set to our object?

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:1009:
WebInspector.Layers3DView.ActiveObject.layerActiveObject = function(layer)
Here and below, since these create objects, let's add either "create" or "from"
prefix.

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineLayersView.js (right):

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineLayersView.js:49:
setSelectPaintEventCallback: function(callback)
I'd rather pass TimelineModeViewDelegate here, it has select().

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:290: _selectPaintEvent:
function(traceEvent)
Why is is specific to paint?

[malch, 2014-07-17 15:17:45]

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/Layers3DView.js (right):

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:154:
setSelectPaintEventCallback: function(callback)
On 2014/07/17 13:12:22, caseq wrote:
> Why isn't this an event?

Done.

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:662:
contextMenu.appendItem("Paint event", this._selectPaintEventCallback.bind(this,
activeObject.traceEvent), false);
On 2014/07/17 13:12:22, caseq wrote:
> Use WebInspector.UIString() around UI strings (also, please fix the one
above).
> Also, it's an action -- there should be some verb. "Jump to paint event"?

Done.

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:707:
this._selectPaintEventCallback.call(this, object.traceEvent);
On 2014/07/17 13:12:22, caseq wrote:
> Why are we invoking the client callback with `this' set to our object?

Acknowledged.

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:1009:
WebInspector.Layers3DView.ActiveObject.layerActiveObject = function(layer)
On 2014/07/17 13:12:22, caseq wrote:
> Here and below, since these create objects, let's add either "create" or
"from"
> prefix.

Done.

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineLayersView.js (right):

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineLayersView.js:49:
setSelectPaintEventCallback: function(callback)
On 2014/07/17 13:12:22, caseq wrote:
> I'd rather pass TimelineModeViewDelegate here, it has select().

Done.

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/400643002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:290: _selectPaintEvent:
function(traceEvent)
On 2014/07/17 13:12:22, caseq wrote:
> Why is is specific to paint?

Done.

[caseq, 2014-07-17 16:10:03]

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/Layers3DView.js (right):

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:985:
WebInspector.Layers3DView.ActiveObject = function()
nit: we probably need to be a more descriptive name than ActiveObject.

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:290: _selectPaintEvent:
function(traceEvent)
do we need it now?

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:318:
this._lazyLayersView.setTimelineModelAndDelegate(this._model, this);
ditto

[malch, 2014-07-17 16:20:32]

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/Layers3DView.js (right):

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:985:
WebInspector.Layers3DView.ActiveObject = function()
Will fix this when I come with a better name.
On 2014/07/17 16:10:02, caseq wrote:
> nit: we probably need to be a more descriptive name than ActiveObject.

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:290: _selectPaintEvent:
function(traceEvent)
On 2014/07/17 16:10:02, caseq wrote:
> do we need it now?

Done.

https://codereview.chromium.org/400643002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:318:
this._lazyLayersView.setTimelineModelAndDelegate(this._model, this);
Need this, as discussed offline.
On 2014/07/17 16:10:02, caseq wrote:
> ditto

[caseq, 2014-07-17 16:24:12]

lgtm

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/Layers3DView.js (right):

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:655:
contextMenu.appendItem(WebInspector.UIString("Jump to paint event"),
this.dispatchEventToListeners.bind(this,
WebInspector.Layers3DView.Events.JumpToPaintEventRequested,
activeObject.traceEvent), false);
we capitalize every word in UI commands (except articles/proverbs etc), i.e.
this should be Reset View and Jump to Paint Event

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineLayersView.js (right):

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineLayersView.js:63: var eventRecord =
undefined;
nit: drop undefined

[malch, 2014-07-17 16:29:46]

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/Layers3DView.js (right):

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/Layers3DView.js:655:
contextMenu.appendItem(WebInspector.UIString("Jump to paint event"),
this.dispatchEventToListeners.bind(this,
WebInspector.Layers3DView.Events.JumpToPaintEventRequested,
activeObject.traceEvent), false);
On 2014/07/17 16:24:12, caseq wrote:
> we capitalize every word in UI commands (except articles/proverbs etc), i.e.
> this should be Reset View and Jump to Paint Event

Done.

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineLayersView.js (right):

https://codereview.chromium.org/400643002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineLayersView.js:63: var eventRecord =
undefined;
On 2014/07/17 16:24:12, caseq wrote:
> nit: drop undefined

Done.

[yurys, 2014-07-18 11:32:11]

lgtm

https://codereview.chromium.org/400643002/diff/60002/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineLayersView.js (right):

https://codereview.chromium.org/400643002/diff/60002/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineLayersView.js:65: function
findRecordWithEvent(record)
Please add annotations.

[malch, 2014-07-18 11:41:36]

https://codereview.chromium.org/400643002/diff/60002/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineLayersView.js (right):

https://codereview.chromium.org/400643002/diff/60002/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineLayersView.js:65: function
findRecordWithEvent(record)
Done, thanks!

On 2014/07/18 11:32:11, yurys wrote:
> Please add annotations.

[caseq, 2014-07-21 13:49:05]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-07-21 13:49:36]

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

[commit-bot: I haz the power, 2014-07-21 14:58:48]

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

[commit-bot: I haz the power, 2014-07-21 15:35:17]

Change committed as 178565