DevTools: draw load/first paint marks as tall vertical delimiters

DevTools: draw load/first paint marks as tall vertical delimiters

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

[yurys, 2014-11-07 17:29:17]

yurys@chromium.org changed reviewers:
+ caseq@chromium.org, loislo@chromium.org, pfeldman@chromium.org

[yurys, 2014-11-07 17:29:18]

See screenshot in the bug.

[caseq, 2014-11-07 17:39:40]

lgtm

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

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:162: isTallMarker:
function(index) { },
nit: I wonder if we should generalize this to markerType?

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:1049: context.lineWidth = 1;
Is this supposed to have effect?

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

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:349: switch
(event.name) {
return event.name !== recordTypes.TimeStamp for clarity?

[yurys, 2014-11-07 19:55:40]

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

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:162: isTallMarker:
function(index) { },
On 2014/11/07 17:39:40, caseq wrote:
> nit: I wonder if we should generalize this to markerType?

I don't see other types at the moment so there is no rush and current method
looks more clear. We can always refactor this.

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:1049: context.lineWidth = 1;
On 2014/11/07 17:39:40, caseq wrote:
> Is this supposed to have effect?

Yes, because outer context scope sets line width to 2.

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

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:349: switch
(event.name) {
On 2014/11/07 17:39:40, caseq wrote:
> return event.name !== recordTypes.TimeStamp for clarity?

Done.

[yurys, 2014-11-07 19:55:45]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-07 19:56:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/706313002/20001

[caseq, 2014-11-07 20:00:37]

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

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:1049: context.lineWidth = 1;
On 2014/11/07 19:55:39, yurys wrote:
> On 2014/11/07 17:39:40, caseq wrote:
> > Is this supposed to have effect?
> 
> Yes, because outer context scope sets line width to 2.

You have "context.lineWidth = 0.5;" below.

[caseq, 2014-11-07 20:00:42]

The CQ bit was unchecked by caseq@chromium.org

[yurys, 2014-11-07 20:02:56]

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

https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/components/FlameChart.js:1049: context.lineWidth = 1;
On 2014/11/07 20:00:37, caseq wrote:
> On 2014/11/07 19:55:39, yurys wrote:
> > On 2014/11/07 17:39:40, caseq wrote:
> > > Is this supposed to have effect?
> > 
> > Yes, because outer context scope sets line width to 2.
> 
> You have "context.lineWidth = 0.5;" below.

Oh, I see, it should be just lineWidth = 1 and translate(0.5, 0.5). Thanks!

[yurys, 2014-11-07 20:10:02]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-07 20:10:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/706313002/40001

[yurys, 2014-11-07 20:15:42]

On 2014/11/07 20:02:56, yurys wrote:
>
https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
> File Source/devtools/front_end/components/FlameChart.js (right):
> 
>
https://codereview.chromium.org/706313002/diff/1/Source/devtools/front_end/co...
> Source/devtools/front_end/components/FlameChart.js:1049: context.lineWidth =
1;
> On 2014/11/07 20:00:37, caseq wrote:
> > On 2014/11/07 19:55:39, yurys wrote:
> > > On 2014/11/07 17:39:40, caseq wrote:
> > > > Is this supposed to have effect?
> > > 
> > > Yes, because outer context scope sets line width to 2.
> > 
> > You have "context.lineWidth = 0.5;" below.
> 
> Oh, I see, it should be just lineWidth = 1 and translate(0.5, 0.5). Thanks!

Actually, 0.5 looks better, so changed the code to set it to 0.5 only.

[yurys, 2014-11-07 20:16:06]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-11-07 20:16:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/706313002/60001

[commit-bot: I haz the power, 2014-11-07 21:27:36]

Committed patchset #4 (id:60001) as 184990