Enable ServerTiming support

Server-Timing

Parse Server-Timing response headers and make the metric name(s), value(s), and 
description(s) available as PerformanceServerTiming entries on the global 
performance timeline and to PerformanceObservers. This feature is behind the 
"ServerTiming" experimental-features flag.

To test, turn the flag on 
(chrome://flags/#enable-experimental-web-platform-features) and explore to 
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')` 
in DevTools to inspect the 4 entries for the base page, as per the response 
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300; "cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a 
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})

Spec: https://w3c.github.io/server-timing/
Intent to implement: 
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/y3qwr490Fc4/boGoo41-BwAJ

BUG=702760

[cvazac, 2017-04-06 18:03:40]

Description was changed from

==========
Server-Timing

BUG=702760

Parse Server-Timing response headers and make the metric name(s), value(s), and
description(s) available as PerformanceServerTiming entries on the global
performance timeline and to PerformanceObservers. This feature is behind the
"ServerTiming" experimental-features flag.

To test, turn the flag on
(chrome://flags/#enable-experimental-web-platform-features) and explore to
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')`
in DevTools to inspect the 4 entries for the base page, as per the response
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300;
"cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})
==========

to

==========
Server-Timing

Parse Server-Timing response headers and make the metric name(s), value(s), and
description(s) available as PerformanceServerTiming entries on the global
performance timeline and to PerformanceObservers. This feature is behind the
"ServerTiming" experimental-features flag.

To test, turn the flag on
(chrome://flags/#enable-experimental-web-platform-features) and explore to
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')`
in DevTools to inspect the 4 entries for the base page, as per the response
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300;
"cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})

Spec: https://w3c.github.io/server-timing/
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/y3qwr490Fc4/bo...

BUG=702760
==========

[cvazac, 2017-04-06 18:27:20]

The CQ bit was checked by cvazac@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 18:28:32]

Dry run: 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-04-06 18:28:33]

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

[commit-bot: I haz the power, 2017-04-06 18:28:34]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Yoav Weiss, 2017-04-06 20:19:54]

yoav@yoav.ws changed reviewers:
+ csharrison@chromium.org, panicker@chromium.org, yoav@yoav.ws

[Yoav Weiss, 2017-04-06 20:19:55]

LGTM!

Thanks for working on this :) A few nits related to comment formatting

panicker@ and csharrison@ - can you also take a look?

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:545: // them here
Nit: Maybe move the comment above the condition? Formatting here is weird. Also,
add a period at the end.

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:212: // expect this
buffer to be too big
nit: Capitalize first letter. Period at the end of the comment.

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/HTTPParsers.cpp:898: 1;  // begin
after the opening DQUOTE, end before the closing DQUOTE
Nit: formatting is weird. Move the comment to the line above. Also capitalize
and add a period.

https://codereview.chromium.org/2799093004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/HTTPParsers.cpp:901: pos++;  // skip
right past the backslash
Can you move the comments to above the `while` so that they describe what this
whole section is doing, rather than line by line?

[panicker, 2017-04-06 22:42:22]

Great work. Couple comments.

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

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:325:
notifyObserversOfEntry(*entry);
would be good to make a notifyObserversOfEntries() and queue them all together.
Otherwise if there's many of them we may add unnecessary overhead by delivering
piecemeal (if delivery timer fires)

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327:
m_serverTimingBuffer.push_back(*entry);
Could we have a max size for this buffer?
On some sites onload fires very late so this could be arbitrarily large.

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceServerTiming.idl (right):

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceServerTiming.idl:6:
RuntimeEnabled=ServerTiming
Add link to spec / explainer

[panicker, 2017-04-06 22:48:19]

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js
(right):

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js:9:
*/
Could you provide some context on this test helper?
In general server-timing (and new performance entry types) should not be adding
their own helpers and instead should enhance / use existing perf-entry helpers
such as:
https://github.com/w3c/web-platform-tests/blob/master/performance-timeline/pe...

[cvazac, 2017-04-06 23:50:00]

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js
(right):

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js:9:
*/
Yes, I added the 4th copy of `wp_test()` :), here are the other 3:
https://cs.chromium.org/search/?q=%22function+wp_test%22+package:%5Echromium$...

Should I refactor it up to somewhere in
`src/third_party/WebKit/LayoutTests/external/wpt` (maybe `common/utils.js`)?
That way at least NT, UT, and ST could share. 


On 2017/04/06 22:48:19, panicker wrote:
> Could you provide some context on this test helper?
> In general server-timing (and new performance entry types) should not be
adding
> their own helpers and instead should enhance / use existing perf-entry helpers
> such as:
>
https://github.com/w3c/web-platform-tests/blob/master/performance-timeline/pe...

[cvazac, 2017-04-06 23:58:02]

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

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327:
m_serverTimingBuffer.push_back(*entry);
Sure! Like resource timing
(https://www.w3.org/TR/resource-timing/#extensions-performance-interface),
should I add the analogous `clearServerTimings()`,
`setServerTimingBufferSize()`, and `onservertimingbufferfull` handler? Or were
you thinking something un-configurable?

On 2017/04/06 22:42:22, panicker wrote:
> Could we have a max size for this buffer?
> On some sites onload fires very late so this could be arbitrarily large.

[Charlie Harrison, 2017-04-07 12:43:02]

LGTM when panicker is happy, I just have a few nits.

Do you mind wrapping your CL description to 80 cols?

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

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.h:118: void
addServerTiming(const ResourceResponse&, const ShouldAddToBuffer&);
ShouldAddtoBuffer does not need to be const.

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceServerTiming.cpp (right):

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceServerTiming.cpp:8: #include
"core/timing/PerformanceBase.h"
Can you #include WTFString.h?

[cvazac, 2017-04-07 15:22:55]

Description was changed from

==========
Server-Timing

Parse Server-Timing response headers and make the metric name(s), value(s), and
description(s) available as PerformanceServerTiming entries on the global
performance timeline and to PerformanceObservers. This feature is behind the
"ServerTiming" experimental-features flag.

To test, turn the flag on
(chrome://flags/#enable-experimental-web-platform-features) and explore to
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')`
in DevTools to inspect the 4 entries for the base page, as per the response
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300;
"cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})

Spec: https://w3c.github.io/server-timing/
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/y3qwr490Fc4/bo...

BUG=702760
==========

to

==========
Server-Timing

Parse Server-Timing response headers and make the metric name(s), value(s), and 
description(s) available as PerformanceServerTiming entries on the global 
performance timeline and to PerformanceObservers. This feature is behind the 
"ServerTiming" experimental-features flag.

To test, turn the flag on 
(chrome://flags/#enable-experimental-web-platform-features) and explore to 
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')` 
in DevTools to inspect the 4 entries for the base page, as per the response 
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300;
"cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a 
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})

Spec: https://w3c.github.io/server-timing/
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/y3qwr490Fc4/bo...

BUG=702760
==========

[cvazac, 2017-04-07 15:23:14]

Description was changed from

==========
Server-Timing

Parse Server-Timing response headers and make the metric name(s), value(s), and 
description(s) available as PerformanceServerTiming entries on the global 
performance timeline and to PerformanceObservers. This feature is behind the 
"ServerTiming" experimental-features flag.

To test, turn the flag on 
(chrome://flags/#enable-experimental-web-platform-features) and explore to 
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')` 
in DevTools to inspect the 4 entries for the base page, as per the response 
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300;
"cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a 
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})

Spec: https://w3c.github.io/server-timing/
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/y3qwr490Fc4/bo...

BUG=702760
==========

to

==========
Server-Timing

Parse Server-Timing response headers and make the metric name(s), value(s), and 
description(s) available as PerformanceServerTiming entries on the global 
performance timeline and to PerformanceObservers. This feature is behind the 
"ServerTiming" experimental-features flag.

To test, turn the flag on 
(chrome://flags/#enable-experimental-web-platform-features) and explore to 
https://server-timing.now.sh/. Execute `performance.getEntriesByType('server')` 
in DevTools to inspect the 4 entries for the base page, as per the response 
header:
server-timing:db=100; "Database metric", api=200; "HTTP/API metric", cache=300;
"cache metric", total=0.000297322; "Total Response Time"

To inspect the 4 entries for the favicon.ico, you will need to set up a 
PerformanceObserver (I use TamperMonkey to inject a content script) like this:
new PerformanceObserver(function(list, observer) {
  console.info(list.getEntries())
  observer.disconnect()
}).observe({entryTypes: ['server']})

Spec: https://w3c.github.io/server-timing/
Intent to implement: 
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/y3qwr490Fc4/bo...

BUG=702760
==========

[panicker, 2017-04-07 23:01:32]

I was looking at your spec CL
https://github.com/w3c/performance-timeline/pull/76/files

Looks like there's a new bool PerformanceObserverInit.buffered to indicate
whether buffered entries should be emitted.
Are you planning to implement this?

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js
(right):

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js:9:
*/
On 2017/04/06 23:50:00, cvazac wrote:
> Yes, I added the 4th copy of `wp_test()` :), here are the other 3:
>
https://cs.chromium.org/search/?q=%22function+wp_test%22+package:%5Echromium$...
> 
> Should I refactor it up to somewhere in
> `src/third_party/WebKit/LayoutTests/external/wpt` (maybe `common/utils.js`)?
> That way at least NT, UT, and ST could share. 
> 
> 
> On 2017/04/06 22:48:19, panicker wrote:
> > Could you provide some context on this test helper?
> > In general server-timing (and new performance entry types) should not be
> adding
> > their own helpers and instead should enhance / use existing perf-entry
helpers
> > such as:
> >
>
https://github.com/w3c/web-platform-tests/blob/master/performance-timeline/pe...

Please let's not add the 4th copy :)
You could either update and use:
https://github.com/w3c/web-platform-tests/blob/master/performance-timeline/pe...
or create common/performance-timeline-utils. 
Refactoring the other 3 files things there (as a followup) is appreciated!

You are probably already aware - external/wpt auto-syncs with the github WPT
repo and there's random stuff in common/utils.js which we probably don't want to
add arbitrary stuff to:
https://github.com/w3c/web-platform-tests/blob/master/common/utils.js

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

https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327:
m_serverTimingBuffer.push_back(*entry);
On 2017/04/06 23:58:02, cvazac wrote:
> Sure! Like resource timing
> (https://www.w3.org/TR/resource-timing/#extensions-performance-interface),
> should I add the analogous `clearServerTimings()`,
> `setServerTimingBufferSize()`, and `onservertimingbufferfull` handler? Or were
> you thinking something un-configurable?
> 
> On 2017/04/06 22:42:22, panicker wrote:
> > Could we have a max size for this buffer?
> > On some sites onload fires very late so this could be arbitrarily large.
> 

Since this API is only exposed on Performance Observer (and not the older
performance timeline) no need to web-expose this.
Just an internal max size would be fine.

[panicker, 2017-04-07 23:15:10]

On 2017/04/07 23:01:32, panicker wrote:
> I was looking at your spec CL
> https://github.com/w3c/performance-timeline/pull/76/files
> 
> Looks like there's a new bool PerformanceObserverInit.buffered to indicate
> whether buffered entries should be emitted.
> Are you planning to implement this?
> 
>
https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js
> (right):
> 
>
https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js:9:
> */
> On 2017/04/06 23:50:00, cvazac wrote:
> > Yes, I added the 4th copy of `wp_test()` :), here are the other 3:
> >
>
https://cs.chromium.org/search/?q=%22function+wp_test%22+package:%5Echromium$...
> > 
> > Should I refactor it up to somewhere in
> > `src/third_party/WebKit/LayoutTests/external/wpt` (maybe `common/utils.js`)?
> > That way at least NT, UT, and ST could share. 
> > 
> > 
> > On 2017/04/06 22:48:19, panicker wrote:
> > > Could you provide some context on this test helper?
> > > In general server-timing (and new performance entry types) should not be
> > adding
> > > their own helpers and instead should enhance / use existing perf-entry
> helpers
> > > such as:
> > >
> >
>
https://github.com/w3c/web-platform-tests/blob/master/performance-timeline/pe...
> 
> Please let's not add the 4th copy :)
> You could either update and use:
>
https://github.com/w3c/web-platform-tests/blob/master/performance-timeline/pe...
> or create common/performance-timeline-utils. 
> Refactoring the other 3 files things there (as a followup) is appreciated!
> 
> You are probably already aware - external/wpt auto-syncs with the github WPT
> repo and there's random stuff in common/utils.js which we probably don't want
to
> add arbitrary stuff to:
> https://github.com/w3c/web-platform-tests/blob/master/common/utils.js
> 
>
https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> 
>
https://codereview.chromium.org/2799093004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327:
> m_serverTimingBuffer.push_back(*entry);
> On 2017/04/06 23:58:02, cvazac wrote:
> > Sure! Like resource timing
> > (https://www.w3.org/TR/resource-timing/#extensions-performance-interface),
> > should I add the analogous `clearServerTimings()`,
> > `setServerTimingBufferSize()`, and `onservertimingbufferfull` handler? Or
were
> > you thinking something un-configurable?
> > 
> > On 2017/04/06 22:42:22, panicker wrote:
> > > Could we have a max size for this buffer?
> > > On some sites onload fires very late so this could be arbitrarily large.
> > 
> 
> Since this API is only exposed on Performance Observer (and not the older
> performance timeline) no need to web-expose this.
> Just an internal max size would be fine.

Also what's the spec recommendation for clearing this buffer?

[cvazac, 2017-04-13 20:32:01]

Yes! I plan on implementing `buffered` for all `entryTypes`. But, because that
part of the performance timeline level 2 spec is still in flux, I was hoping to
land this first. If you prefer that we don’t land this until the `buffered`
part, let me know. 

Once finalized, I’ll rebase server-timing. So far `clearServerTimings()`,
`setServerTimingBufferSize()`, and `onservertimingbufferfull` are not a part of
that.

I moved `wp_test()`, `test_true()`, and `test_equals()` to
LayoutTests/external/wpt/common/performance-timing-utils.js. That caused me to
have to add a script reference to ~25 html test files, so that explains the jump
in modified file count.

I set a buffer limit of 1000. The math isn’t perfect on it - the code will
prevent you from adding 4 if you already have 998, for example. My thinking was
this would be better than switching `appendVector()` to a bunch of `push_back()`
calls, or having a second data structure.

[panicker, 2017-04-14 19:31:04]

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js
(right):

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/webperftestharness.js:12:
function test_entries(actualEntries, expectedEntries) {
why not move this to the common util?
(There is nothing specific to server-timing here)

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

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:69: static const
size_t kServerTimingBufferSize = 1000;
Curious - How did you come up with 1000?
This seems quite high, while this is probably okay for kServerTimingBufferSize
is isolation, remember that every PerformanceEntry type now needs this buffer.
Many thousands of entries is not okay as there is no plan to free this up.
Could we go with the 150 limit?

BTW At the least the spec should indicate that under memory pressure, the system
is free to delete these entries (and advise developers to collect these entries
early).

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:213: // expect this
buffer to be too big.
can you update this comment to reflect that the buffer size is per-determined?

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:521: for (auto&
observer : observers_) {
call NotifyObserversOfEntry() here and remove "MaybeEnqueuePerformanceEntry" ?

[cvazac, 2017-04-17 17:48:42]

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

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:69: static const
size_t kServerTimingBufferSize = 1000;
Was a WAG, changed to 150! :)

On 2017/04/14 19:31:04, panicker wrote:
> Curious - How did you come up with 1000?
> This seems quite high, while this is probably okay for kServerTimingBufferSize
> is isolation, remember that every PerformanceEntry type now needs this buffer.
> Many thousands of entries is not okay as there is no plan to free this up.
> Could we go with the 150 limit?
> 
> BTW At the least the spec should indicate that under memory pressure, the
system
> is free to delete these entries (and advise developers to collect these
entries
> early).

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:213: // expect this
buffer to be too big.
On 2017/04/14 19:31:04, panicker wrote:
> can you update this comment to reflect that the buffer size is per-determined?

Acknowledged.

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:521: for (auto&
observer : observers_) {
I'm fairly certain we still need this logic to determine if the observer cares
about that particular entryType.
```
if (FilterOptions() & entry.EntryTypeEnum())
```


On 2017/04/14 19:31:04, panicker wrote:
> call NotifyObserversOfEntry() here and remove "MaybeEnqueuePerformanceEntry" ?

[panicker, 2017-04-18 17:07:32]

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

https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:521: for (auto&
observer : observers_) {
On 2017/04/17 17:48:42, cvazac wrote:
> I'm fairly certain we still need this logic to determine if the observer cares
> about that particular entryType.
> ```
> if (FilterOptions() & entry.EntryTypeEnum())
> ```
> 
> 
> On 2017/04/14 19:31:04, panicker wrote:
> > call NotifyObserversOfEntry() here and remove "MaybeEnqueuePerformanceEntry"
?

This is already there on line #513 above

[cvazac, 2017-04-18 21:48:41]

On 2017/04/18 17:07:32, panicker wrote:
>
https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> 
>
https://codereview.chromium.org/2799093004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/timing/PerformanceBase.cpp:521: for (auto&
> observer : observers_) {
> On 2017/04/17 17:48:42, cvazac wrote:
> > I'm fairly certain we still need this logic to determine if the observer
cares
> > about that particular entryType.
> > ```
> > if (FilterOptions() & entry.EntryTypeEnum())
> > ```
> > 
> > 
> > On 2017/04/14 19:31:04, panicker wrote:
> > > call NotifyObserversOfEntry() here and remove
"MaybeEnqueuePerformanceEntry"
> ?
> 
> This is already there on line #513 above

Ahhh! My fault, merge failure. :(
Check out my latest, I have `NotifyObserversOfEntries()` using
`MaybeEnqueuePerformanceEntry()`, which was the whole idea to begin with, to
reuse that logic.

[panicker, 2017-04-18 21:57:45]

https://codereview.chromium.org/2799093004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2799093004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:520: for (auto&
observer : observers_) {
you can still call local NotifyObserversOfEntry here (avoid DRY violation).
BTW I prefer to keep the filter check here instead of adding 2 variants to
observer: Enqueue and MaybeEnqueue; 
Otherwise let's atleast reduce it to one method vs. two, only thing using
Enqueue is a unittest currently.

[panicker, 2017-04-20 18:19:36]

On 2017/04/18 21:57:45, panicker wrote:
>
https://codereview.chromium.org/2799093004/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> 
>
https://codereview.chromium.org/2799093004/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/timing/PerformanceBase.cpp:520: for (auto&
> observer : observers_) {
> you can still call local NotifyObserversOfEntry here (avoid DRY violation).
> BTW I prefer to keep the filter check here instead of adding 2 variants to
> observer: Enqueue and MaybeEnqueue; 
> Otherwise let's atleast reduce it to one method vs. two, only thing using
> Enqueue is a unittest currently.

Friendly ping. This is very close and I'd love to see it land.

[cvazac, 2017-04-20 18:28:21]

On 2017/04/20 18:19:36, panicker wrote:
> On 2017/04/18 21:57:45, panicker wrote:
> >
>
https://codereview.chromium.org/2799093004/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2799093004/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/timing/PerformanceBase.cpp:520: for (auto&
> > observer : observers_) {
> > you can still call local NotifyObserversOfEntry here (avoid DRY violation).
> > BTW I prefer to keep the filter check here instead of adding 2 variants to
> > observer: Enqueue and MaybeEnqueue; 
> > Otherwise let's atleast reduce it to one method vs. two, only thing using
> > Enqueue is a unittest currently.
> 
> Friendly ping. This is very close and I'd love to see it land.

Yep! I got this in yesterday, see patch #8. Sorry I didn't email as well!

[panicker, 2017-04-20 18:32:28]

LGTM
Thanks for your work on this!

[cvazac, 2017-04-20 23:10:56]

my pleasure! is there anything i need to do?

[Yoav Weiss, 2017-04-21 20:02:20]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 20:02:47]

Dry run: 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-04-21 20:02:49]

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

[commit-bot: I haz the power, 2017-04-21 20:02:51]

Dry run: The author cvazac@gmail.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[panicker, 2017-04-24 17:43:19]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-04-24 17:43:19]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
csharrison@chromium.org, panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2799093004/#ps160001
(title: "updates for Patch Set 7")

[commit-bot: I haz the power, 2017-04-24 17:44:03]

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-04-24 17:44:04]

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

[commit-bot: I haz the power, 2017-04-24 17:44:05]

The author cvazac@gmail.com has not signed Google Contributor License Agreement.
Please visit https://cla.developers.google.com to sign and manage CLA.