DevTools: Support arbitrary bar heights on flamechart

DevTools: Support arbitrary bar heights on flamechart

BUG=705054
Review-Url: https://codereview.chromium.org/2850793004
Cr-Commit-Position: refs/heads/master@{#468543}
Committed: https://chromium.googlesource.com/chromium/src/+/22025d9c19bd5ec9a927760b49d46dbdab35a0df

[alph, 2017-04-28 23:45:05]

alph@chromium.org changed reviewers:
+ caseq@chromium.org

[caseq, 2017-04-29 00:26:09]

Can you also add some usage so it's easier to see whether the suggested
interface is convenient to use? I've got a feeling it may be more practical to
configure bar height per group, not level.

https://codereview.chromium.org/2850793004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2850793004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:213:
this.setScrollOffset(y, 20);
20?

https://codereview.chromium.org/2850793004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:674: var
barHeight = this._levelHeight(barLevel);
nit: var y = this._levelToOffset(barLevel) + this._levelHeight(barLevel) / 2;

[alph, 2017-04-29 01:23:01]

ptal

https://codereview.chromium.org/2850793004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2850793004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:213:
this.setScrollOffset(y, 20);
On 2017/04/29 00:26:09, caseq wrote:
> 20?

Done.

https://codereview.chromium.org/2850793004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:674: var
barHeight = this._levelHeight(barLevel);
On 2017/04/29 00:26:09, caseq wrote:
> nit: var y = this._levelToOffset(barLevel) + this._levelHeight(barLevel) / 2;

Done.

[caseq, 2017-05-01 22:19:30]

lgtm

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:1128: height
= isFirstOnLevel ? style.height : style.itemsHeight || this._barHeight;
nit: add parenthesis around last clause of ternary operator.

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:1134:
this._visibleLevelHeights[level] = height;
this looks more like just this._levelHeight

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:1234: return
this._visibleLevelHeights[level];
nit: I wonder if we should just inline it.

[alph, 2017-05-02 01:20:21]

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:1128: height
= isFirstOnLevel ? style.height : style.itemsHeight || this._barHeight;
On 2017/05/01 22:19:30, caseq wrote:
> nit: add parenthesis around last clause of ternary operator.

Done.

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:1134:
this._visibleLevelHeights[level] = height;
On 2017/05/01 22:19:30, caseq wrote:
> this looks more like just this._levelHeight

Acknowledged.

https://codereview.chromium.org/2850793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:1234: return
this._visibleLevelHeights[level];
On 2017/05/01 22:19:30, caseq wrote:
> nit: I wonder if we should just inline it.

I've done it the same way as _levelToOffset

[alph, 2017-05-02 01:22:30]

The CQ bit was checked by alph@chromium.org

[alph, 2017-05-02 01:22:30]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2850793004/#ps60001
(title: "4 landing")

[commit-bot: I haz the power, 2017-05-02 01:23:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 03:12:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493688150508060,
"parent_rev": "5d206a778cd0c9fbc5b14447e13bf825a20f9152", "commit_rev":
"22025d9c19bd5ec9a927760b49d46dbdab35a0df"}

[commit-bot: I haz the power, 2017-05-02 03:13:26]

Description was changed from

==========
DevTools: Support arbitrary bar heights on flamechart

BUG=705054
==========

to

==========
DevTools: Support arbitrary bar heights on flamechart

BUG=705054

Review-Url: https://codereview.chromium.org/2850793004
Cr-Commit-Position: refs/heads/master@{#468543}
Committed:
https://chromium.googlesource.com/chromium/src/+/22025d9c19bd5ec9a927760b49d4...
==========

[commit-bot: I haz the power, 2017-05-02 03:13:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/22025d9c19bd5ec9a927760b49d4...