Unify logic for properties and nonproperties in computed_style_base.py

third_party/WebKit/Source/build/scripts/make_computed_style_base.py

 #!/usr/bin/env python
 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import math
 import sys
 
 import json5_generator
 import template_expander
 import make_style_builder
 
 from name_utilities import (
-    enum_for_css_keyword, enum_value_name, class_member_name, method_name
+    enum_for_css_keyword, enum_type_name, enum_value_name, class_member_name, method_name
 )
 
 
 # Temporary hard-coded list of fields that are not CSS properties.
-# Ideally these would be specified in a .json5 file.
-NONPROPERTY_FIELDS = [
-    {'name': 'IsLink', 'field_template': 'monotonic_flag'},
+# TODO(shend): Put this into its own JSON5 file.
+NONPROPERTIES = [
+    {'name': 'IsLink', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
     # Style can not be shared.
-    {'name': 'Unique', 'field_template': 'monotonic_flag'},
+    {'name': 'Unique', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
     # Whether this style is affected by these pseudo-classes.
-    {'name': 'AffectedByFocus', 'field_template': 'monotonic_flag'},
-    {'name': 'AffectedByHover', 'field_template': 'monotonic_flag'},
-    {'name': 'AffectedByActive', 'field_template': 'monotonic_flag'},
-    {'name': 'AffectedByDrag', 'field_template': 'monotonic_flag'},
+    {'name': 'AffectedByFocus', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
+    {'name': 'AffectedByHover', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
+    {'name': 'AffectedByActive', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
+    {'name': 'AffectedByDrag', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
     # A non-inherited property references a variable or @apply is used
-    {'name': 'HasVariableReferenceFromNonInheritedProperty', 'field_template': 'monotonic_flag'},
+    {'name': 'HasVariableReferenceFromNonInheritedProperty', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
     # Explicitly inherits a non-inherited property
-    {'name': 'HasExplicitlyInheritedProperties', 'field_template': 'monotonic_flag'},
+    {'name': 'HasExplicitlyInheritedProperties', 'field_template': 'monotonic_flag',
+     'inherited': False, 'independent': False},
     # These properties only have generated storage, and their methods are handwritten in ComputedStyle.
     # TODO(shend): Remove these fields and delete the 'storage_only' template.
-    {'name': 'EmptyState', 'field_template': 'storage_only', 'size': 1}
+    {'name': 'EmptyState', 'field_template': 'storage_only', 'size': 1, 'default_value': 'false',
+     'type_name': 'bool', 'inherited': False, 'independent': False},
 ]
 
 
 class Field(object):
     """
     The generated ComputedStyle object is made up of a series of Fields.
     Each Field has a name, size, type, etc, and a bunch of attributes to
     determine which methods it will be used in.
 
     A Field also has enough information to use any storage type in C++, such as
@@ skipping 25 matching lines @@
         self.name = class_member_name(name_for_methods)
         self.property_name = property_name
         self.type_name = type_name
         self.field_template = field_template
         self.size = size
         self.default_value = default_value
 
         # Field role: one of these must be true
         self.is_property = field_role == 'property'
         self.is_inherited_flag = field_role == 'inherited_flag'
         self.is_nonproperty = field_role == 'nonproperty'

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

We should aim to not store the field_role anywhere and instead add Field members
that generalise over all roles so that templates are unaware of the Field role.
Let's add a TODO for this here.

         assert (self.is_property, self.is_inherited_flag, self.is_nonproperty).count(True) == 1, \
             'Field role has to be exactly one of: property, inherited_flag, nonproperty'
 
-        if self.is_property:
+        if self.is_property or self.is_nonproperty:

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

Use "if not self.is_inherited_flag" as this is about not putting inherited flags
on inherited flags.

[shend, 2017/03/22 22:29:18]

Done.

             self.is_inherited = kwargs.pop('inherited')
             self.is_independent = kwargs.pop('independent')
             assert self.is_inherited or not self.is_independent, 'Only inherited fields can be independent'
 
             self.is_inherited_method_name = method_name(name_for_methods + 'IsInherited')
 
         # Method names
         getter_prefix = 'Get' if name_for_methods == self.type_name else ''
         self.getter_method_name = method_name(getter_prefix + name_for_methods)
         self.setter_method_name = method_name('Set' + name_for_methods)
@@ skipping 30 matching lines @@
                 assert set(enums[enum_name]) == set(enum_values), \
                     ("'" + property_['name'] + "' can't have type_name '" + enum_name + "' "
                      "because it was used by a previous property, but with a different set of keywords. "
                      "Either give it a different name or ensure the keywords are the same.")
 
             enums[enum_name] = enum_values
 
     return enums
 
 
-def _create_property_field(property_):
+def _create_field(field_role, property_):
     """
-    Create a property field from a CSS property and return the Field object.
+    Create a property or nonproperty field.
     """
-    bits_needed = math.log(len(property_['keywords']), 2)  # TODO: implement for non-enums
-    type_name = property_['type_name']
+    assert field_role in ('property', 'nonproperty')
 
-    assert property_['initial_keyword'] is not None, \
-        ('MakeComputedStyleBase requires an initial keyword for keyword fields, none specified '
-         'for property ' + property_['name'])
-    default_value = type_name + '::' + enum_value_name(property_['initial_keyword'])
+    name_for_methods = property_['name_for_methods']
+
+    if property_['field_template'] == 'keyword':
+        assert property_['initial_keyword'] is not None, \
+            ('MakeComputedStyleBase requires an initial keyword for keyword fields, none specified '
+             'for property ' + property_['name'])
+        type_name = property_['type_name']
+        default_value = type_name + '::' + enum_value_name(property_['initial_keyword'])
+        size = int(math.ceil(math.log(len(property_['keywords']), 2)))
+    elif property_['field_template'] == 'storage_only':
+        # 'storage_only' fields need to specify a size, type_name and default_value
+        type_name = property_['type_name']
+        default_value = property_['default_value']
+        size = property_['size']
+    else:
+        # Must be a boolean flag

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

This comment should be an assert instead.

[shend, 2017/03/22 22:29:18]

Done.

+        type_name = 'bool'
+        default_value = 'false'
+        size = 1
 
     return Field(
-        'property',
-        property_['name_for_methods'],
+        field_role,
+        name_for_methods,
         property_name=property_['name'],
         inherited=property_['inherited'],
         independent=property_['independent'],
         type_name=type_name,
         field_template=property_['field_template'],
-        size=int(math.ceil(bits_needed)),
+        size=size,
         default_value=default_value,
     )
 
 
 def _create_inherited_flag_field(property_):
     """
     Create the field used for an inheritance fast path from an independent CSS property,
     and return the Field object.
     """
     return Field(
         'inherited_flag',
         property_['name_for_methods'] + 'IsInherited',
         property_name=property_['name'],
         type_name='bool',
         field_template='flag',
         size=1,
         default_value='true',
     )
 
 
-def _create_nonproperty_field(property_):
+def _create_fields(field_role, properties):
     """
-    Create a nonproperty field from an entry in NONPROPERTY_FIELDS and return the Field object.
-    """
-    # TODO(shend): Make this work for nonflags
-    assert property_['field_template'] in ('flag', 'monotonic_flag', 'storage_only'), \
-        "Nonproperties with arbitrary templates are not yet supported"
-
-    if property_['field_template'] == 'storage_only':
-        assert 'size' in property_, 'storage_only fields need to specify a size'
-        size = property_['size']
-    else:
-        # Otherwise the field must be some type of flag.
-        size = 1
-
-    return Field(
-        'nonproperty',
-        property_['name_for_methods'],
-        property_name=property_['name'],
-        type_name='bool',
-        field_template=property_['field_template'],
-        size=size,
-        default_value='false',
-    )
-
-
-def _create_fields(properties):
-    """
-    Create ComputedStyle fields from CSS properties and return a list of Field objects.
+    Create ComputedStyle fields from properties or nonproperties and return a list of Field objects.

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

Instead of describing the input (which is a detail of the call site) just
describe that the output is a list of Fields including is_inherited flag fields.

As a side note the name nonproperties is confusing in that it doesn't include
the is_inherited flags which are "nonproperties" themselves.

[shend, 2017/03/22 22:29:18]

Yes I don't like the name nonproperties. Do you have a suggestion for an
alternate name?

[shend, 2017/03/23 00:05:29]

Actually, the term nonproperty was already in the codebase:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com...

[alancutter (OOO until 2018), 2017/03/23 02:13:41]

The name nonproperties itself is okay it's just strange that in this context it
excludes inherited flags.

[shend, 2017/03/23 02:54:50]

Hmm it might be possible to unify the code paths for nonproperties and
inheritance flags as well, so that an inheritance flag is just one type of
nonproperty. I'll look into it.

     """
     fields = []
     for property_ in properties:
         # Only generate properties that have a field template
         if property_['field_template'] is not None:
             # If the property is independent, add the single-bit sized isInherited flag
             # to the list of Fields as well.
             if property_['independent']:
                 fields.append(_create_inherited_flag_field(property_))
 
-            fields.append(_create_property_field(property_))
-
-    # TODO(shend): Merge NONPROPERTY_FIELDS with property_values so that properties and
-    # nonproperties can be treated uniformly.
-    for property_ in NONPROPERTY_FIELDS:
-        property_['name_for_methods'] = property_['name']
-        fields.append(_create_nonproperty_field(property_))
+            fields.append(_create_field(field_role, property_))
 
     return fields
 
 
 def _pack_fields(fields):
     """
     Group a list of fields into buckets to minimise padding.
     Returns a list of buckets, where each bucket is a list of Field objects.
     """
     # Since fields cannot cross word boundaries, in order to minimize
@@ skipping 27 matching lines @@
 class ComputedStyleBaseWriter(make_style_builder.StyleBuilderWriter):
     def __init__(self, json5_file_path):
         super(ComputedStyleBaseWriter, self).__init__(json5_file_path)
         self._outputs = {
             'ComputedStyleBase.h': self.generate_base_computed_style_h,
             'ComputedStyleBase.cpp': self.generate_base_computed_style_cpp,
             'ComputedStyleBaseConstants.h': self.generate_base_computed_style_constants,
             'CSSValueIDMappingsGenerated.h': self.generate_css_value_mappings,
         }
 
+        # TODO(shend): Remove this once we move NONPROPERTIES to its own JSON file,
+        # since the JSON5 reader will handle missing fields and defaults.
+        for property_ in NONPROPERTIES:
+            property_['field_type_path'] = property_.get('field_type_path')

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

I'd rather see it crash if it's missing than be defensive here. When the
"nonproperties" are put in a JSON5 file this can be handled by the JSON5 reader
automatically.

[shend, 2017/03/22 22:29:18]

Hmm, so I'm trying to figure out what you mean here. So you're suggesting that,
if an entry in the NONPROPERTIES dict has no field_type_path, it should
explicitly specify "'field_type_path': None" at the top of the file instead of
omitting it?

[alancutter (OOO until 2018), 2017/03/23 02:13:41]

Ah, sorry it does make sense to have this then.
I'd prefer that it used if instead of get to default the value though.

+            property_['name_for_methods'] = property_['name']
+            property_['type_name'] = property_.get('type_name', 'E' + enum_type_name(property_['name_for_methods']))

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

Let's use "if attr not in object: object[attr] = default" instead.

[shend, 2017/03/22 22:29:18]

Same here, would we write "if object[attr] is None: object[attr] = default"
instead?

[alancutter (OOO until 2018), 2017/03/23 02:13:41]

Doesn't this default only apply if the attr isn't in the object?

[shend, 2017/03/23 02:54:50]

Yeah don't worry, your previous comment clarified everything.

+
         property_values = self._properties.values()
 
         # Override the type name when field_type_path is specified
         for property_ in property_values:
             if property_['field_type_path']:
                 property_['type_name'] = property_['field_type_path'].split('/')[-1]
 
-        # Create all the enums used by properties
-        self._generated_enums = _create_enums(self._properties.values())
+        # Create all the enums used by properties and nonproperties
+        self._generated_enums = _create_enums(property_values + NONPROPERTIES)
 
         # Create all the fields
-        all_fields = _create_fields(self._properties.values())
+        all_fields = (_create_fields('property', property_values) +
+                      _create_fields('nonproperty', NONPROPERTIES))

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

We don't really need these "create X" comments, the code is self explanatory
enough.

[shend, 2017/03/22 22:29:18]

Done.

 
         # Group fields into buckets
         field_buckets = _pack_fields(all_fields)
 
         # The expected size of ComputedStyleBase is equivalent to as many words
         # as the total number of buckets.
         self._expected_total_field_bytes = len(field_buckets)
 
         # The most optimal size of ComputedStyleBase is the total sum of all the
         # field sizes, rounded up to the nearest word. If this produces the
         # incorrect value, either the packing algorithm is not optimal or there
         # is no way to pack the fields such that excess padding space is not
         # added.
-        # If this fails, increase extra_padding_bytes by 1, but be aware that
+        # If this fails, increase padding_bytes by 1, but be aware that
         # this also increases ComputedStyleBase by 1 word.
-        # We should be able to bring extra_padding_bytes back to 0 from time to
+        # We should be able to bring padding_bytes back to 0 from time to
         # time.
-        extra_padding_bytes = 0
+        padding_bytes = 0
         optimal_total_field_bytes = int(math.ceil(sum(f.size for f in all_fields) / 32.0))
-        real_total_field_bytes = optimal_total_field_bytes + extra_padding_bytes
+        real_total_field_bytes = optimal_total_field_bytes + padding_bytes
         assert self._expected_total_field_bytes == real_total_field_bytes, \
             ('The field packing algorithm produced %s bytes, optimal is %s bytes' %
              (self._expected_total_field_bytes, real_total_field_bytes))
 
         # Order the fields so fields in each bucket are adjacent.
         self._fields = []
         for bucket in field_buckets:
             for field in bucket:
                 self._fields.append(field)
 
-        self._include_paths = _get_include_paths(self._properties.values())
+        self._property_values = property_values

[alancutter (OOO until 2018), 2017/03/22 07:01:44]

No need to upgrade the local variable as a member, leave as is.

[shend, 2017/03/22 22:29:18]

self._property_values is actually used in a separate method
(generate_css_value_mappings, see below).

[alancutter (OOO until 2018), 2017/03/23 02:13:41]

That method can continue to use self._properties.values(). It's redundant to
have _property_values as a member when you already have access to that.

[shend, 2017/03/23 02:54:49]

Yep okay, done.

+        self._include_paths = _get_include_paths(property_values + NONPROPERTIES)
 
 
     @template_expander.use_jinja('ComputedStyleBase.h.tmpl')
     def generate_base_computed_style_h(self):
         return {
             'properties': self._properties,
             'enums': self._generated_enums,
             'include_paths': self._include_paths,
             'fields': self._fields,
             'expected_total_field_bytes': self._expected_total_field_bytes,
@@ skipping 14 matching lines @@
             'properties': self._properties,
             'enums': self._generated_enums,
             'fields': self._fields,
             'expected_total_field_bytes': self._expected_total_field_bytes,
         }
 
     @template_expander.use_jinja('CSSValueIDMappingsGenerated.h.tmpl')
     def generate_css_value_mappings(self):
         mappings = {}
 
-        for property_ in self._properties.values():
+        for property_ in self._property_values:
             if property_['field_template'] == 'keyword':
                 mappings[property_['type_name']] = {
                     'default_value': enum_value_name(property_['initial_keyword']),
                     'mapping': [(enum_value_name(k), enum_for_css_keyword(k)) for k in property_['keywords']],
                 }
 
         return {
             'include_paths': self._include_paths,
             'mappings': mappings,
         }
 
 if __name__ == '__main__':
     json5_generator.Maker(ComputedStyleBaseWriter).main()