Bug 167147 – [Resource Timing] Add Experimental Feature Flag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

Summary:
Add runtime flag for Resource Timing API.

This will make it easier to develop. We currently have partial support and pass 18/28 of the w3c tests, failing tests for "encodedBodySize" and "protocol" properties.

[Joseph Pecoraro]

Created attachment 299086 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 299090 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 299090 [details]
[PATCH] Proposed Fix

Attachment 299090 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2906632

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 299095 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299090 [details]
[PATCH] Proposed Fix

Attachment 299090 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2906685

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 299097 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299090 [details]
[PATCH] Proposed Fix

Attachment 299090 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2906739

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 299098 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 299090 [details]
[PATCH] Proposed Fix

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

r=me assuming you fix the build as indicated below.

> Source/WebKit/mac/WebView/WebView.mm:2913
> +    RuntimeEnabledFeatures::sharedFeatures().setSubtleCryptoEnabled(preferences.resourceTimingEnabled);
> +

Don't disable subtle crypto LOL.

> Websites/webkit.org/experimental-features.html:109
> +    return window.performance.clearResourceTimings;

We probably want to detect the existence of window.PerformanceResourceTiming instead
since I'd really consider this API on Performance interface as a part of timeline API we're trying to move away from.

[Joseph Pecoraro]

Created attachment 299103 [details]
[PATCH] For Landing

[Build Bot]

Comment on attachment 299103 [details]
[PATCH] For Landing

Attachment 299103 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2907066

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 299111 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

(In reply to comment #11)
> Comment on attachment 299103 [details]
> [PATCH] For Landing
> 
> Attachment 299103 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/2907066
> 
> New failing tests:
> js/dom/global-constructors-attributes.html

My guess is this is failing because RuntimeEnabledFeatures is not reset properly between tests.

[Joseph Pecoraro]

Wow, I didn't know this. WebkitTestRunner enables all experimental features:

TestController::resetPreferencesToConsistentValues:

    WKPreferencesEnableAllExperimentalFeatures(preferences);

Which explains why this failure only happens on WebKit2 and not WebKit1.

[Joseph Pecoraro]

Created attachment 299177 [details]
[PATCH] Proposed Fix

Now that I understand how LayoutTests are supposed to handle this. Lets try another fresh patch.

[Build Bot]

Comment on attachment 299177 [details]
[PATCH] Proposed Fix

Attachment 299177 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2911407

New failing tests:
http/tests/performance/performance-resource-timing-entries.html
http/tests/performance/performance-resource-timing-entries-iterable.html
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/performance/performance-resource-timing-initiator-no-override.html
media/controls/track-menu.html
fast/dom/Window/window-properties-performance-resource-timing.html
media/controls/elementOrder.html
http/tests/performance/performance-resource-timing-cached-entries.html
media/audio-concurrent-supported.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
media/controls/statusDisplay.html
http/tests/preload/dynamic_remove_preload_href.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm
http/tests/misc/resource-timing-resolution.html
http/tests/preload/single_download_preload.html
http/tests/performance/performance-resource-timing-xhr-single-entry.html
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html
http/tests/performance/performance-resource-timing-initiator-css.html

[Build Bot]

Created attachment 299182 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

(In reply to comment #16)
> Comment on attachment 299177 [details]
> [PATCH] Proposed Fix
> 
> Attachment 299177 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/2911407
> 
> New failing tests:
> http/tests/performance/performance-resource-timing-entries.html
> http/tests/performance/performance-resource-timing-entries-iterable.html

My code search tool has been ignoring a "test" directory for years... Thats tremendously sad.

[Build Bot]

Comment on attachment 299177 [details]
[PATCH] Proposed Fix

Attachment 299177 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2911476

New failing tests:
http/tests/performance/performance-resource-timing-entries.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/performance/performance-resource-timing-entries-iterable.html
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html
fast/dom/Window/window-properties-performance-resource-timing.html
http/tests/performance/performance-resource-timing-cached-entries.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
http/tests/preload/dynamic_remove_preload_href.html
http/tests/performance/performance-resource-timing-initiator-no-override.html
http/tests/misc/resource-timing-resolution.html
http/tests/preload/single_download_preload.html
http/tests/performance/performance-resource-timing-xhr-single-entry.html
http/tests/performance/performance-resource-timing-initiator-css.html

[Build Bot]

Created attachment 299189 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299177 [details]
[PATCH] Proposed Fix

Attachment 299177 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2911556

New failing tests:
http/tests/performance/performance-resource-timing-entries.html
http/tests/performance/performance-resource-timing-entries-iterable.html
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/performance/performance-resource-timing-initiator-no-override.html
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html
fast/dom/Window/window-properties-performance-resource-timing.html
http/tests/performance/performance-resource-timing-cached-entries.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
http/tests/preload/dynamic_remove_preload_href.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm
http/tests/misc/resource-timing-resolution.html
http/tests/preload/single_download_preload.html
http/tests/performance/performance-resource-timing-xhr-single-entry.html
media/audio-delete-while-slider-thumb-clicked.html
http/tests/performance/performance-resource-timing-initiator-css.html

[Build Bot]

Created attachment 299191 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299177 [details]
[PATCH] Proposed Fix

Attachment 299177 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2911551

New failing tests:
http/tests/performance/performance-resource-timing-entries.html
http/tests/performance/performance-resource-timing-initiator-no-override.html
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/performance/performance-resource-timing-entries-iterable.html
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html
fast/dom/Window/window-properties-performance-resource-timing.html
http/tests/performance/performance-resource-timing-cached-entries.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
http/tests/preload/dynamic_remove_preload_href.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm
http/tests/misc/resource-timing-resolution.html
http/tests/preload/single_download_preload.html
http/tests/performance/performance-resource-timing-xhr-single-entry.html
http/tests/performance/performance-resource-timing-initiator-css.html

[Build Bot]

Created attachment 299195 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Tests are very confused about RuntimeEnabledFeatures.

    DumpRenderTree:
      - DumpRenderTree.mm (Mac): resetWebPreferencesToConsistentValues sets a number of WebPreferences APIs that toggle RuntimeEnabledFeatures
      - DumpRenderTree.cpp (Win): resetWebPreferencesToConsistentValues sets a number of WebPreferences APIs that toggle RuntimeEnabledFeatures

    WebKitTestRunner:
      - TestController.cpp (All): TestController::resetPreferencesToConsistentValues toggles RuntimeEnabledFeatures (WKPreferencesEnableAllExperimentalFeatures)

    WebCoreTestSupport:
      - Internals.cpp (All): Internals::Internals resets RuntimeEnabledFeatures without taking into account DumpRenderTree/WebKitTestRunner non-defaults
      - Internals.cpp (All): Various APIs toggle RuntimeEnabledFeatures while a test is running

Internals stomps on the Test Runners values, but only sometimes. This results in very confusing results where sometimes a Runtime feature is enabled and sometimes disabled.

I'm going to propose some guidelines:

    1. DumpRenderTree / WebKitTestRunner should configure settings appropriately
      - They already do between tests with the appropriately named resetPreferencesToConsistentValues.
      - This allows our tests to have things like experimental features enabled for tests even if they are not enabled by default for ports.
        - WebKit2 currently does this automatically.
        - WebKit1 developers / reviewers will need to remember to follow the pattern.

    2. Internals should not reset RuntimeEnabledFeatures
      - The values in RuntimeEnabledFeatures::reset may not match the defaults set by TestRunners.
      - Internals doesn't know what features the TestRunners will want.

    3. Internal toggle APIs can exist
      - If a test really wants to enable/disable a runtime feature this is a quick and easy way to do it. This should be, and is, super rare.
      - Tests shouldn't need to remember to restore the default value. The test runners should already be doing that, see (1).

[Joseph Pecoraro]

Created attachment 299209 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 299216 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 299216 [details]
[PATCH] Proposed Fix

Attachment 299216 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2912839

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html
http/tests/cache/vary-frameless-document.html

[Build Bot]

Created attachment 299219 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299216 [details]
[PATCH] Proposed Fix

Attachment 299216 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2912863

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html
http/tests/cache/vary-frameless-document.html

[Build Bot]

Created attachment 299221 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299216 [details]
[PATCH] Proposed Fix

Attachment 299216 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2912868

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html
http/tests/cache/vary-frameless-document.html

[Build Bot]

Created attachment 299223 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 299216 [details]
[PATCH] Proposed Fix

Attachment 299216 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2912869

New failing tests:
http/tests/preload/dynamic_remove_preload_href.html
http/tests/cache/vary-frameless-document.html

[Build Bot]

Created attachment 299225 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 299274 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Bots look green. This should be reviewable now.

If reviewers want, I can break this up into two pieces, but to keep tests green I'll want to land them at the same time.

[Simon Fraser (smfr)]

Comment on attachment 299274 [details]
[PATCH] Proposed Fix

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

I'm a bit confused with this patch how RuntimeEnabledFeature changes don't leak between tests?

> LayoutTests/http/tests/performance/performance-resource-timing-cached-entries-expected.txt:4
> +CONSOLE MESSAGE: line 9: current entries: 0
> +CONSOLE MESSAGE: line 18: entry: http://127.0.0.1:8000/resources/js-test-pre.js
> +CONSOLE MESSAGE: line 18: entry: http://127.0.0.1:8000/resources/square100.png
> +CONSOLE MESSAGE: line 18: entry: http://127.0.0.1:8000/resources/square100.png

I hate it when this happens.

> Source/WebCore/page/RuntimeEnabledFeatures.h:271
> +    bool m_isDeviceOrientationEnabled { true };
> +    bool m_isShadowDOMEnabled { true };
> +    bool m_areCustomElementsEnabled { true };
> +    bool m_inputEventsEnabled { true };
> +
> +    bool m_areModernMediaControlsEnabled { false };
> +    bool m_isLangAttributeAwareFormControlUIEnabled { false };
> +    bool m_isLinkPreloadEnabled { false };
> +    bool m_isResourceTimingEnabled { false };

The random true/false here are hilarious (not your problem).

> Tools/MiniBrowser/mac/SettingsController.m:178
> +        item.toolTip = feature.details;

Is this related?

[Joseph Pecoraro]

Comment on attachment 299274 [details]
[PATCH] Proposed Fix

This is getting out of hand. I'm going to break this up into smaller pieces.

[Joseph Pecoraro]

Comment on attachment 299274 [details]
[PATCH] Proposed Fix

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

>> Tools/MiniBrowser/mac/SettingsController.m:178
>> +        item.toolTip = feature.details;
> 
> Is this related?

This is just improving the Experimental Features menu of mini browser to show the details string.

For example in this new experimental feature:

    macro(ResourceTimingEnabled, resourceTimingEnabled, Bool, bool, false, "Resource Timing", "Resource Timing API support") \

It would be the last string.

[Ryosuke Niwa]

Has this been landed?

[Joseph Pecoraro]

(In reply to comment #42)
> Has this been landed?

No. And I plan on handling this separately later.

[Yoav Weiss]

Wanted to add that I'd love to see this land, as it would enable us to test the link preload feature with WPT imported tests.

[Joseph Pecoraro]

I don't feel confident enough yet to add a runtime flag for Resource Timing as it exists today:

  (1) Needs more Tests
    - web-platform-tests has very few tests testing very limited functionality
    - we don't have timing for Synchronous XHRs (which those tests depend on)

  (2) Update Implementation
    - ResourceTiming Level 2 has Worker support
    - Initial version should work with PerformanceObservers now

Though it would make development easier if this was an experimental feature, I want to approach this responsibly. I will get to as soon as I write enough tests.

[Yoav Weiss]

(In reply to comment #45)
> I don't feel confident enough yet to add a runtime flag for Resource Timing
> as it exists today:
> 
>   (1) Needs more Tests
>     - web-platform-tests has very few tests testing very limited
> functionality
>     - we don't have timing for Synchronous XHRs (which those tests depend on)
> 
>   (2) Update Implementation
>     - ResourceTiming Level 2 has Worker support
>     - Initial version should work with PerformanceObservers now
> 
> Though it would make development easier if this was an experimental feature,
> I want to approach this responsibly. I will get to as soon as I write enough
> tests.

I totally agree on your points regarding the need for more tests and implementation updates, but I'm not sure what is the risk of enabling this as an experimental feature. What do you have in mind?

Maybe we can enable this by default for tests without exposing it via a flag to STP? If that's OK, I'd be happy to take on that work.

[Ryosuke Niwa]

(In reply to comment #46)
> (In reply to comment #45)
> 
> I totally agree on your points regarding the need for more tests and
> implementation updates, but I'm not sure what is the risk of enabling this
> as an experimental feature. What do you have in mind?
> 
> Maybe we can enable this by default for tests without exposing it via a flag
> to STP? If that's OK, I'd be happy to take on that work.

The risk is that it would make new STP less usable. e.g. it can cause a random crash, cause pages to fail, etc...

[Yoav Weiss]

(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #45)
> > 
> > I totally agree on your points regarding the need for more tests and
> > implementation updates, but I'm not sure what is the risk of enabling this
> > as an experimental feature. What do you have in mind?
> > 
> > Maybe we can enable this by default for tests without exposing it via a flag
> > to STP? If that's OK, I'd be happy to take on that work.
> 
> The risk is that it would make new STP less usable. e.g. it can cause a
> random crash, cause pages to fail, etc...

Fair enough. Would you object then to turning on the feature for tests, but not as an STP experimental feature? Is there a risk there?

[Joseph Pecoraro]

I was more concerned with sites getting incomplete / inaccurate data. I did at least one crash that I've tackled.

I really like the idea of turning the feature on in tests even before making it experimental. I'll do that tomorrow (well later today!), it will be a big step forward!

[Yoav Weiss]

(In reply to comment #49)
> I was more concerned with sites getting incomplete / inaccurate data. I did
> at least one crash that I've tackled.
> 
> I really like the idea of turning the feature on in tests even before making
> it experimental. I'll do that tomorrow (well later today!), it will be a big
> step forward!

Thank you! :)

[Joseph Pecoraro]

> I really like the idea of turning the feature on in tests even before making
> it experimental. I'll do that tomorrow (well later today!), it will be a big
> step forward!

Bug 168145.

[Joseph Pecoraro]

After writing a ton of tests and modernizing the API a bit, I will be comfortable making this an experimental feature after bug 168086. But I'll probably still wait until after bug 168351. I'll revisit this once those land.

[Joseph Pecoraro]

Comment on attachment 299274 [details]
[PATCH] Proposed Fix

This patch is obsoleted. See comments.

[Joseph Pecoraro]

Created attachment 302186 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/212945>