DevTools: add per-frame grouping into aggregated timeline details

DevTools: add per-frame grouping into aggregated timeline details

- add frame list to TracingStartedInPage
- add frame details (URL, name) to CommitLoad
- refactor event grouping in the top-down tree to never prematurely
    merge events that would end up in different groups adter grouping.
- move GroupBy enum from WI.TimelineAggregator to AggregatedTimelineTreeView;

BUG=662990
Committed: https://crrev.com/79b4a4119de12f1f37748e7fedb3e2a87a534e3a
Cr-Commit-Position: refs/heads/master@{#430719}

[caseq, 2016-11-05 01:40:49]

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

[dgozman, 2016-11-07 17:28:39]

I believe this needs a bug.

[dgozman, 2016-11-07 17:28:56]

On 2016/11/07 17:28:39, dgozman wrote:
> I believe this needs a bug.

And a screenshot probably.

[dgozman, 2016-11-07 17:43:16]

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-aggregated-details.html
(right):

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-aggregated-details.html:36:
"page": "0x2f7b63884000",
Raw address in test?

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:171: static
String urlForFrame(LocalFrame* frame) {
nit: no need for static in anonymous namespace

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

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:621:
function groupByURL(event) {
This function does not take second string parameter, but is used as such. How
does this compile?

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js
(right):

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:65:
var id = eventGroupIdCallback ? WebInspector.TimelineProfileTree._eventId(e) :
Symbol('uniqueEventId');
Why do we need special ids in case of grouping?

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:259:
* @param {string} id
groupId

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:263:
nodeForId(id, event) {
groupNodeForId?

[caseq, 2016-11-07 17:45:14]

On 2016/11/07 17:28:56, dgozman wrote:
> On 2016/11/07 17:28:39, dgozman wrote:
> > I believe this needs a bug.
> 
> And a screenshot probably.

This does not introduce any visual changes aside from adding a new item to an
existing drop-down list -- there's no UI features to discuss.

[caseq, 2016-11-07 18:14:18]

Description was changed from

==========
DevTools: add per-frame grouping into aggregated timeline details

- add frame list to TracingStartedInPage
- add frame details (URL, name) to CommitLoad
- refactor event grouping in the top-down tree to never prematurely
    merge events that would end up in different groups adter grouping.
- move GroupBy enum from WI.TimelineAggregator to AggregatedTimelineTreeView;
==========

to

==========
DevTools: add per-frame grouping into aggregated timeline details

- add frame list to TracingStartedInPage
- add frame details (URL, name) to CommitLoad
- refactor event grouping in the top-down tree to never prematurely
    merge events that would end up in different groups adter grouping.
- move GroupBy enum from WI.TimelineAggregator to AggregatedTimelineTreeView;

BUG=662990
==========

[caseq, 2016-11-07 18:48:32]

On 2016/11/07 17:43:16, dgozman wrote:
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-aggregated-details.html
> (right):
> 
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-aggregated-details.html:36:
> "page": "0x2f7b63884000",
> Raw address in test?

Why raw, it's quoted! No worries, I swear this one won't change :-)

>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
(right):
> 
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:171: static
> String urlForFrame(LocalFrame* frame) {
> nit: no need for static in anonymous namespace

Fixed.

>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
> File third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js
> (right):
> 
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:621:
> function groupByURL(event) {
> This function does not take second string parameter, but is used as such. How
> does this compile?

Because function(type1) is a sub-type of function(type1, type2), so it makes it
safe to coerce the former into the latter.

>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
> File
>
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js
> (right):
> 
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:65:
> var id = eventGroupIdCallback ? WebInspector.TimelineProfileTree._eventId(e) :
> Symbol('uniqueEventId');
> Why do we need special ids in case of grouping?

This is the special case of "no grouping". The idea is to group nodes iff the
ids are equal, and the Symbol() is never equal to anything, which means every
node will remain standalone.

>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:259:
> * @param {string} id
> groupId

Done.

>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:263:
> nodeForId(id, event) {
> groupNodeForId?

Done.

[alph, 2016-11-07 19:10:03]

lgtm

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-aggregated-details.html
(right):

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/tracing/timeline-misc/timeline-aggregated-details.html:36:
"page": "0x2f7b63884000",
On 2016/11/07 17:43:16, dgozman wrote:
> Raw address in test?

This is a hardcoded const in the test.

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

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:621:
function groupByURL(event) {
On 2016/11/07 17:43:16, dgozman wrote:
> This function does not take second string parameter, but is used as such. How
> does this compile?

It is opposite. The function does not need the second parameter, so it the
caller provides one it's not used. This is ok.

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:661:
return (event, pageFrameId) => pageFrameId;
Adding an extra argument for a single grouping mode looks bad. Can we somehow
infer it from the event?

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js
(right):

https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:226:
return 'f:' + data['functionName'] + '@' + (data['scriptId'] || data['url'] ||
'');
nit: template string.

[caseq, 2016-11-08 19:45:30]

Patchset #3 (id:40001) has been deleted

[caseq, 2016-11-08 19:48:47]

On 2016/11/07 19:10:03, alph wrote:
> 
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:661:
> return (event, pageFrameId) => pageFrameId;
> Adding an extra argument for a single grouping mode looks bad. Can we somehow
> infer it from the event?

Agree. I'm going to address this a bit later, here's the patch that will prepare
us for this: https://codereview.chromium.org/2486853002/

>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
> File
>
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js
> (right):
> 
>
https://codereview.chromium.org/2473283003/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineProfileTree.js:226:
> return 'f:' + data['functionName'] + '@' + (data['scriptId'] || data['url'] ||
> '');
> nit: template string.

I've considered this, but decided against it based on too little string
constants per too much code.

[caseq, 2016-11-08 19:48:50]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2016-11-08 19:49:36]

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, 2016-11-08 21:34:15]

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

[commit-bot: I haz the power, 2016-11-08 21:34:16]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[caseq, 2016-11-08 21:42:40]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-11-08 21:42:41]

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

[commit-bot: I haz the power, 2016-11-08 21:43:32]

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, 2016-11-08 21:45:53]

Description was changed from

==========
DevTools: add per-frame grouping into aggregated timeline details

- add frame list to TracingStartedInPage
- add frame details (URL, name) to CommitLoad
- refactor event grouping in the top-down tree to never prematurely
    merge events that would end up in different groups adter grouping.
- move GroupBy enum from WI.TimelineAggregator to AggregatedTimelineTreeView;

BUG=662990
==========

to

==========
DevTools: add per-frame grouping into aggregated timeline details

- add frame list to TracingStartedInPage
- add frame details (URL, name) to CommitLoad
- refactor event grouping in the top-down tree to never prematurely
    merge events that would end up in different groups adter grouping.
- move GroupBy enum from WI.TimelineAggregator to AggregatedTimelineTreeView;

BUG=662990

Committed: https://crrev.com/79b4a4119de12f1f37748e7fedb3e2a87a534e3a
Cr-Commit-Position: refs/heads/master@{#430719}
==========

[commit-bot: I haz the power, 2016-11-08 21:45:54]

Patchset 3 (id:??) landed as
https://crrev.com/79b4a4119de12f1f37748e7fedb3e2a87a534e3a
Cr-Commit-Position: refs/heads/master@{#430719}