FlameChart: convert colors magic into dataProvider.entryColor getter

FlameChart: convert colors magic into dataProvider.entryColor getter

BUG=
R=pfeldman@chromium.org

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

[alph, 2014-03-03 10:06:04]

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

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:870: if (entryOffsetRight <
timeWindowLeft)
Is it possible to find the bound using binary search rather than iterating over
all the entries?

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:875: barWidth = (barRight - barX) ||
minWidth;
nit: Should be Math.max

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:901: barWidth = (barRight - barX) ||
minWidth;
ditto.

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:927: barWidth = (barRight - barX) ||
minWidth;
ditto

[loislo, 2014-03-03 11:53:30]

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

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:870: if (entryOffsetRight <
timeWindowLeft)
On 2014/03/03 10:06:04, alph wrote:
> Is it possible to find the bound using binary search rather than iterating
over
> all the entries?

Nope. The array is sorted by the bar offset, so the the right borders of the
bars are unsorted and we can't use binary search for that.

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:875: barWidth = (barRight - barX) ||
minWidth;
On 2014/03/03 10:06:04, alph wrote:
> nit: Should be Math.max

Done.

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:901: barWidth = (barRight - barX) ||
minWidth;
On 2014/03/03 10:06:04, alph wrote:
> ditto.

Done.

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:927: barWidth = (barRight - barX) ||
minWidth;
On 2014/03/03 10:06:04, alph wrote:
> ditto

Done.

[pfeldman, 2014-03-03 12:20:54]

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
File Source/devtools/front_end/CPUProfileView.js (right):

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/CPUProfileView.js:1196: entries.push(new
ChartEntry(color, depth, samplingInterval, sampleIndex * samplingInterval,
node));
You only want to do this on demand.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
File Source/devtools/front_end/FlameChart.js (right):

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:829: var marksRow;
Should be annotated.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:843: var colorBackets = {};
Buckets

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:845: var backet = [];
bucket.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:847: var textBaseHeight =
this._baseHeight + this._barHeight - 4;
You only use these variables below the first iteration, declare them later.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:852: var widthText = 0;
textWidth ?

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:865: if (barLevel > maxBarLevel)
>= ?

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:883: color =
this._dataProvider.entryColor(entryIndex);
This needs to be a second path - otherwise you query too many colors.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:886: backet = colorBackets[color] = [];
One statement on one line.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:891: for (var c = 0; c < colors.length;
++c) {
Comment why you don't use for in.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
File Source/devtools/front_end/TimelineFlameChart.js (right):

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/TimelineFlameChart.js:167: entryLevels: /** @type
{!Array.<number>} */ ([]),
You should annotate _timelineData instead.

[alph, 2014-03-03 12:31:03]

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

https://codereview.chromium.org/185533004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:870: if (entryOffsetRight <
timeWindowLeft)
On 2014/03/03 11:53:31, loislo wrote:
> On 2014/03/03 10:06:04, alph wrote:
> > Is it possible to find the bound using binary search rather than iterating
> over
> > all the entries?
> 
> Nope. The array is sorted by the bar offset, so the the right borders of the
> bars are unsorted and we can't use binary search for that.

So, you can find the right window bound then, which is startTime. Right?

[loislo, 2014-03-03 14:29:26]

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
File Source/devtools/front_end/FlameChart.js (right):

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:829: var marksRow;
On 2014/03/03 12:20:54, pfeldman wrote:
> Should be annotated.

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:843: var colorBackets = {};
On 2014/03/03 12:20:54, pfeldman wrote:
> Buckets

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:845: var backet = [];
On 2014/03/03 12:20:54, pfeldman wrote:
> bucket.

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:847: var textBaseHeight =
this._baseHeight + this._barHeight - 4;
On 2014/03/03 12:20:54, pfeldman wrote:
> You only use these variables below the first iteration, declare them later.

V8 may deoptimize the whole function due to the fact of declaring new variable.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:852: var widthText = 0;
On 2014/03/03 12:20:54, pfeldman wrote:
> textWidth ?

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:865: if (barLevel > maxBarLevel)
On 2014/03/03 12:20:54, pfeldman wrote:
> >= ?

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:883: color =
this._dataProvider.entryColor(entryIndex);
On 2014/03/03 12:20:54, pfeldman wrote:
> This needs to be a second path - otherwise you query too many colors.

The previous row was the last check of the algorithm.
So at this point we know that we will draw the bar.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:886: backet = colorBackets[color] = [];
On 2014/03/03 12:20:54, pfeldman wrote:
> One statement on one line.

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:891: for (var c = 0; c < colors.length;
++c) {
On 2014/03/03 12:20:54, pfeldman wrote:
> Comment why you don't use for in.

Done.

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
File Source/devtools/front_end/TimelineFlameChart.js (right):

https://codereview.chromium.org/185533004/diff/10004/Source/devtools/front_en...
Source/devtools/front_end/TimelineFlameChart.js:167: entryLevels: /** @type
{!Array.<number>} */ ([]),
On 2014/03/03 12:20:54, pfeldman wrote:
> You should annotate _timelineData instead.

Done.

[pfeldman, 2014-03-03 14:42:46]

lgtm

https://codereview.chromium.org/185533004/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/FlameChart.js (right):

https://codereview.chromium.org/185533004/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/FlameChart.js:848: var font;
Extract method then.

[loislo, 2014-03-03 15:02:50]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2014-03-03 15:02:57]

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

[commit-bot: I haz the power, 2014-03-03 20:01:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-03 20:01:59]

Retried try job too often on win_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[loislo, 2014-03-04 05:10:17]

Committed patchset #9 manually as r168361 (presubmit successful).