Bug 157133

Summary: Move ResourceTiming behind a runtime flag
Product: WebKit Reporter: Yoav Weiss <yoav>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, ossy, ryanhaddad
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 157189    
Bug Blocks: 61138    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Yoav Weiss 2016-04-28 05:44:01 PDT
Move ResourceTiming behind a runtime flag
Comment 1 Yoav Weiss 2016-04-28 06:09:10 PDT
Created attachment 277614 [details]
Patch
Comment 2 Yoav Weiss 2016-04-28 06:44:40 PDT
Created attachment 277617 [details]
Patch
Comment 3 Yoav Weiss 2016-04-28 07:01:51 PDT
Created attachment 277620 [details]
Patch
Comment 4 Yoav Weiss 2016-04-28 08:29:43 PDT
Created attachment 277621 [details]
Patch
Comment 5 Alex Christensen 2016-04-28 10:08:33 PDT
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 6 Yoav Weiss 2016-04-28 12:18:56 PDT
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).
Comment 7 Yoav Weiss 2016-04-28 12:25:14 PDT
(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().
Comment 8 Yoav Weiss 2016-04-28 13:44:34 PDT
Created attachment 277646 [details]
Patch
Comment 9 Yoav Weiss 2016-04-28 14:07:02 PDT
Created attachment 277648 [details]
Patch
Comment 10 Yoav Weiss 2016-04-28 14:23:38 PDT
review comments addressed. PTAL
Comment 11 Alex Christensen 2016-04-28 21:16:37 PDT
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.
Comment 12 Yoav Weiss 2016-04-28 23:32:11 PDT
Created attachment 277676 [details]
Patch
Comment 13 Yoav Weiss 2016-04-28 23:33:05 PDT
Comment on attachment 277676 [details]
Patch

Thanks for reviewing! :)
Comment 14 WebKit Commit Bot 2016-04-28 23:35:09 PDT
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
Comment 15 Yoav Weiss 2016-04-28 23:38:33 PDT
Created attachment 277678 [details]
Patch
Comment 16 WebKit Commit Bot 2016-04-29 00:32:33 PDT
Comment on attachment 277678 [details]
Patch

Clearing flags on attachment: 277678

Committed r200232: <http://trac.webkit.org/changeset/200232>
Comment 17 WebKit Commit Bot 2016-04-29 00:32:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Csaba Osztrogonác 2016-04-29 02:13:56 PDT
(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.
Comment 19 Ryan Haddad 2016-04-29 10:21:48 PDT
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
Comment 20 WebKit Commit Bot 2016-04-29 10:36:10 PDT
Re-opened since this is blocked by bug 157189
Comment 21 Yoav Weiss 2016-04-29 12:21:48 PDT
*** Bug 157177 has been marked as a duplicate of this bug. ***
Comment 22 Yoav Weiss 2016-05-01 02:06:39 PDT
Created attachment 277846 [details]
Patch
Comment 23 Yoav Weiss 2016-05-01 02:20:59 PDT
Created attachment 277847 [details]
Patch
Comment 24 Yoav Weiss 2016-05-01 02:26:03 PDT
Created attachment 277848 [details]
Patch
Comment 25 Yoav Weiss 2016-05-01 02:32:55 PDT
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)
Comment 26 Yoav Weiss 2016-05-01 03:08:59 PDT
Created attachment 277852 [details]
Patch
Comment 27 Alex Christensen 2016-05-02 00:19:54 PDT
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?
Comment 28 Yoav Weiss 2016-05-02 00:24:25 PDT
(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 29 Yoav Weiss 2016-05-02 00:29:38 PDT
Comment on attachment 277852 [details]
Patch

Thanks for reviewing! :)
Comment 30 WebKit Commit Bot 2016-05-02 01:21:16 PDT
Comment on attachment 277852 [details]
Patch

Clearing flags on attachment: 277852

Committed r200320: <http://trac.webkit.org/changeset/200320>
Comment 31 WebKit Commit Bot 2016-05-02 01:21:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Csaba Osztrogonác 2016-05-02 02:38:12 PDT
(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());
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^
Comment 33 Yoav Weiss 2016-05-02 02:54:54 PDT
(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