Revert "Refactoring origin trial dependency generation in feature_policy_helper.cc"

This reverts commit 0c31aad8.

Reason for revert: Somewhat speculative; looks like it made
the build nondeterministic (https://crbug.com/1038775)

Original change's description:
> Refactoring origin trial dependency generation in feature_policy_helper.cc
> 
> This CL refactors build script code that generates feature_policy_helper.cc,
> so that the logic could be reused for document policy.
> 
> - Simplifies make_runtime_features_utilities.py API.
> - Makes most util functions pure.
> - Adds more unittests
> 
> The output of make_feature_policy_helper is verified to be the same as before.
> 
> Bug: 993790
> Change-Id: I0f0e07888f5b9e02727e3f210bbfb4cd871ecc15
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003255
> Reviewed-by: Ian Clelland <iclelland@chromium.org>
> Commit-Queue: Charlie Hu <chenleihu@google.com>
> Cr-Commit-Position: refs/heads/master@{#733063}

TBR=iclelland@chromium.org,chenleihu@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 993790
Change-Id: If7aaa4c6b851f74db6b418b480cf8482b15cf7b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013472Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734014}

third_party/blink/renderer/build/scripts/PRESUBMIT.py

@@ -37,13 +37,11 @@ def _GenerateTestCommand(input_api, output_api, file_name, affected_list):
 
 
 def _RunTests(input_api, output_api):
-    tests = [{
-        'file_name': 'json5_generator_unittest.py',
-        'affected_list': [r'.*json5_generator.*', r'.*\btests[\\\/].*']
-    }, {
-        'file_name': 'make_runtime_features_utilities_unittest.py',
-        'affected_list': [r'.*make_runtime_features_utilities.*']
-    }]
+    tests = [
+        {'file_name': 'json5_generator_unittest.py', 'affected_list': [r'.*json5_generator.*', r'.*\btests[\\\/].*']},
+        {'file_name': 'make_runtime_features_utilities_unittest.py', 'affected_list': [r'.*make_runtime_features_utilities.*']},
+        # {'file_name': 'make_origin_trials_unittest.py', 'affected_list': [r'.*make_origin_trials.*', r'.*make_runtime_features.*', r'.*\btests[\\\/]runtime_enabled_features.*']},
+    ]
     test_commands = []
     for test in tests:
         test_commands.append(_GenerateTestCommand(input_api, output_api, test['file_name'], test['affected_list']))

third_party/blink/renderer/build/scripts/make_feature_policy_helper.py

@@ -2,10 +2,9 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import make_runtime_features_utilities as util
 import json5_generator
 import template_expander
-from collections import defaultdict
-from make_runtime_features_utilities import origin_trials
 
 
 class FeaturePolicyFeatureWriter(json5_generator.Writer):
@@ -27,18 +26,78 @@ class FeaturePolicyFeatureWriter(json5_generator.Writer):
             else:
                 self._runtime_features.append(feature)
 
-        origin_trials_set = origin_trials(self._runtime_features)
+        self._convert_runtime_name_to_feature()
+        self._set_in_origin_trial()
+        self._runtime_to_feature_policy_map = self._make_runtime_to_feature_policy_map()
+        self._origin_trial_dependency_map = self._make_origin_trial_dependency_map()
+        self._header_guard = self.make_header_guard(self._relative_output_dir + self.file_basename + '.h')
+
+    # Replaces runtime names in 'depends_on' list to feature objects.
+    def _convert_runtime_name_to_feature(self):
+        name_to_runtime_feature_map = {}
+        for feature in self._runtime_features:
+            name_to_runtime_feature_map[str(feature['name'])] = feature
+
+        def replace_name_with_object(name):
+            assert name in name_to_runtime_feature_map, name + ' is not a runtime feature.'
+            return name_to_runtime_feature_map[name]
+
+        for feature in self._feature_policy_features:
+            feature['depends_on'] = map(replace_name_with_object, feature['depends_on'])
 
-        self._origin_trial_dependency_map = defaultdict(list)
-        self._runtime_to_feature_policy_map = defaultdict(list)
+    # Returns a map of runtime features (not in origin trial) to the feature
+    # policy features that depend on them. The maps is used to generate
+    # DefaultFeatureNameMap().
+    def _make_runtime_to_feature_policy_map(self):
+        runtime_to_feature_policy_map = {}
         for feature in self._feature_policy_features:
-            for dependency in feature['depends_on']:
-                if str(dependency) in origin_trials_set:
-                    self._origin_trial_dependency_map[feature['name']].append(dependency)
+            # Filter out all the ones not in OT.
+            for dependant in [runtime_feature for runtime_feature
+                              in feature['depends_on']
+                              if not runtime_feature['in_origin_trial']]:
+                dependant_name = str(dependant['name'])
+                if dependant_name not in runtime_to_feature_policy_map:
+                    runtime_to_feature_policy_map[dependant_name] = [feature]
                 else:
-                    self._runtime_to_feature_policy_map[dependency].append(feature['name'])
+                    runtime_to_feature_policy_map[dependant_name].append(feature)
+        return runtime_to_feature_policy_map
 
-        self._header_guard = self.make_header_guard(self._relative_output_dir + self.file_basename + '.h')
+    # Returns a map of feature policy features that depend on OT features to
+    # their dependencies. The map is used to generate DisabledByOriginTrial().
+    def _make_origin_trial_dependency_map(self):
+        origin_trial_dependency_map = {}
+        for feature in [fp_feature for fp_feature
+                        in self._feature_policy_features
+                        if fp_feature['in_origin_trial']]:
+            feature_name = str(feature['name'])
+            # Only go through the dependencies that are in an origin trial.
+            for dependant in [runtime_feature for runtime_feature
+                              in feature['depends_on']
+                              if runtime_feature['in_origin_trial']]:
+                if feature_name in origin_trial_dependency_map:
+                    origin_trial_dependency_map[feature_name].append(dependant)
+                else:
+                    origin_trial_dependency_map[feature_name] = [dependant]
+        return origin_trial_dependency_map
+
+    # Sets the 'in_origin_trial' flag for features. A feature is in an origin
+    # trial if either itself or one of its dependencies are in an origin trial.
+    def _set_in_origin_trial(self):
+        # |_dependency_graph| is used to keep the 'depends_on' relationship
+        # for the runtime features defined in "runtime_enabled_features.json5".
+        self._dependency_graph = util.init_graph(self._runtime_features)
+        for feature in self._runtime_features:
+            for dependant_name in feature['depends_on']:
+                assert dependant_name in self._dependency_graph, dependant_name + ' is not a feature.'
+                self._dependency_graph[dependant_name].append(str(feature['name']))
+        util.check_if_dependency_graph_contains_cycle(self._dependency_graph)
+        util.set_origin_trials_features(self._runtime_features, self._dependency_graph)
+        # Set the flag for feature policy features as well.
+        for feature in self._feature_policy_features:
+            feature['in_origin_trial'] = False
+            if any([runtime_feature['in_origin_trial'] for
+                    runtime_feature in feature['depends_on']]):
+                feature['in_origin_trial'] = True
 
     def _template_inputs(self):
         return {

third_party/blink/renderer/build/scripts/make_runtime_features.py

@@ -53,11 +53,10 @@ class BaseRuntimeFeatureWriter(json5_generator.Writer):
         assert self.file_basename
 
         self._features = self.json5_file.name_dictionaries
-        origin_trial_set = util.origin_trials(self._features)
-
+        # Dependency graph specified by 'depends_on' attribute.
+        self._dependency_graph = util.init_graph(self._features)
         # Make sure the resulting dictionaries have all the keys we expect.
         for feature in self._features:
-            feature['in_origin_trial'] = str(feature['name']) in origin_trial_set
             feature['data_member_name'] = self._data_member_name(feature['name'])
             # Most features just check their is_foo_enabled_ bool
             # but some depend on or are implied by other bools.
@@ -66,6 +65,8 @@ class BaseRuntimeFeatureWriter(json5_generator.Writer):
             for implied_by_name in feature['implied_by']:
                 enabled_condition += ' || ' + self._data_member_name(implied_by_name)
             for dependant_name in feature['depends_on']:
+                assert dependant_name in self._dependency_graph, dependant_name + ' is not a feature.'
+                self._dependency_graph[dependant_name].append(str(feature['name']))
                 enabled_condition += ' && ' + self._data_member_name(dependant_name)
             feature['enabled_condition'] = enabled_condition
             # If 'status' is a dict, add the values for all the not-mentioned platforms too.
@@ -74,6 +75,9 @@ class BaseRuntimeFeatureWriter(json5_generator.Writer):
             # Specify the type of status
             feature['status_type'] = "dict" if isinstance(feature['status'], dict) else "str"
 
+        util.check_if_dependency_graph_contains_cycle(self._dependency_graph)
+        util.set_origin_trials_features(self._features, self._dependency_graph)
+
         self._standard_features = [feature for feature in self._features if not feature['custom']]
         self._origin_trial_features = [feature for feature in self._features if feature['in_origin_trial']]
         self._header_guard = self.make_header_guard(self._relative_output_dir + self.file_basename + '.h')

third_party/blink/renderer/build/scripts/make_runtime_features_utilities.py

@@ -3,68 +3,47 @@
 # found in the LICENSE file.
 
 
-from collections import defaultdict
-
-
-def _runtime_features_graph_sanity_check(features):
-    """
-    Raises AssertionError when sanity check failed.
-    @param features: a List[Dict]. Each Dict must have key 'depends_on' and 'name'
-    @returns None
-    """
-    feature_pool = {str(f['name']) for f in features}
-    for f in features:
-        for d in f['depends_on']:
-            assert d in feature_pool, "{} not found in runtime_enabled_features.json5".format(d)
-
-    def cyclic(features):
-        """
-        Returns True if the runtime features graph contains a cycle
-        @returns bool
-        """
-        graph = {str(feature['name']): feature['depends_on'] for feature in features}
-        path = set()
-
-        def visit(vertex):
-            path.add(vertex)
-            for neighbor in graph[vertex]:
-                if neighbor in path or visit(neighbor):
-                    return True
-            path.remove(vertex)
-            return False
-
-        return any(visit(str(f['name'])) for f in features)
-
-    assert not cyclic(features), "Cycle found in dependency graph"
-
-
-def origin_trials(features):
-    """
-    This function returns all features that are in origin trial.
-    The dependency is considered in origin trial if itself is in origin trial
-    or any of its dependencies are in origin trial. Propagate dependency
-    tag use DFS can find all features that are in origin trial.
+def init_graph(features):
+    graph = {}
+    for feature in features:
+        graph[str(feature['name'])] = []
+    return graph
 
-    @param features: a List[Dict]. Each Dict much has key named 'depends_on'
-    @returns Set[str(runtime feature name)]
-    """
-    _runtime_features_graph_sanity_check(features)
 
-    origin_trials_set = set()
+def check_if_dependency_graph_contains_cycle(graph):
+    state = {}
+    visited, done = 0, 1
 
-    graph = defaultdict(list)
-    for feature in features:
-        for dependency in feature['depends_on']:
-            graph[dependency].append(str(feature['name']))
+    def dfs(node):
+        state[node] = visited
+        for neighbor in graph.get(node, []):
+            neighbor_state = state.get(neighbor)
+            if neighbor_state == visited:
+                raise Exception('Detected cycle in feature dependencies.')
+            if neighbor_state == done:
+                continue
+            dfs(neighbor)
+        state[node] = done
+
+    for feature in graph:
+        if feature not in state:
+            dfs(feature)
+
+
+# Marks a feature to be in origin trials if
+# one of its dependencies is in origin trials.
+def set_origin_trials_features(features, graph):
+    in_origin_trial = set()
 
     def dfs(node):
-        origin_trials_set.add(node)
-        for dependent in graph[node]:
-            if dependent not in origin_trials_set:
-                dfs(dependent)
+        in_origin_trial.add(node)
+        for neighbor in graph[node]:
+            if neighbor not in in_origin_trial:
+                dfs(neighbor)
 
     for feature in features:
         if feature['origin_trial_feature_name']:
             dfs(str(feature['name']))
-
-    return origin_trials_set
+    # Set 'in_origin_trial' for each feature.
+    for feature in features:
+        feature['in_origin_trial'] = True if str(feature['name']) in in_origin_trial else False

third_party/blink/renderer/build/scripts/make_runtime_features_utilities_unittest.py

@@ -18,12 +18,8 @@ class MakeRuntimeFeaturesUtilitiesTest(unittest.TestCase):
             'd': ['e'],
             'e': ['c']
         }
-        with self.assertRaises(AssertionError):
-            util.origin_trials([{'name': name, 'depends_on': deps} for name, deps in graph.items()])
-
-    def test_bad_dependency(self):
-        with self.assertRaises(AssertionError):
-            util.origin_trials([{'name': 'a', 'depends_on': 'x'}])
+        with self.assertRaises(Exception):
+            util.check_if_dependency_graph_contains_cycle(graph)
 
     def test_in_origin_trials_flag(self):
         features = [
@@ -33,35 +29,38 @@ class MakeRuntimeFeaturesUtilitiesTest(unittest.TestCase):
             {'name': NameStyleConverter('d'), 'depends_on': ['b'], 'origin_trial_feature_name': None},
             {'name': NameStyleConverter('e'), 'depends_on': ['d'], 'origin_trial_feature_name': None},
         ]
-        self.assertSetEqual(util.origin_trials(features), {'b', 'c', 'd', 'e'})
+        graph = {
+            'a': ['b'],
+            'b': ['c', 'd'],
+            'c': [],
+            'd': ['e'],
+            'e': []
+        }
+        results = [
+            {'name': NameStyleConverter('a'), 'in_origin_trial': False},
+            {'name': NameStyleConverter('b'), 'depends_on': ['a'],
+             'origin_trial_feature_name': 'OriginTrials', 'in_origin_trial': True},
+            {'name': NameStyleConverter('c'), 'depends_on': ['b'], 'in_origin_trial': True},
+            {'name': NameStyleConverter('d'), 'depends_on': ['b'], 'in_origin_trial': True},
+            {'name': NameStyleConverter('e'), 'depends_on': ['d'], 'in_origin_trial': True},
+        ]
+
+        util.set_origin_trials_features(features, graph)
+        self.assertEqual(len(features), len(results))
+        for feature, result in zip(features, results):
+            self.assertEqual(result['in_origin_trial'], feature['in_origin_trial'])
 
-        features = [{
-            'name': 'a',
-            'depends_on': ['x'],
-            'origin_trial_feature_name': None
-        }, {
-            'name': 'b',
-            'depends_on': ['x', 'y'],
-            'origin_trial_feature_name': None
-        }, {
-            'name': 'c',
-            'depends_on': ['y', 'z'],
-            'origin_trial_feature_name': None
-        }, {
-            'name': 'x',
-            'depends_on': [],
-            'origin_trial_feature_name': None
-        }, {
-            'name': 'y',
-            'depends_on': ['x'],
-            'origin_trial_feature_name': 'y'
-        }, {
-            'name': 'z',
-            'depends_on': ['y'],
-            'origin_trial_feature_name': None
-        }]
+    def test_init_graph(self):
+        features = [
+            {'name': NameStyleConverter('a')},
+            {'name': NameStyleConverter('b')},
+            {'name': NameStyleConverter('c')},
+        ]
 
-        self.assertSetEqual(util.origin_trials(features), {'b', 'c', 'y', 'z'})
+        graph = util.init_graph(features)
+        self.assertEqual(len(features), len(graph))
+        for node in graph:
+            self.assertEqual(len(graph[node]), 0)
 
 
 if __name__ == "__main__":

third_party/blink/renderer/build/scripts/templates/feature_policy_helper.cc.tmpl

@@ -32,7 +32,7 @@ const FeatureNameMap& GetDefaultFeatureNameMap() {
   DEFINE_STATIC_LOCAL(FeatureNameMap, default_feature_name_map, ());
   if (default_feature_name_map.IsEmpty()) {
     {% for feature in feature_policy_features %}
-    {% if not feature.depends_on or feature.name in origin_trial_dependency_map %}
+    {% if not feature.depends_on or feature.in_origin_trial %}
     default_feature_name_map.Set(k{{feature.name}}PolicyName,
                                  mojom::FeaturePolicyFeature::k{{feature.name}});
     {% endif %}
@@ -40,8 +40,8 @@ const FeatureNameMap& GetDefaultFeatureNameMap() {
     {% for runtime_feature_name, FP_features in runtime_to_feature_policy_map.items() %}
     if (RuntimeEnabledFeatures::{{runtime_feature_name}}Enabled()) {
       {% for feature in FP_features %}
-      default_feature_name_map.Set(k{{feature}}PolicyName,
-                                   mojom::FeaturePolicyFeature::k{{feature}});
+      default_feature_name_map.Set(k{{feature.name}}PolicyName,
+                                   mojom::FeaturePolicyFeature::k{{feature.name}});
       {% endfor %}
     }
     {% endfor %}
@@ -49,8 +49,6 @@ const FeatureNameMap& GetDefaultFeatureNameMap() {
   return default_feature_name_map;
 }
 
-// If any of the origin trial runtime feature is enabled, returns false,
-// i.e. the feature is considered enabled by origin trial.
 bool DisabledByOriginTrial(const String& feature_name,
                            FeatureContext* feature_context) {
   {% for feature_name, dependencies in origin_trial_dependency_map.items() %}
@@ -58,7 +56,7 @@ bool DisabledByOriginTrial(const String& feature_name,
     return
     {%- for dependency in dependencies %}
     {%- if not loop.first %} &&{% endif %}
- !RuntimeEnabledFeatures::{{dependency}}Enabled(feature_context)
+ !RuntimeEnabledFeatures::{{dependency.name}}Enabled(feature_context)
     {%- endfor %};
   }
   {% endfor %}