DevTools: Show screenshots on the main flamechart

DevTools: Show screenshots on the main flamechart

The patch puts the screenshots on the frame bar and makes it expandable.

BUG=705054
Review-Url: https://codereview.chromium.org/2852173002
Cr-Commit-Position: refs/heads/master@{#468819}
Committed: https://chromium.googlesource.com/chromium/src/+/35061bcb47bf268077a37ae7a44f334d585745a6

[alph, 2017-05-01 21:18:38]

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

[caseq, 2017-05-01 22:13:35]

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineEventOverview.js
(right):

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineEventOverview.js:384:
imagePromise = frame.imageDataPromise().then(data => UI.loadImage(data ?
'data:image/jpg;base64,' + data : ''));
Are we sure UI.loadImage() will handle '' well?

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js
(right):

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:454:
this._entryTypeByLevel[this._currentLevel] =
Timeline.TimelineFlameChartEntryType.Frame;
Let's add another type for FrameThumbnail.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:456:
this._markers.push(new Timeline.TimelineFlameChartMarker(
Why do we do it twice?

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:573:
_drawFrame(entryIndex, context, text, barX, barY, barWidth, barHeight) {
Let's split it into two, one for drawing thumbnail and one for drawing label.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:591:
.then(() =>
this.dispatchEventToListeners(Timeline.TimelineFlameChartDataProvider.Events.DataChanged));
no need for another then, let's do this synchronously with the one above.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1829:
static filmStripModelFrame(filmStripModel, frame) {
Let's rather move it to PerformanceModel, it does not look particularly specific
to UI.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1835:
filmStripFrame = null;
nit: return null instead.

[alph, 2017-05-02 18:31:14]

ptal

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineEventOverview.js
(right):

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineEventOverview.js:384:
imagePromise = frame.imageDataPromise().then(data => UI.loadImage(data ?
'data:image/jpg;base64,' + data : ''));
On 2017/05/01 22:13:35, caseq wrote:
> Are we sure UI.loadImage() will handle '' well?

Done.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js
(right):

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:454:
this._entryTypeByLevel[this._currentLevel] =
Timeline.TimelineFlameChartEntryType.Frame;
On 2017/05/01 22:13:35, caseq wrote:
> Let's add another type for FrameThumbnail.

Acknowledged.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:456:
this._markers.push(new Timeline.TimelineFlameChartMarker(
On 2017/05/01 22:13:35, caseq wrote:
> Why do we do it twice?

Done.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:573:
_drawFrame(entryIndex, context, text, barX, barY, barWidth, barHeight) {
On 2017/05/01 22:13:35, caseq wrote:
> Let's split it into two, one for drawing thumbnail and one for drawing label.

Done it differently

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChartDataProvider.js:591:
.then(() =>
this.dispatchEventToListeners(Timeline.TimelineFlameChartDataProvider.Events.DataChanged));
On 2017/05/01 22:13:35, caseq wrote:
> no need for another then, let's do this synchronously with the one above.

Done.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1829:
static filmStripModelFrame(filmStripModel, frame) {
On 2017/05/01 22:13:35, caseq wrote:
> Let's rather move it to PerformanceModel, it does not look particularly
specific
> to UI.

Done.

https://codereview.chromium.org/2852173002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1835:
filmStripFrame = null;
On 2017/05/01 22:13:35, caseq wrote:
> nit: return null instead.

Done.

[caseq, 2017-05-02 18:37:56]

lgtm

[alph, 2017-05-02 21:41:51]

The CQ bit was checked by alph@chromium.org

[commit-bot: I haz the power, 2017-05-02 21:42:17]

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 23:37:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493761311539990,
"parent_rev": "c147785f62af1a1a8fbea1fb56fa093d3b3d84e8", "commit_rev":
"35061bcb47bf268077a37ae7a44f334d585745a6"}

[commit-bot: I haz the power, 2017-05-02 23:37:22]

Description was changed from

==========
DevTools: Show screenshots on the main flamechart

The patch puts the screenshots on the frame bar and makes it expandable.

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

to

==========
DevTools: Show screenshots on the main flamechart

The patch puts the screenshots on the frame bar and makes it expandable.

BUG=705054

Review-Url: https://codereview.chromium.org/2852173002
Cr-Commit-Position: refs/heads/master@{#468819}
Committed:
https://chromium.googlesource.com/chromium/src/+/35061bcb47bf268077a37ae7a44f...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/35061bcb47bf268077a37ae7a44f...