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

This reverts commit a6198a9e.
Sort keys of python dictionary so that deterministic build won't fail.
Original change's description:
> 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
> >
> # 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/+/2013472

Bug: 993790
Change-Id: I4100bfed3c14da388d625df39f93d428497f1671
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2016339
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735436}

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

@@ -37,11 +37,13 @@ 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.*']},
-        # {'file_name': 'make_origin_trials_unittest.py', 'affected_list': [r'.*make_origin_trials.*', r'.*make_runtime_features.*', r'.*\btests[\\\/]runtime_enabled_features.*']},
-    ]
+    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.*']
+    }]
     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,9 +2,10 @@
 # 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):
@@ -26,78 +27,18 @@ class FeaturePolicyFeatureWriter(json5_generator.Writer):
             else:
                 self._runtime_features.append(feature)
 
-        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'])
+        origin_trials_set = origin_trials(self._runtime_features)
 
-    # 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 = {}
+        self._origin_trial_dependency_map = defaultdict(list)
+        self._runtime_to_feature_policy_map = defaultdict(list)
         for feature in self._feature_policy_features:
-            # 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]
+            for dependency in feature['depends_on']:
+                if str(dependency) in origin_trials_set:
+                    self._origin_trial_dependency_map[feature['name']].append(dependency)
                 else:
-                    runtime_to_feature_policy_map[dependant_name].append(feature)
-        return runtime_to_feature_policy_map
+                    self._runtime_to_feature_policy_map[dependency].append(feature['name'])
 
-    # 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
+        self._header_guard = self.make_header_guard(self._relative_output_dir + self.file_basename + '.h')
 
     def _template_inputs(self):
         return {

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

@@ -53,10 +53,11 @@ class BaseRuntimeFeatureWriter(json5_generator.Writer):
         assert self.file_basename
 
         self._features = self.json5_file.name_dictionaries
-        # Dependency graph specified by 'depends_on' attribute.
-        self._dependency_graph = util.init_graph(self._features)
+        origin_trial_set = util.origin_trials(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.
@@ -65,8 +66,6 @@ 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.
@@ -75,9 +74,6 @@ 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,47 +3,68 @@
 # found in the LICENSE file.
 
 
-def init_graph(features):
-    graph = {}
-    for feature in features:
-        graph[str(feature['name'])] = []
-    return graph
+from collections import defaultdict
 
 
-def check_if_dependency_graph_contains_cycle(graph):
-    state = {}
-    visited, done = 0, 1
+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 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 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.
+
+    @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()
+
+    graph = defaultdict(list)
+    for feature in features:
+        for dependency in feature['depends_on']:
+            graph[dependency].append(str(feature['name']))
 
     def dfs(node):
-        in_origin_trial.add(node)
-        for neighbor in graph[node]:
-            if neighbor not in in_origin_trial:
-                dfs(neighbor)
+        origin_trials_set.add(node)
+        for dependent in graph[node]:
+            if dependent not in origin_trials_set:
+                dfs(dependent)
 
     for feature in features:
         if feature['origin_trial_feature_name']:
             dfs(str(feature['name']))
-    # 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
+
+    return origin_trials_set

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

@@ -18,8 +18,12 @@ class MakeRuntimeFeaturesUtilitiesTest(unittest.TestCase):
             'd': ['e'],
             'e': ['c']
         }
-        with self.assertRaises(Exception):
-            util.check_if_dependency_graph_contains_cycle(graph)
+        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'}])
 
     def test_in_origin_trials_flag(self):
         features = [
@@ -29,38 +33,35 @@ 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},
         ]
-        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'])
+        self.assertSetEqual(util.origin_trials(features), {'b', 'c', 'd', 'e'})
 
-    def test_init_graph(self):
-        features = [
-            {'name': NameStyleConverter('a')},
-            {'name': NameStyleConverter('b')},
-            {'name': NameStyleConverter('c')},
-        ]
+        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
+        }]
 
-        graph = util.init_graph(features)
-        self.assertEqual(len(features), len(graph))
-        for node in graph:
-            self.assertEqual(len(graph[node]), 0)
+        self.assertSetEqual(util.origin_trials(features), {'b', 'c', 'y', 'z'})
 
 
 if __name__ == "__main__":

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

@@ -32,16 +32,16 @@ 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.in_origin_trial %}
+    {% if not feature.depends_on or feature.name in origin_trial_dependency_map %}
     default_feature_name_map.Set(k{{feature.name}}PolicyName,
                                  mojom::FeaturePolicyFeature::k{{feature.name}});
     {% endif %}
     {% endfor %}
-    {% for runtime_feature_name, FP_features in runtime_to_feature_policy_map.items() %}
+    {% for runtime_feature_name, FP_features in runtime_to_feature_policy_map.items() | sort %}
     if (RuntimeEnabledFeatures::{{runtime_feature_name}}Enabled()) {
       {% for feature in FP_features %}
-      default_feature_name_map.Set(k{{feature.name}}PolicyName,
-                                   mojom::FeaturePolicyFeature::k{{feature.name}});
+      default_feature_name_map.Set(k{{feature}}PolicyName,
+                                   mojom::FeaturePolicyFeature::k{{feature}});
       {% endfor %}
     }
     {% endfor %}
@@ -49,14 +49,16 @@ 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() %}
+  {% for feature_name, dependencies in origin_trial_dependency_map.items() | sort %}
   if (feature_name == k{{feature_name}}PolicyName) {
     return
     {%- for dependency in dependencies %}
     {%- if not loop.first %} &&{% endif %}
- !RuntimeEnabledFeatures::{{dependency.name}}Enabled(feature_context)
+ !RuntimeEnabledFeatures::{{dependency}}Enabled(feature_context)
     {%- endfor %};
   }
   {% endfor %}