Bug 84885

I think we need to choose a smaller initial number. I know the spec says 150, but I think the spec is wrong. We can always change it later if it turns out we're wrong. I'd be fine with the default buffer size being 0 or being something small enough to give time for the first script to run (e.g. 5-10). We shouldn't be wasting memory on every page for this when a incredibly tiny percentage of pages will actually use this API.

(In reply to comment #1) > I think we need to choose a smaller initial number. I know the spec says 150, but I think the spec is wrong. We can always change it later if it turns out we're wrong. > > I'd be fine with the default buffer size being 0 or being something small enough to give time for the first script to run (e.g. 5-10). > > We shouldn't be wasting memory on every page for this when a incredibly tiny percentage of pages will actually use this API. That's fine with me. I wouldn't totally discount its use though. Every analytics tool will use this data. We put the buffer in, at their request, so that they could continue to load their script after load fires and still have access to the site's data. I'd imagine a significant percentage of sites use analytics.

(In reply to comment #2) > (In reply to comment #1) > > I think we need to choose a smaller initial number. I know the spec says 150, but I think the spec is wrong. We can always change it later if it turns out we're wrong. > > > > I'd be fine with the default buffer size being 0 or being something small enough to give time for the first script to run (e.g. 5-10). > > > > We shouldn't be wasting memory on every page for this when a incredibly tiny percentage of pages will actually use this API. > > That's fine with me. I wouldn't totally discount its use though. Every analytics tool will use this data. We put the buffer in, at their request, so that they could continue to load their script after load fires and still have access to the site's data. I'd imagine a significant percentage of sites use analytics. To add some stats, 46% of the Quantcast top million sites are currently using Analytics: http://trends.builtwith.com/analytics/Google-Analytics

Created attachment 161955 [details] Patch

Comment on attachment 161955 [details] Patch Attachment 161955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13732953

Comment on attachment 161955 [details] Patch Attachment 161955 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13734631

Comment on attachment 161955 [details] Patch Attachment 161955 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13746260

Comment on attachment 161955 [details] Patch Attachment 161955 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13746263

Comment on attachment 161955 [details] Patch Attachment 161955 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13739362

View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review > Source/WebCore/page/Performance.cpp:54 > + m_resourceTimingBufferSize = defaultResourceTimingBufferSize; This should be in the initializer list. > Source/WebCore/page/Performance.cpp:154 > + dispatchEvent(Event::create(eventNames().webkitresourcetimingbufferfullEvent, false, false)); This event is only supposed to fire once when it becomes full, not on every resource that arrives after it's full. If the user clears the buffer during the event, then we should go ahead and record this resource. I started a thread about this on web-perf. My goal for the thread was to fire the event after append() makes it full. > Source/WebCore/page/Performance.h:99 > + static const size_t defaultResourceTimingBufferSize = 150; This doesn't need to be in the .h file. > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:8 > + performance.webkitSetResourceTimingBufferSize(2); 2 -> buffer_size > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:24 > + test_greater_than(count, 0, "onresourcetimingbufferfull should have been invoked!"); It should only happen once. > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:33 > +</html> We need more test cases. For instance, if you shrink the buffer size, it shouldn't modify the buffer until clear is called. Also, something that reads and clears the buffer during the callback.

(In reply to comment #10) > View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review > > > Source/WebCore/page/Performance.cpp:54 > > + m_resourceTimingBufferSize = defaultResourceTimingBufferSize; > > This should be in the initializer list. > > > Source/WebCore/page/Performance.cpp:154 > > + dispatchEvent(Event::create(eventNames().webkitresourcetimingbufferfullEvent, false, false)); > > This event is only supposed to fire once when it becomes full, not on every resource that arrives after it's full. > > If the user clears the buffer during the event, then we should go ahead and record this resource. > Good point. > I started a thread about this on web-perf. My goal for the thread was to fire the event after append() makes it full. one exception is, what if a smaller buffer size is set when previous buffer size is not achieved yet? an extreme scenario is buffer size is set to 0 before any append() happen, I prefer fire the event in this condition if that is not fired on every resource that arrives after full. in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group?

[1] http://www.w3.org/TR/2012/CR-resource-timing-20120522/#dom-performance-onresourcetimingbufferfull

(In reply to comment #11) > one exception is, what if a smaller buffer size is set when previous buffer size is not achieved yet? Good example. Do you want to raise that on the list? I vote we fire the event once immediately in that case. > an extreme scenario is buffer size is set to 0 before any append() happen, I prefer fire the event in this condition if that is not fired on every resource that arrives after full. That's an interesting idea and is inline with what I initially proposed for Resource Timing (I hate this buffer). I think it should be brought up on the list though. > in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group? This line doesn't apply to us. WebKit is single threaded. We can't add another resource to the buffer while the event is firing.

> > in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group? > > This line doesn't apply to us. WebKit is single threaded. We can't add another resource to the buffer while the event is firing. Agree this, one thing puzzles me is, item 5.1.20 of processing model in [1] describes, a temp buffer should be used to collect beyond limit timing entries, while nothing related mentioned in section 4. I've raised this issue to web perf group, unfortunately no feedback. my preference is drop the beyond timing entries as honor user's choice.

(In reply to comment #14) > > > in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group? > > > > This line doesn't apply to us. WebKit is single threaded. We can't add another resource to the buffer while the event is firing. Actually, I thought of one: sync XHR. But that's a pretty extreme case and I really don't care whether or not it works.

Comment on attachment 161955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review r- for the issues raised by simonjam. This is looking close, just needs a little more work. > Source/WebCore/ChangeLog:3 > + [Resource Timing]Implementation of reource timing buffer size restriction functionality s/reourse/resource/ > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:25 > + <iframe id="frameContext" onload=onload_test() src="/w3c/webperf/resources/blank_page_green.htm" style="width: 250px; height: 250px;"></iframe> These three tests don't look like they need to have an iframe. Could you please simplifying them and just put the onload handler in <body>.

Created attachment 168887 [details] Patch

(In reply to comment #10) > View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review > > > Source/WebCore/page/Performance.cpp:54 > > + m_resourceTimingBufferSize = defaultResourceTimingBufferSize; > > This should be in the initializer list. > Done. > > Source/WebCore/page/Performance.cpp:154 > > + dispatchEvent(Event::create(eventNames().webkitresourcetimingbufferfullEvent, false, false)); > > This event is only supposed to fire once when it becomes full, not on every resource that arrives after it's full. > > If the user clears the buffer during the event, then we should go ahead and record this resource. > > I started a thread about this on web-perf. My goal for the thread was to fire the event after append() makes it full. > now, it event fire after append() makes it full, and shrink a less size. > > Source/WebCore/page/Performance.h:99 > > + static const size_t defaultResourceTimingBufferSize = 150; > > This doesn't need to be in the .h file. > Done. > > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:8 > > + performance.webkitSetResourceTimingBufferSize(2); > > 2 -> buffer_size > Done > > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:24 > > + test_greater_than(count, 0, "onresourcetimingbufferfull should have been invoked!"); > > It should only happen once. > Done, happen once now. > > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:33 > > +</html> > > We need more test cases. For instance, if you shrink the buffer size, it shouldn't modify the buffer until clear is called. Also, something that reads and clears the buffer during the callback. Good idea, I add some typical use case of this

(In reply to comment #16) > (From update of attachment 161955 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review > > r- for the issues raised by simonjam. This is looking close, just needs a little more work. > > > Source/WebCore/ChangeLog:3 > > + [Resource Timing]Implementation of reource timing buffer size restriction functionality > > s/reourse/resource/ > > > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:25 > > + <iframe id="frameContext" onload=onload_test() src="/w3c/webperf/resources/blank_page_green.htm" style="width: 250px; height: 250px;"></iframe> > > These three tests don't look like they need to have an iframe. Could you please simplifying them and just put the onload handler in <body>. Done, thanks!

Comment on attachment 168887 [details] Patch Attachment 168887 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14389118 New failing tests: http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_on_shrink_buffer_size.html http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_buffer_size_restriction.html http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_clear_resource_timing_functionality.html http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_buffer_full_when_populate_entries.html http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_store_and_clear_during_callback.html http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_buffer_full_when_shrink_buffer_size.html

Comment on attachment 168887 [details] Patch You just need to skip the new tests until it is enabled. Everything else looks good.

Created attachment 169376 [details] Patch

(In reply to comment #21) > (From update of attachment 168887 [details]) > You just need to skip the new tests until it is enabled. Everything else looks good. skip all the new test cases. I change test cases to match upstream, including prefix handle, and folder. thanks.

Comment on attachment 169376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169376&action=review > Source/WebCore/page/Performance.cpp:167 > + if (isResourceTimingBufferFull()) For efficiency, can't this go above the creation of the entry?

Created attachment 169510 [details] Patch

(In reply to comment #24) > (From update of attachment 169376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169376&action=review > > > Source/WebCore/page/Performance.cpp:167 > > + if (isResourceTimingBufferFull()) > > For efficiency, can't this go above the creation of the entry? Good idea, done :)

Comment on attachment 169510 [details] Patch Clearing flags on attachment: 169510 Committed r131829: <http://trac.webkit.org/changeset/131829>

All reviewed patches have been landed. Closing bug.