Create TaskAttributionTiming PerformanceEntry for attribution in PerformanceLongTaskTiming. Move cu…

Create TaskAttributionTiming PerformanceEntry for attribution in PerformanceLongTaskTiming. Move culprit-frame-related fields in there

BUG=676484
Review-Url: https://codereview.chromium.org/2610063006
Cr-Commit-Position: refs/heads/master@{#441814}
Committed: https://chromium.googlesource.com/chromium/src/+/262009cc0afed3a6a9c152c122c28599f2fe67f6

[panicker, 2017-01-04 23:55:42]

panicker@chromium.org changed reviewers:
+ haraken@chromium.org, tdresser@chromium.org

[panicker, 2017-01-04 23:55:42]

PTAL.

[haraken, 2017-01-05 01:13:27]

Implementation-wise looks good.

https://codereview.chromium.org/2610063006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/TaskAttributionTiming.h (right):

https://codereview.chromium.org/2610063006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/TaskAttributionTiming.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.

2017

[panicker, 2017-01-05 01:25:12]

panicker@chromium.org changed reviewers:
+ igrigorik@chromium.org

[panicker, 2017-01-05 01:25:13]

Thanks for the review.
Ilya and Tim (to some extent) are familiar with the API.

https://codereview.chromium.org/2610063006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/TaskAttributionTiming.h (right):

https://codereview.chromium.org/2610063006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/TaskAttributionTiming.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/01/05 01:13:27, haraken wrote:
> 
> 2017

Done.

[tdresser, 2017-01-05 13:34:14]

LGTM with nits.

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/performance/longtasktiming.html
(right):

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/performance/longtasktiming.html:20:
"startTime expected to have 1 miilisecond granularity");
millisecond

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/performance/longtasktiming.html:22: //
Assert the TaskAttributionTiming entry in attribution
Nit: I think comments are supposed to always end in . in Layout tests, aren't
they?

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:141: case
PerformanceEntry::LongTask:
I'd be tempted to stick these cases at the bottom of the switch, and immediately
break, instead of having separate returns, as it's kind of nice to have a single
return statement.

This way is fine too though.

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.cpp
(right):

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.cpp:28: String
culpritFrameName) {
I'd strip the culprit prefix throughout - I don't think it adds anything, and
currently the naming is inconsistent.

[haraken, 2017-01-05 15:15:05]

LGTM

[panicker, 2017-01-05 20:56:49]

Thanks for the review.

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/performance/longtasktiming.html
(right):

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/performance/longtasktiming.html:20:
"startTime expected to have 1 miilisecond granularity");
On 2017/01/05 13:34:14, tdresser wrote:
> millisecond

Done.

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/performance/longtasktiming.html:22: //
Assert the TaskAttributionTiming entry in attribution
On 2017/01/05 13:34:14, tdresser wrote:
> Nit: I think comments are supposed to always end in . in Layout tests, aren't
> they?

Done.

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:141: case
PerformanceEntry::LongTask:
On 2017/01/05 13:34:14, tdresser wrote:
> I'd be tempted to stick these cases at the bottom of the switch, and
immediately
> break, instead of having separate returns, as it's kind of nice to have a
single
> return statement.
> 
> This way is fine too though.
> 

Done.

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.cpp
(right):

https://codereview.chromium.org/2610063006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.cpp:28: String
culpritFrameName) {
On 2017/01/05 13:34:14, tdresser wrote:
> I'd strip the culprit prefix throughout - I don't think it adds anything, and
> currently the naming is inconsistent.

Done.

[panicker, 2017-01-05 20:57:16]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-01-05 20:57:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2610063006/#ps60001
(title: "address review nits")

[commit-bot: I haz the power, 2017-01-05 20:57:56]

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-01-05 22:55:15]

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

[commit-bot: I haz the power, 2017-01-05 22:55:16]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[panicker, 2017-01-05 23:59:28]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-01-05 23:59:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2610063006/#ps80001
(title: "update layout test expectation")

[commit-bot: I haz the power, 2017-01-06 00:00: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, 2017-01-06 01:59:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483660768293610,
"parent_rev": "95355aa2e11a009b2fc2b747d4127e70dfd16249", "commit_rev":
"262009cc0afed3a6a9c152c122c28599f2fe67f6"}

[commit-bot: I haz the power, 2017-01-06 02:00:01]

Description was changed from

==========
Create TaskAttributionTiming PerformanceEntry for attribution in
PerformanceLongTaskTiming. Move culprit-frame-related fields in there

BUG=676484
==========

to

==========
Create TaskAttributionTiming PerformanceEntry for attribution in
PerformanceLongTaskTiming. Move culprit-frame-related fields in there

BUG=676484

Review-Url: https://codereview.chromium.org/2610063006
Cr-Commit-Position: refs/heads/master@{#441814}
Committed:
https://chromium.googlesource.com/chromium/src/+/262009cc0afed3a6a9c152c122c2...
==========

[commit-bot: I haz the power, 2017-01-06 02:00:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/262009cc0afed3a6a9c152c122c2...

[Yoav Weiss, 2017-01-06 10:13:04]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2017-01-06 10:13:05]

https://codereview.chromium.org/2610063006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2610063006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:168: break;
Hmm, this is according to spec:
https://w3c.github.io/performance-timeline/#filter-performance-entry-buffer-b...

On top of that "PerformanceObserverInit.entryTypes is a list of valid entryType
names to be observed. The list must not be empty and types not recognized by the
user agent must be ignored."

Both mean that there's no way for developers to detect lack of support for new
entry types and create their own fallbacks for cases it is needed.

Again, not an implementation issue, but IMO a significant spec one. I'll file a
spec bug.

[panicker, 2017-01-06 20:02:17]

On 2017/01/06 10:13:05, Yoav Weiss wrote:
>
https://codereview.chromium.org/2610063006/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> 
>
https://codereview.chromium.org/2610063006/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/timing/PerformanceBase.cpp:168: break;
> Hmm, this is according to spec:
>
https://w3c.github.io/performance-timeline/#filter-performance-entry-buffer-b...
> 
> On top of that "PerformanceObserverInit.entryTypes is a list of valid
entryType
> names to be observed. The list must not be empty and types not recognized by
the
> user agent must be ignored."
> 
> Both mean that there's no way for developers to detect lack of support for new
> entry types and create their own fallbacks for cases it is needed.
> 
> Again, not an implementation issue, but IMO a significant spec one. I'll file
a
> spec bug.

It is possible for developers to detect lack of support: 
observer.observe({entryTypes: ["longtask"]}); will throw an error if
unsupported.
We recently updated the error message to be less confusing.

[Yoav Weiss, 2017-01-06 20:11:23]

On 2017/01/06 20:02:17, panicker wrote:
> On 2017/01/06 10:13:05, Yoav Weiss wrote:
> >
>
https://codereview.chromium.org/2610063006/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2610063006/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/timing/PerformanceBase.cpp:168: break;
> > Hmm, this is according to spec:
> >
>
https://w3c.github.io/performance-timeline/#filter-performance-entry-buffer-b...
> > 
> > On top of that "PerformanceObserverInit.entryTypes is a list of valid
> entryType
> > names to be observed. The list must not be empty and types not recognized by
> the
> > user agent must be ignored."
> > 
> > Both mean that there's no way for developers to detect lack of support for
new
> > entry types and create their own fallbacks for cases it is needed.
> > 
> > Again, not an implementation issue, but IMO a significant spec one. I'll
file
> a
> > spec bug.
> 
> It is possible for developers to detect lack of support: 
> observer.observe({entryTypes: ["longtask"]}); will throw an error if
> unsupported.
> We recently updated the error message to be less confusing.

That's great! All that's left is to fix the spec to match.

We should probably do the same for getEntriesByType, but let's discuss that on
https://github.com/w3c/performance-timeline/issues/64