Remove support for experimental: and internal: prefixes from WebKitTe…

…stRunner and DumpRenderTree

https://bugs.webkit.org/show_bug.cgi?id=218569

Reviewed by Tim Horton.

The prefixes were removed from use in tests in r269360.

* DumpRenderTree/TestOptions.cpp:
(WTR::TestOptions::webViewIsCompatibleWithOptions const):
Replace duplicated comparison code with call to operator==.

* TestRunnerShared/TestFeatures.cpp:
(WTR::merge):
Remove special maps for internal and experimental features.

(WTR::operator==):
(WTR::operator!=):
Add operator== support for use by TestOptions in determining compatibility.

(WTR::parseTestHeaderFeature):
(WTR::parseTestHeader):
Split out feature parsing for future use in command line parsing.

* TestRunnerShared/TestFeatures.h:
Remove special maps for internal and experimental features.

* WebKitTestRunner/Options.cpp:
(WTR::handleOptionAcceleratedDrawing):
(WTR::handleOptionRemoteLayerTree):
(WTR::handleOptionShowWebView):
(WTR::handleOptionShowTouches):
(WTR::parseFeature):
(WTR::handleOptionExperimentalFeature):
(WTR::handleOptionInternalFeature):
* WebKitTestRunner/Options.h:
Rather than parsing into bools / extra maps, parse the command line options
directly into a TestFeatures.

* WebKitTestRunner/TestController.cpp:
(WTR::TestController::initialize):
Initialize global features from the new Options' TestFeatures.

(WTR::TestController::resetPreferencesToConsistentValues):
Remove special casing for experimental and internal features, they are now just
generic bool WebPreferences. Move special cases for internal features into TestOptions.

(WTR::TestController::testOptionsForTest const):
Now that global features is not seeded with the default features, construct the
full merge chain starting with the default features instead.

* WebKitTestRunner/TestOptions.cpp:
(WTR::TestOptions::defaults):
Add some additional defaults moved from TestController::resetPreferencesToConsistentValues.

(WTR::TestOptions::hasSameInitializationOptions const):
Use operator== to reduce duplicated code.

* WebKitTestRunner/TestOptions.h:
(WTR::TestOptions::experimentalFeatures const): Deleted.
(WTR::TestOptions::internalDebugFeatures const): Deleted.
Remove now unused extra maps for external and internal features.


Canonical link: https://commits.webkit.org/231228@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@269390 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog

@@ -1,3 +1,67 @@
+2020-11-04  Sam Weinig  <weinig@apple.com>
+
+        Remove support for experimental: and internal: prefixes from WebKitTestRunner and DumpRenderTree
+        https://bugs.webkit.org/show_bug.cgi?id=218569
+
+        Reviewed by Tim Horton.
+
+        The prefixes were removed from use in tests in r269360.
+
+        * DumpRenderTree/TestOptions.cpp:
+        (WTR::TestOptions::webViewIsCompatibleWithOptions const):
+        Replace duplicated comparison code with call to operator==.
+
+        * TestRunnerShared/TestFeatures.cpp:
+        (WTR::merge):
+        Remove special maps for internal and experimental features.
+
+        (WTR::operator==):
+        (WTR::operator!=):
+        Add operator== support for use by TestOptions in determining compatibility.
+
+        (WTR::parseTestHeaderFeature):
+        (WTR::parseTestHeader):
+        Split out feature parsing for future use in command line parsing.
+
+        * TestRunnerShared/TestFeatures.h:
+        Remove special maps for internal and experimental features.
+
+        * WebKitTestRunner/Options.cpp:
+        (WTR::handleOptionAcceleratedDrawing):
+        (WTR::handleOptionRemoteLayerTree):
+        (WTR::handleOptionShowWebView):
+        (WTR::handleOptionShowTouches):
+        (WTR::parseFeature):
+        (WTR::handleOptionExperimentalFeature):
+        (WTR::handleOptionInternalFeature):
+        * WebKitTestRunner/Options.h:
+        Rather than parsing into bools / extra maps, parse the command line options
+        directly into a TestFeatures.
+
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::initialize):
+        Initialize global features from the new Options' TestFeatures. 
+
+        (WTR::TestController::resetPreferencesToConsistentValues):
+        Remove special casing for experimental and internal features, they are now just
+        generic bool WebPreferences. Move special cases for internal features into TestOptions.
+
+        (WTR::TestController::testOptionsForTest const):
+        Now that global features is not seeded with the default features, construct the
+        full merge chain starting with the default features instead.
+
+        * WebKitTestRunner/TestOptions.cpp:
+        (WTR::TestOptions::defaults):
+        Add some additional defaults moved from TestController::resetPreferencesToConsistentValues.
+
+        (WTR::TestOptions::hasSameInitializationOptions const):
+        Use operator== to reduce duplicated code.
+
+        * WebKitTestRunner/TestOptions.h:
+        (WTR::TestOptions::experimentalFeatures const): Deleted.
+        (WTR::TestOptions::internalDebugFeatures const): Deleted.
+        Remove now unused extra maps for external and internal features.
+
 2020-11-04  David Kilzer  <ddkilzer@apple.com>
 
         WebKit should remove unused debug variant support

Tools/DumpRenderTree/TestOptions.cpp

@@ -130,29 +130,9 @@ const std::unordered_map<std::string, TestHeaderKeyType>& TestOptions::keyTypeMa
     return map;
 }
 
-bool TestOptions::webViewIsCompatibleWithOptions(const TestOptions& options) const
+bool TestOptions::webViewIsCompatibleWithOptions(const TestOptions& other) const
 {
-    if (m_features.experimentalFeatures != options.m_features.experimentalFeatures)
-        return false;
-    if (m_features.internalDebugFeatures != options.m_features.internalDebugFeatures)
-        return false;
-    if (m_features.boolWebPreferenceFeatures != options.m_features.boolWebPreferenceFeatures)
-        return false;
-    if (m_features.doubleWebPreferenceFeatures != options.m_features.doubleWebPreferenceFeatures)
-        return false;
-    if (m_features.uint32WebPreferenceFeatures != options.m_features.uint32WebPreferenceFeatures)
-        return false;
-    if (m_features.stringWebPreferenceFeatures != options.m_features.stringWebPreferenceFeatures)
-        return false;
-    if (m_features.boolTestRunnerFeatures != options.m_features.boolTestRunnerFeatures)
-        return false;
-    if (m_features.doubleTestRunnerFeatures != options.m_features.doubleTestRunnerFeatures)
-        return false;
-    if (m_features.stringTestRunnerFeatures != options.m_features.stringTestRunnerFeatures)
-        return false;
-    if (m_features.stringVectorTestRunnerFeatures != options.m_features.stringVectorTestRunnerFeatures)
-        return false;
-    return true;
+    return m_features == other.m_features;
 }
 
 template<typename T> T featureValue(std::string key, T defaultValue, const std::unordered_map<std::string, T>& map)

Tools/TestRunnerShared/TestFeatures.cpp

@@ -42,8 +42,6 @@ template<typename T> void merge(std::unordered_map<std::string, T>& base, const
 void merge(TestFeatures& base, TestFeatures additional)
 {
     // FIXME: This should use std::unordered_map::merge when it is available for all ports.
-    merge(base.experimentalFeatures, additional.experimentalFeatures);
-    merge(base.internalDebugFeatures, additional.internalDebugFeatures);
     merge(base.boolWebPreferenceFeatures, additional.boolWebPreferenceFeatures);
     merge(base.doubleWebPreferenceFeatures, additional.doubleWebPreferenceFeatures);
     merge(base.uint32WebPreferenceFeatures, additional.uint32WebPreferenceFeatures);
@@ -54,6 +52,32 @@ void merge(TestFeatures& base, TestFeatures additional)
     merge(base.stringVectorTestRunnerFeatures, additional.stringVectorTestRunnerFeatures);
 }
 
+bool operator==(const TestFeatures& a, const TestFeatures& b)
+{
+    if (a.boolWebPreferenceFeatures != b.boolWebPreferenceFeatures)
+        return false;
+    if (a.doubleWebPreferenceFeatures != b.doubleWebPreferenceFeatures)
+        return false;
+    if (a.uint32WebPreferenceFeatures != b.uint32WebPreferenceFeatures)
+        return false;
+    if (a.stringWebPreferenceFeatures != b.stringWebPreferenceFeatures)
+        return false;
+    if (a.boolTestRunnerFeatures != b.boolTestRunnerFeatures)
+        return false;
+    if (a.doubleTestRunnerFeatures != b.doubleTestRunnerFeatures)
+        return false;
+    if (a.stringTestRunnerFeatures != b.stringTestRunnerFeatures)
+        return false;
+    if (a.stringVectorTestRunnerFeatures != b.stringVectorTestRunnerFeatures)
+        return false;
+    return true;
+}
+
+bool operator!=(const TestFeatures& a, const TestFeatures& b)
+{
+    return !(a == b);
+}
+
 static bool pathContains(const std::string& pathOrURL, const char* substring)
 {
     return pathOrURL.find(substring) != std::string::npos;
@@ -170,7 +194,7 @@ static std::vector<std::string> parseStringTestHeaderValueAsStringVector(const s
     return result;
 }
 
-static TestFeatures parseTestHeader(std::filesystem::path path, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
+bool parseTestHeaderFeature(TestFeatures& features, std::string key, std::string value, std::filesystem::path path, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
 {
     auto keyType = [&keyTypeMap](auto& key) {
         auto it = keyTypeMap.find(key);
@@ -179,6 +203,48 @@ static TestFeatures parseTestHeader(std::filesystem::path path, const std::unord
         return it->second;
     };
 
+    switch (keyType(key)) {
+    case TestHeaderKeyType::BoolWebPreference:
+        features.boolWebPreferenceFeatures.insert_or_assign(key, parseBooleanTestHeaderValue(value));
+        return true;
+    case TestHeaderKeyType::DoubleWebPreference:
+        features.doubleWebPreferenceFeatures.insert_or_assign(key, parseDoubleTestHeaderValue(value));
+        return true;
+    case TestHeaderKeyType::UInt32WebPreference:
+        features.uint32WebPreferenceFeatures.insert_or_assign(key, parseUInt32TestHeaderValue(value));
+        return true;
+    case TestHeaderKeyType::StringWebPreference:
+        features.stringWebPreferenceFeatures.insert_or_assign(key, value);
+        return true;
+
+    case TestHeaderKeyType::BoolTestRunner:
+        features.boolTestRunnerFeatures.insert_or_assign(key, parseBooleanTestHeaderValue(value));
+        return true;
+    case TestHeaderKeyType::DoubleTestRunner:
+        features.doubleTestRunnerFeatures.insert_or_assign(key, parseDoubleTestHeaderValue(value));
+        return true;
+    case TestHeaderKeyType::StringTestRunner:
+        features.stringTestRunnerFeatures.insert_or_assign(key, value);
+        return true;
+    case TestHeaderKeyType::StringRelativePathTestRunner:
+        features.stringTestRunnerFeatures.insert_or_assign(key, parseStringTestHeaderValueAsRelativePath(value, path));
+        return true;
+    case TestHeaderKeyType::StringURLTestRunner:
+        features.stringTestRunnerFeatures.insert_or_assign(key, parseStringTestHeaderValueAsURL(value));
+        return true;
+    case TestHeaderKeyType::StringVectorTestRunner:
+        features.stringVectorTestRunnerFeatures.insert_or_assign(key, parseStringTestHeaderValueAsStringVector(value));
+        return true;
+
+    case TestHeaderKeyType::Unknown:
+        return false;
+    }
+
+    return false;
+}
+
+static TestFeatures parseTestHeader(std::filesystem::path path, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
+{
     TestFeatures features;
     if (!std::filesystem::exists(path))
         return features;
@@ -214,53 +280,10 @@ static TestFeatures parseTestHeader(std::filesystem::path path, const std::unord
         }
         auto key = pairString.substr(pairStart, equalsLocation - pairStart);
         auto value = pairString.substr(equalsLocation + 1, pairEnd - (equalsLocation + 1));
-
-        if (key.rfind("experimental:") == 0) {
-            key = key.substr(13);
-            features.experimentalFeatures.insert({ key, parseBooleanTestHeaderValue(value) });
-        } else if (key.rfind("internal:") == 0) {
-            key = key.substr(9);
-            features.internalDebugFeatures.insert({ key, parseBooleanTestHeaderValue(value) });
-        }
-
-        switch (keyType(key)) {
-        case TestHeaderKeyType::BoolWebPreference:
-            features.boolWebPreferenceFeatures.insert({ key, parseBooleanTestHeaderValue(value) });
-            break;
-        case TestHeaderKeyType::DoubleWebPreference:
-            features.doubleWebPreferenceFeatures.insert({ key, parseDoubleTestHeaderValue(value) });
-            break;
-        case TestHeaderKeyType::UInt32WebPreference:
-            features.uint32WebPreferenceFeatures.insert({ key, parseUInt32TestHeaderValue(value) });
-            break;
-        case TestHeaderKeyType::StringWebPreference:
-            features.stringWebPreferenceFeatures.insert({ key, value });
-            break;
-
-        case TestHeaderKeyType::BoolTestRunner:
-            features.boolTestRunnerFeatures.insert({ key, parseBooleanTestHeaderValue(value) });
-            break;
-        case TestHeaderKeyType::DoubleTestRunner:
-            features.doubleTestRunnerFeatures.insert({ key, parseDoubleTestHeaderValue(value) });
-            break;
-        case TestHeaderKeyType::StringTestRunner:
-            features.stringTestRunnerFeatures.insert({ key, value });
-            break;
-        case TestHeaderKeyType::StringRelativePathTestRunner:
-            features.stringTestRunnerFeatures.insert({ key, parseStringTestHeaderValueAsRelativePath(value, path) });
-            break;
-        case TestHeaderKeyType::StringURLTestRunner:
-            features.stringTestRunnerFeatures.insert({ key, parseStringTestHeaderValueAsURL(value) });
-            break;
-        case TestHeaderKeyType::StringVectorTestRunner:
-            features.stringVectorTestRunnerFeatures.insert({ key, parseStringTestHeaderValueAsStringVector(value) });
-            break;
-
-        case TestHeaderKeyType::Unknown:
+
+        if (!parseTestHeaderFeature(features, key, value, path, keyTypeMap))
             LOG_ERROR("Unknown key, '%s, in test header in %s", key.c_str(), path.c_str());
-            break;
-        }
-
+
         pairStart = pairEnd + 1;
     }
 

Tools/TestRunnerShared/TestFeatures.h

@@ -35,9 +35,6 @@ namespace WTR {
 struct TestCommand;
 
 struct TestFeatures {
-    std::unordered_map<std::string, bool> experimentalFeatures;
-    std::unordered_map<std::string, bool> internalDebugFeatures;
-
     std::unordered_map<std::string, bool> boolWebPreferenceFeatures;
     std::unordered_map<std::string, double> doubleWebPreferenceFeatures;
     std::unordered_map<std::string, uint32_t> uint32WebPreferenceFeatures;
@@ -49,6 +46,9 @@ struct TestFeatures {
     std::unordered_map<std::string, std::vector<std::string>> stringVectorTestRunnerFeatures;
 };
 
+bool operator==(const TestFeatures&, const TestFeatures&);
+bool operator!=(const TestFeatures&, const TestFeatures&);
+
 void merge(TestFeatures& base, TestFeatures additional);
 
 TestFeatures hardcodedFeaturesBasedOnPathForTest(const TestCommand&);

Tools/WebKitTestRunner/Options.cpp

@@ -66,25 +66,25 @@ static bool handleOptionComplexText(Options& options, const char*, const char*)
 
 static bool handleOptionAcceleratedDrawing(Options& options, const char*, const char*)
 {
-    options.shouldUseAcceleratedDrawing = true;
+    options.features.boolWebPreferenceFeatures.insert_or_assign("AcceleratedDrawingEnabled", true);
     return true;
 }
 
 static bool handleOptionRemoteLayerTree(Options& options, const char*, const char*)
 {
-    options.shouldUseRemoteLayerTree = true;
+    options.features.boolTestRunnerFeatures.insert_or_assign("useRemoteLayerTree", true);
     return true;
 }
 
 static bool handleOptionShowWebView(Options& options, const char*, const char*)
 {
-    options.shouldShowWebView = true;
+    options.features.boolTestRunnerFeatures.insert_or_assign("shouldShowWebView", true);
     return true;
 }
 
 static bool handleOptionShowTouches(Options& options, const char*, const char*)
 {
-    options.shouldShowTouches = true;
+    options.features.boolTestRunnerFeatures.insert_or_assign("shouldShowTouches", true);
     return true;
 }
 
@@ -114,7 +114,7 @@ static bool handleOptionAllowedHost(Options& options, const char*, const char* h
     return true;
 }
 
-static bool parseFeature(std::string_view featureString, std::unordered_map<std::string, bool>& features)
+static bool parseFeature(std::string_view featureString, TestFeatures& features)
 {
     auto strings = split(featureString, '=');
     if (strings.empty() || strings.size() > 2)
@@ -123,18 +123,19 @@ static bool parseFeature(std::string_view featureString, std::unordered_map<std:
     auto featureName = strings[0];
     bool enabled = strings.size() == 1 || strings[1] == "true";
 
-    features.insert({ std::string { featureName }, enabled });
+    // FIXME: Generalize this to work for any type of web preference using test header logic in TestFeatures.cpp
+    features.boolWebPreferenceFeatures.insert({ std::string { featureName }, enabled });
     return true;
 }
 
 static bool handleOptionExperimentalFeature(Options& options, const char*, const char* feature)
 {
-    return parseFeature(feature, options.experimentalFeatures);
+    return parseFeature(feature, options.features);
 }
 
 static bool handleOptionInternalFeature(Options& options, const char*, const char* feature)
 {
-    return parseFeature(feature, options.internalFeatures);
+    return parseFeature(feature, options.features);
 }
 
 static bool handleOptionUnmatched(Options& options, const char* option, const char*)

Tools/WebKitTestRunner/Options.h

@@ -27,6 +27,7 @@
 
 #pragma once
 
+#include "TestFeatures.h"
 #include <functional>
 #include <set>
 #include <stdio.h>
@@ -46,19 +47,14 @@ struct Options {
     bool gcBetweenTests { false };
     bool shouldDumpPixelsForAllTests { false };
     bool forceComplexText { false };
-    bool shouldUseAcceleratedDrawing { false };
-    bool shouldUseRemoteLayerTree { false };
-    bool shouldShowWebView { false };
-    bool shouldShowTouches { false };
     bool checkForWorldLeaks { false };
     bool allowAnyHTTPSCertificateForAllowedHosts { false };
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
     bool accessibilityIsolatedTreeMode { false };
 #endif
     std::vector<std::string> paths;
     std::set<std::string> allowedHosts;
-    std::unordered_map<std::string, bool> internalFeatures;
-    std::unordered_map<std::string, bool> experimentalFeatures;
+    TestFeatures features;
 };
 
 class Option {