Move ResourceTiming behind a runtime flag

• Component: New Bugs
• Status: RESOLVED FIXED

[Yoav Weiss]

Move ResourceTiming behind a runtime flag

[Yoav Weiss]

Created attachment 277614 [details]
Patch

[Yoav Weiss]

Created attachment 277617 [details]
Patch

[Yoav Weiss]

Created attachment 277620 [details]
Patch

[Yoav Weiss]

Created attachment 277621 [details]
Patch

[Alex Christensen]

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
> [phone]-18  Brady Eidson  <[e-mail]>

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 //;?

[Yoav Weiss]

Comment on attachment 277621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277621&action=review

>> Source/WebCore/ChangeLog:5248
>> [phone]-18  Brady Eidson  <[e-mail]>
> 
> 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).

[Yoav Weiss]

(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().

[Yoav Weiss]

Created attachment 277646 [details]
Patch

[Yoav Weiss]

Created attachment 277648 [details]
Patch

[Yoav Weiss]

review comments addressed. PTAL

[Alex Christensen]

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.

[Yoav Weiss]

Created attachment 277676 [details]
Patch

[Yoav Weiss]

Comment on attachment 277676 [details]
Patch

Thanks for reviewing! :)

[WebKit Commit Bot]

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

[Yoav Weiss]

Created attachment 277678 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277678 [details]
Patch

Clearing flags on attachment: 277678

Committed r200232: <http://trac.webkit.org/changeset/200232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Ryan Haddad]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 157189

[Yoav Weiss]

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

[Yoav Weiss]

Created attachment 277846 [details]
Patch

[Yoav Weiss]

Created attachment 277847 [details]
Patch

[Yoav Weiss]

Created attachment 277848 [details]
Patch

[Yoav Weiss]

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)

[Yoav Weiss]

Created attachment 277852 [details]
Patch

[Alex Christensen]

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?

[Yoav Weiss]

(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).

[Yoav Weiss]

Comment on attachment 277852 [details]
Patch

Thanks for reviewing! :)

[WebKit Commit Bot]

Comment on attachment 277852 [details]
Patch

Clearing flags on attachment: 277852

Committed r200320: <http://trac.webkit.org/changeset/200320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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());
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^

[Yoav Weiss]

(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