Bug 167185 – Internals should not reset RuntimeEnabledFeatures, they should be set consistently by TestRunners

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Joseph Pecoraro]

Summary:
Internals should not reset RuntimeEnabledFeatures, they should be set consistently by TestRunners

--

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

Possible issues:

    - There may be a RuntimeEnabled feature for which there is no WebPreferences API to toggle it.
      - Seems in these cases we should just add an API.

[Joseph Pecoraro]

Created attachment 299205 [details]
[PATCH] Proposed Fix

Lets see what the bots think.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/cache/vary-frameless-document.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm

[Build Bot]

Created attachment 299207 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Joseph Pecoraro]

Ugh. It seems I'm not going to be able to separate this from making a flag for ResourceTiming because existing ResourceTiming tests depend on this reset happening. Adding a runtime toggle for the test runners is the same as bug 167147.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/cache/vary-frameless-document.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm
js/dom/global-constructors-attributes.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
http/tests/cache/vary-frameless-document.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm

[Build Bot]

Created attachment 299212 [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 299205 [details]
[PATCH] Proposed Fix

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/resource_dynamic_insertion.html
imported/w3c/web-platform-tests/resource-timing/resource_connection_reuse.html
imported/w3c/web-platform-tests/resource-timing/resource_cached.htm

[Build Bot]

Created attachment 299213 [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

[Simon Fraser (smfr)]

So glad you're fixing this!

[Joseph Pecoraro]

I'm going to fold this into bug 167147.

Some tests enable ResourceTiming and was no way to disable ResourceTiming from TestRunners. Rather then repeat the work across two bugs, I'm putting it all in one.

*** This bug has been marked as a duplicate of bug 167147 ***

[Joseph Pecoraro]

Now that I'm separating the original up into smaller pieces I'll reuse this one for the task in the title.

[Joseph Pecoraro]

This was handled by bug 167228.

*** This bug has been marked as a duplicate of bug 167228 ***