Draw marker events on Timeline flame chart

Draw marker events on Timeline flame chart

Marker events are now displayed on the flame chart's timeline bar.

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

[loislo, 2014-07-21 11:40:19]

On 2014/07/21 09:42:43, yurys wrote:

Looks like we are mixing two design styles here.
We manage the entries via indexes but we use array of marker objects with colors
etc.

I'd prefer to work with markers the same way as we do with entries.

[alph, 2014-07-21 12:05:30]

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/FlameChart.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:639: var accurracyOffset = 1;
nit: accuracyOffsetPx

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:654: var nextDistance =
nextMarker.timestamp - time;
abs?

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:656: marker = nextMarker;
distance = nextDistance

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:812:
WebInspector.TimelineGrid.drawTimelineMarkers(this._canvas, this._calculator,
this._markers);
I'm a bit concerned that FlameChart uses TimelineGrid *and* TimelineGrid uses
FlameChartDP.Marker
Can we eliminate the cross dependency?

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:828: if (barX ===
this._canvas.width)
This comparison looks suspicious. Is it a kinda special value?

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/TimelineGrid.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/TimelineGrid.js:170: context.lineWidth = 2;
it should depend on devicePixelRatio

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/TimelineGrid.js:171: context.translate(0.5,
0.5);
You need to translate by 0.5 iff lineWidth is odd.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TimelineFlameChart.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TimelineFlameChart.js:195: if (!e.endTime &&
e.phase !== WebInspector.TracingModel.Phase.Instant && e.phase !== "I")
 && e.phase !== "I" seems to be not needed anymore.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:297: if (eventName
=== recordTypes.MarkDOMContent)
switch?

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:305: //else if
(eventName === recordTypes.BeginFrame)
nuke?

[yurys, 2014-07-21 13:49:36]

On 2014/07/21 11:40:19, loislo wrote:
> On 2014/07/21 09:42:43, yurys wrote:
> 
> Looks like we are mixing two design styles here.
> We manage the entries via indexes but we use array of marker objects with
colors
> etc.
> 
> I'd prefer to work with markers the same way as we do with entries.

Done.

[yurys, 2014-07-21 13:49:48]

PTAL

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/FlameChart.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:639: var accurracyOffset = 1;
On 2014/07/21 12:05:30, alph wrote:
> nit: accuracyOffsetPx

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:654: var nextDistance =
nextMarker.timestamp - time;
On 2014/07/21 12:05:30, alph wrote:
> abs?

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:656: marker = nextMarker;
On 2014/07/21 12:05:30, alph wrote:
> distance = nextDistance

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:812:
WebInspector.TimelineGrid.drawTimelineMarkers(this._canvas, this._calculator,
this._markers);
On 2014/07/21 12:05:30, alph wrote:
> I'm a bit concerned that FlameChart uses TimelineGrid *and* TimelineGrid uses
> FlameChartDP.Marker
> Can we eliminate the cross dependency?

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:828: if (barX ===
this._canvas.width)
On 2014/07/21 12:05:30, alph wrote:
> This comparison looks suspicious. Is it a kinda special value?

Removed.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/components/TimelineGrid.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/TimelineGrid.js:170: context.lineWidth = 2;
On 2014/07/21 12:05:30, alph wrote:
> it should depend on devicePixelRatio

There is already a call to context.scale above.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/TimelineGrid.js:171: context.translate(0.5,
0.5);
On 2014/07/21 12:05:30, alph wrote:
> You need to translate by 0.5 iff lineWidth is odd.

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TimelineFlameChart.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TimelineFlameChart.js:195: if (!e.endTime &&
e.phase !== WebInspector.TracingModel.Phase.Instant && e.phase !== "I")
On 2014/07/21 12:05:30, alph wrote:
>  && e.phase !== "I" seems to be not needed anymore.

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:297: if (eventName
=== recordTypes.MarkDOMContent)
On 2014/07/21 12:05:30, alph wrote:
> switch?

Done.

https://codereview.chromium.org/402113002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:305: //else if
(eventName === recordTypes.BeginFrame)
On 2014/07/21 12:05:30, alph wrote:
> nuke?

Done.

[alph, 2014-07-21 14:21:07]

lgtm w/ comments.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/components/FlameChart.js (right):

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/components/FlameChart.js:661: if (markers[i] >
rightTime)
nit: you can move the check into the for condition.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/components/FlameChart.js:840: var right =
markerTimestamps.upperBound(this._calculator.maximumBoundary(), compare);
nit: you don't really need to calculate it. you can do it the same way as in
_markerIndexAtPosition

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/flameChart.css (right):

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/flameChart.css:51: top: 0px;
no need for px suffix.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/flameChart.css:52: height: 20px;
i don't like the idea of hard coding height here.
may be bottom: 0

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

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineFlameChart.js:120: * @param {number}
index
@override

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineFlameChart.js:130: * @param {number}
index
ditto

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

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:290:
WebInspector.TimelineUIUtilsImpl.markerEventColor = function(eventName)
Shouldn't it be TracingTimelineUIUtils?
Otherwise this function doesn't seem to belong to this file.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:332: if (name ===
recordTypes.TimeStamp)
looks like a place for switch. wdyt?

[yurys, 2014-07-21 14:30:00]

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/components/FlameChart.js (right):

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/components/FlameChart.js:661: if (markers[i] >
rightTime)
On 2014/07/21 14:21:07, alph wrote:
> nit: you can move the check into the for condition.

Done.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/components/FlameChart.js:840: var right =
markerTimestamps.upperBound(this._calculator.maximumBoundary(), compare);
On 2014/07/21 14:21:07, alph wrote:
> nit: you don't really need to calculate it. you can do it the same way as in
> _markerIndexAtPosition

Done.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/flameChart.css (right):

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/flameChart.css:51: top: 0px;
On 2014/07/21 14:21:07, alph wrote:
> no need for px suffix.

Done.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/flameChart.css:52: height: 20px;
On 2014/07/21 14:21:07, alph wrote:
> i don't like the idea of hard coding height here.
> may be bottom: 0

bottom: 0 wouldn't work as we draw it on top of a canvas.

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

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineFlameChart.js:120: * @param {number}
index
On 2014/07/21 14:21:07, alph wrote:
> @override

Done.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineFlameChart.js:130: * @param {number}
index
On 2014/07/21 14:21:07, alph wrote:
> ditto

Done.

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

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:290:
WebInspector.TimelineUIUtilsImpl.markerEventColor = function(eventName)
On 2014/07/21 14:21:07, alph wrote:
> Shouldn't it be TracingTimelineUIUtils?
> Otherwise this function doesn't seem to belong to this file.

Done.

https://codereview.chromium.org/402113002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:332: if (name ===
recordTypes.TimeStamp)
On 2014/07/21 14:21:07, alph wrote:
> looks like a place for switch. wdyt?

Done.

[yurys, 2014-07-21 15:06:52]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-07-21 15:08:04]

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

[commit-bot: I haz the power, 2014-07-21 16:09:31]

Change committed as 178568