Bug 157133

Created attachment 277614 [details] Patch

Created attachment 277617 [details] Patch

Created attachment 277620 [details] Patch

Created attachment 277621 [details] Patch

Comment on attachment 277621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277621&action=review Probably r=me. I have two questions: A resourceTimingBufferSize is set. Is a buffer allocated if the feature is disabled at run time? How is the timing data collected? It shouldn't be collected if the feature is disabled at run time. > Source/WebCore/ChangeLog:5248 > +2016-04-28 2016-04-18 Brady Eidson <beidson@apple.com> This change must have been an accident. > Source/WebCore/page/Performance.idl:39 > + [EnabledAtRuntime=ResourceTiming] PerformanceEntryList webkitGetEntries(); Since we've never shipped this before, now would be a good time to unprefix everything in this file. > LayoutTests/fast/dom/Window/window-properties-performance-resource-timing.html:36 > + if (typeof value == "object" && value == null) //; What's with the //;?

Comment on attachment 277621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277621&action=review >> Source/WebCore/ChangeLog:5248 >> +2016-04-28 2016-04-18 Brady Eidson <beidson@apple.com> > > This change must have been an accident. Indeed >> Source/WebCore/page/Performance.idl:39 >> + [EnabledAtRuntime=ResourceTiming] PerformanceEntryList webkitGetEntries(); > > Since we've never shipped this before, now would be a good time to unprefix everything in this file. With pleasure! :) >> LayoutTests/fast/dom/Window/window-properties-performance-resource-timing.html:36 >> + if (typeof value == "object" && value == null) //; > > What's with the //;? I copied this test from another one, and just enabled resource timing . I'll get rid of the "//;" (in both tests).

(In reply to comment #5) > Comment on attachment 277621 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277621&action=review > > Probably r=me. I have two questions: > A resourceTimingBufferSize is set. Is a buffer allocated if the feature is > disabled at run time? > How is the timing data collected? It shouldn't be collected if the feature > is disabled at run time. > When the feature is disabled at runtime, A vector of RefPtr<PerformanceEntry> is created, but nothing is ever appended to it, nor does it get allocated to resourceTimingBufferSize. The timing data is not collected when the feature is disabled, as addResourceTiming() in CachedResourceLoader::loadDone() is never called unless RuntimeEnabledFeatures()::sharedFeatures().resourceTimingEnabled().

Created attachment 277646 [details] Patch

Created attachment 277648 [details] Patch

review comments addressed. PTAL

Comment on attachment 277648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277648&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:79 > #include "Performance.h" This should go up with the other includes. > Source/WebCore/page/Performance.h:69 > + PassRefPtr<PerformanceEntryList> getEntries() const; > + PassRefPtr<PerformanceEntryList> getEntriesByType(const String& entryType); > + PassRefPtr<PerformanceEntryList> getEntriesByName(const String& name, const String& entryType); Not necessary to return PassRefPtr. Can just be RefPtr, or we could do that in another patch. We're trying to get rid of PassRefPtr.

Created attachment 277676 [details] Patch

Comment on attachment 277676 [details] Patch Thanks for reviewing! :)

Comment on attachment 277676 [details] Patch Rejecting attachment 277676 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 277676, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: g file LayoutTests/fast/dom/Window/window-properties-performance-resource-timing.html patching file LayoutTests/fast/dom/Window/window-properties-performance.html patching file LayoutTests/http/tests/performance/performance-resource-timing-entries-expected.txt patching file LayoutTests/http/tests/performance/performance-resource-timing-entries.html patching file ChangeLog Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/1238223

Created attachment 277678 [details] Patch

Comment on attachment 277678 [details] Patch Clearing flags on attachment: 277678 Committed r200232: <http://trac.webkit.org/changeset/200232>

All reviewed patches have been landed. Closing bug.

(In reply to comment #16) > Comment on attachment 277678 [details] > Patch > > Clearing flags on attachment: 277678 > > Committed r200232: <http://trac.webkit.org/changeset/200232> It broke the Apple Mac cmake build, see build.webkit.org for details.

http/tests/performance/performance-resource-timing-entries.html is failing on Mac and flaky on other platforms. http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fperformance%2Fperformance-resource-timing-entries.html

Re-opened since this is blocked by bug 157189

*** Bug 157177 has been marked as a duplicate of this bug. ***

Created attachment 277846 [details] Patch

Created attachment 277847 [details] Patch

Created attachment 277848 [details] Patch

I've updated the tests to be less flaky (Looks like ResourceTiming is not registering resource loads if it gets the resource from, so only the first run of the previous test passed, and all subsequent ones failed). I fixed it by cache busting the tested resource URL. I'll file a bug against that behavior in a separate issue. I also merged the patch from https://bugs.webkit.org/show_bug.cgi?id=157177 which fixed the cmake build. Alex - could you review this new patch? (Mostly the new test, as you already r+ed the rest)

Created attachment 277852 [details] Patch

Comment on attachment 277852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277852&action=review > LayoutTests/http/tests/performance/performance-resource-timing-entries.html:8 > + var url = "../../resources/square100.png?"; > + url += Math.random(); > + document.write("<img src='" + url + "'>"); This definitely will force reloading. There are better ways of doing this, and we should handle caching better, but this is fine to go in if you file a bug to fix that. > LayoutTests/http/tests/performance/performance-resource-timing-entries.html:17 > + var resources = performance.getEntriesByType('resource'); > + for (var i = 0; i < resources.length; ++i) { > + if (resources[i].name.indexOf("square100") != -1) { Last time we just made sure that resources.length was 2. Now we iterate it and make sure square100.png?randomnumber is in it. Was the length 2 before because the main resource and the image were expected to be in it?

(In reply to comment #27) > Comment on attachment 277852 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277852&action=review > > > LayoutTests/http/tests/performance/performance-resource-timing-entries.html:8 > > + var url = "../../resources/square100.png?"; > > + url += Math.random(); > > + document.write("<img src='" + url + "'>"); > > This definitely will force reloading. There are better ways of doing this, > and we should handle caching better, but this is fine to go in if you file a > bug to fix that. Will do. > > > LayoutTests/http/tests/performance/performance-resource-timing-entries.html:17 > > + var resources = performance.getEntriesByType('resource'); > > + for (var i = 0; i < resources.length; ++i) { > > + if (resources[i].name.indexOf("square100") != -1) { > > Last time we just made sure that resources.length was 2. Now we iterate it > and make sure square100.png?randomnumber is in it. Was the length 2 before > because the main resource and the image were expected to be in it? The length was 2 because the of the test framework JS files. I changed the test to look for a specific resource as I didn't want to cache-bust the JS framework, and checking only length now would be flaky, as the JS would be cached, while the image will not be (so length would be 2 on the first run, and 1 in following runs).

Comment on attachment 277852 [details] Patch Thanks for reviewing! :)

Comment on attachment 277852 [details] Patch Clearing flags on attachment: 277852 Committed r200320: <http://trac.webkit.org/changeset/200320>

All reviewed patches have been landed. Closing bug.

(In reply to comment #30) > Comment on attachment 277852 [details] > Patch > > Clearing flags on attachment: 277852 > > Committed r200320: <http://trac.webkit.org/changeset/200320> It broke the Apple Mac cmake build: /Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/loader/cache/CachedResourceLoader.cpp:983:45: error: no member named 'performance' in 'WebCore::DOMWindow' initiatorDocument->domWindow()->performance()->addResourceTiming(info.name, initiatorDocument, resource->resourceRequest(), resource->response(), info.startTime, resource->loadFinishTime()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^

(In reply to comment #32) > (In reply to comment #30) > > Comment on attachment 277852 [details] > > Patch > > > > Clearing flags on attachment: 277852 > > > > Committed r200320: <http://trac.webkit.org/changeset/200320> > > It broke the Apple Mac cmake build: > > /Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/loader/cache/ > CachedResourceLoader.cpp:983:45: error: no member named 'performance' in > 'WebCore::DOMWindow' > > initiatorDocument->domWindow()->performance()->addResourceTiming(info.name, > initiatorDocument, resource->resourceRequest(), resource->response(), > info.startTime, resource->loadFinishTime()); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ Speculative fix at https://bugs.webkit.org/show_bug.cgi?id=157262