Add constants and primitive type readonly attributes to Python IDL compiler

Source/bindings/scripts/code_generator_v8.py

Index: Source/bindings/scripts/code_generator_v8.py
diff --git a/Source/bindings/scripts/code_generator_v8.py b/Source/bindings/scripts/code_generator_v8.py
index 0cfaec9bdf5bab4a464e0b30ccb960d27ad1a0ff..8632206a0b5e4d3a0bc7a299882e0419e8843be5 100644
--- a/Source/bindings/scripts/code_generator_v8.py
+++ b/Source/bindings/scripts/code_generator_v8.py
@@ -35,6 +35,7 @@ Output: V8X.h and V8X.cpp
 import os
 import posixpath
 import re
+import struct
 import sys
 
 # jinja2 is in chromium's third_party directory.
@@ -44,38 +45,35 @@ sys.path.append(third_party)
 import jinja2
 
 
+ACRONYMS = ['CSS', 'HTML', 'IME', 'JS', 'SVG', 'URL', 'XML', 'XSLT']
 CALLBACK_INTERFACE_CPP_INCLUDES = set([
     'core/dom/ScriptExecutionContext.h',
     'bindings/v8/V8Binding.h',
     'bindings/v8/V8Callback.h',
     'wtf/Assertions.h',
 ])
-
-
 CALLBACK_INTERFACE_H_INCLUDES = set([
     'bindings/v8/ActiveDOMCallback.h',
     'bindings/v8/DOMWrapperWorld.h',
     'bindings/v8/ScopedPersistent.h',
 ])
-
-
 INTERFACE_CPP_INCLUDES = set([
     'RuntimeEnabledFeatures.h',
     'bindings/v8/ScriptController.h',
     'bindings/v8/V8Binding.h',
+    'bindings/v8/V8DOMWrapper.h',
+    'bindings/v8/V8DOMConfiguration.h',
     'core/dom/ContextFeatures.h',
     'core/dom/Document.h',
     'core/page/Frame.h',
     'core/platform/chromium/TraceEvent.h',
     'wtf/UnusedParam.h',
 ])
-
-
 INTERFACE_H_INCLUDES = set([
     'bindings/v8/V8Binding.h',
+    'bindings/v8/V8DOMWrapper.h',
+    'bindings/v8/WrapperTypeInfo.h',
 ])
-
-
 CPP_TYPE_SPECIAL_CONVERSION_RULES = {
     'float': 'float',
     'double': 'double',
@@ -87,8 +85,12 @@ CPP_TYPE_SPECIAL_CONVERSION_RULES = {
     'boolean': 'bool',
     'DOMString': 'const String&',
 }
-
-
+CPP_UNSIGNED_TYPES = set([
+    'octet',
+    'unsigned int',
+    'unsigned long',
+    'unsigned short',
+])
 PRIMITIVE_TYPES = set([
     'boolean',
     'void',
@@ -104,6 +106,9 @@ PRIMITIVE_TYPES = set([
     'float',
     'double',
 ])
+CPP_VALUE_TO_JS_VALUE_RETURN_DICT = {

[haraken, 2013/08/22 07:31:33]

JS_VALUE => V8_VALUE

We're mixing "js" and "v8".

[Nils Barth (inactive), 2013/08/22 09:08:13]

Got it; changed throughout.

+    'unsigned': 'v8SetReturnValueUnsigned({callback_info}, {cpp_value});',
+}
 
 
 def apply_template(path_to_template, contents):
@@ -131,6 +136,16 @@ def cpp_value_to_js_value(data_type, cpp_value, isolate, creation_context=''):
     return 'toV8(%s, %s, %s)' % (cpp_value, creation_context, isolate)
 
 
+def cpp_value_to_js_value_return(data_type, cpp_value, callback_info=''):

[haraken, 2013/08/22 07:31:33]

Why don't you simply call this cpp_value_v8_value ? Does 'return' matter?

[Nils Barth (inactive), 2013/08/22 09:08:13]

cpp_value_v8_value and cpp_value_v8_value_return are quite different; one
returns a conversion *expression* (which can be used many places), while the
other returns a setReturn... *statement* (which is used before a return;
statement).
I've changed the name to cpp_value_v8_value_set_return and distinguished the
comments.

You can see what the functions look like at the WIP:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...

The expressions are of the form
'v8String({cpp_value}, {isolate})'
...while the set return statements are of the from
'v8SetReturnValueUnsigned({callback_info}, {cpp_value});'

[haraken, 2013/08/22 11:05:28]

Makes sense. Then set_v8_return_value ?

[Nils Barth (inactive), 2013/08/23 01:50:05]

Good point; actually, v8_set_return_value seems even clearer, no?

+    """Return an expression converting a C++ value to a JS value, as a return value."""
+    this_cpp_type = cpp_type(data_type)
+    if this_cpp_type in CPP_VALUE_TO_JS_VALUE_RETURN_DICT:
+        expression_format_string = CPP_VALUE_TO_JS_VALUE_RETURN_DICT[this_cpp_type]
+    else:
+        raise Exception('unexpected data_type %s' % data_type)
+    return expression_format_string.format(callback_info=callback_info, cpp_value=cpp_value)
+
+
 def generate_conditional_string(interface_or_attribute_or_operation):
     if 'Conditional' not in interface_or_attribute_or_operation.extended_attributes:
         return ''
@@ -143,6 +158,75 @@ def generate_conditional_string(interface_or_attribute_or_operation):
     return 'ENABLE(%s)' % conditional
 
 
+def generate_constants(interface):
+    return [generate_constant(constant) for constant in interface.constants]
+
+
+def generate_constant(constant):
+    # Extended Attributes: Conditional, DeprecateAs, EnabledAtRuntime, Reflect
+    # FIXME: Conditional and DeprecateAs are only used in tests, so remove
+    name = constant.extended_attributes.get('Reflect', constant.name)

[haraken, 2013/08/22 07:31:33]

name => reflected_name

[Nils Barth (inactive), 2013/08/22 09:08:13]

Done.

+    value_raw = constant.value
+    # If the value we're dealing with is a hex literal, statically cast it to a
+    # signed integer here, rather than dynamically casting via
+    # static_cast<signed int> in the generated code.
+    # FIXME: why are we casting to signed?
+    # NodeFilter has unsigned 0xFFFFFFFF, which is converted to -1.
+    # FIXME: is this necessary? Hex literals are valid C.
+    #        (only semantic difference is casting to signed)
+    # FIXME: what about octal? decimal?
+    # FIXME: what if we have a negative literal?
+    # FIXME: BUG: Perl only checks '0x', not '0X',
+    #             but we have a test case using 0X.
+    if value_raw.startswith('0x'):
+        value = struct.unpack('i', struct.pack('I', int(value_raw, 16)))[0]
+    else:
+        value = value_raw

[haraken, 2013/08/22 07:31:33]

I don't fully understand why this is needed. Can we get rid of the complexity by
replacing "0X" with "0x" in the IDL files? Or if it's really needed, you can do
the conversion in the parser. (We don't want to mess up the code generator for
edge cases no one cares about.)

[Nils Barth (inactive), 2013/08/22 09:08:13]

I have no idea why this is needed either, hence all the FIXMEs.
However, it's what the Perl currently generates.
So it's quicker to land this (this doesn't change anything) and then see if we
can fix it.
I *think* we can just skip this; the only actual effect is listing 0xFFFFFFFF as
-1 instead of 4294967295.

In a followup, I'll see if we remove this from the Perl as well.
If it causes no problems, it cleans up the code quickly.
If it causes problems, we can fix it later.

[haraken, 2013/08/22 11:05:28]

OK, then let's fix it in a follow-up CL.

[Nils Barth (inactive), 2013/08/23 01:50:05]

Got it!

+
+    constant_parameter = {
+        'name': constant.name,
+        'name_reflect': name,  # FIXME: use name_reflect as correct 'name'

[haraken, 2013/08/22 07:31:33]

name_reflect => reflected_name

[Nils Barth (inactive), 2013/08/22 09:08:13]

Done.

+        'value': value,
+        'value_raw': value_raw,
+        # FIXME: remove conditional: only used in tests
+        'conditional_string': generate_conditional_string(constant),
+        'enabled_at_runtime': 'EnabledAtRuntime' in constant.extended_attributes,
+        'enable_function': runtime_enable_function_name(constant),
+    }
+    return constant_parameter
+
+
+def getter_expression(interface, attribute):

[haraken, 2013/08/22 07:31:33]

getter_expression => attribute_getter_name ?

You can concatenate 'imp->' in the caller side.

[Nils Barth (inactive), 2013/08/22 09:08:13]

attribute_getter is redundant; good point about name vs. expression, but we do
want a function returning the expression (see below).
The main function that generates these includes the 'imp->':
get_attribute_function_call_expression in v8_attributes.py:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...

We can probably refactor getter_expression into getter_name + getter_expression
(adding the imp-> in the expression function), but for now I'm keeping it *very*
simple.

[haraken, 2013/08/22 11:05:28]

If this function is going to be used by other getters (indexed getters, named
getters etc), getter_expression sounds nice. Otherwise, attribute_getter_name
would sound nicer.

Given that it is the caller side that declares "imp", I'd put "imp" in the
caller side. Specifically, the caller side can say
"imp->"+attribute_getter_name(interface, attribute).

[Nils Barth (inactive), 2013/08/23 01:50:05]

Looking at this more, at this point there's no need for a separate function,
since it's barely doing anything.
Once we actually have complex logic, we can see how it's sensible to factor.

On 2013/08/22 11:05:28, haraken wrote:
It's just used for regular getters.
Got it, that's good to know!
In that case the function just becomes "uncapitalize", so it's unnecessary
abstraction.

For context, the function is GetterExpression in Perl,
and get_attribute_function_call_expression in the WIP:
https://codereview.chromium.org/17572008/diff/181001/Source/bindings/scripts/...
It's a pretty long function, and there's adding/stripping of imp->,
so when we actually start landing this, we'll probably want to refactor it so
it's sensible.

+    # FIXME: very incomplete
+    return 'imp->%s()' % uncapitalize(attribute.name)
+
+
+def runtime_enable_function_name(definition_or_member):
+    """Return the name of the RuntimeEnabledFeatures function.
+
+    The returned function checks if a method/attribute is enabled.
+    If a parameter is given (e.g. 'EnabledAtRuntime=FeatureName'), return:
+        RuntimeEnabledFeatures::{featureName}Enabled
+    Otherwise return:
+        RuntimeEnabledFeatures::{methodName}Enabled
+    """
+    name = definition_or_member.extended_attributes.get('EnabledAtRuntime') or definition_or_member.name
+    return 'RuntimeEnabledFeatures::%sEnabled' % uncapitalize(name)
+
+
+def uncapitalize(name):
+    """Uncapitalize first letter or initial acronym (* with some exceptions).
+
+    E.g., 'SetURL' becomes 'setURL', but 'URLFoo' becomes 'urlFoo'.
+    Used in method names; exceptions differ from capitalize().
+    """
+    for acronym in ACRONYMS:
+        if name.startswith(acronym):
+            name.replace(acronym, acronym.lower())
+            return name
+    return name[0].lower() + name[1:]
+
+
 def includes_for_type(data_type):
     if primitive_type(data_type) or data_type == 'DOMString':
         return set()
@@ -183,7 +267,8 @@ def array_type(data_type):
 def array_or_sequence_type(data_type):
     return array_type(data_type) or sequence_type(data_type)
 
-def cpp_type(data_type, pointer_type):
+
+def cpp_type(data_type, pointer_type=None):
     """Returns the C++ type corresponding to the IDL type.
 
     Args:
@@ -194,6 +279,8 @@ def cpp_type(data_type, pointer_type):
     """
     if data_type in CPP_TYPE_SPECIAL_CONVERSION_RULES:
         return CPP_TYPE_SPECIAL_CONVERSION_RULES[data_type]
+    if data_type in CPP_UNSIGNED_TYPES:
+        return 'unsigned'
     if array_or_sequence_type(data_type):
         return 'const Vector<%s >&' % cpp_type(array_or_sequence_type(data_type), 'RefPtr')
     if pointer_type == 'raw':
@@ -288,13 +375,26 @@ class CodeGeneratorV8:
             cpp_file.write(cpp_file_text)
 
     def generate_attribute(self, attribute):
-        self.cpp_includes |= includes_for_type(attribute.data_type)
+        data_type = attribute.data_type
+        # FIXME: need to check should_keep_attribute_alive, but for now
+        # sufficient to check if primitive.
+        should_keep_attribute_alive = not primitive_type(data_type)
+        # FIXME: eliminate should_keep_attribute_alive

[haraken, 2013/08/22 07:31:33]

Remove the FIXME. We will need should_keep_attribute_alive.

[Nils Barth (inactive), 2013/08/22 09:08:13]

Got it!
(There was a FIXME asking to remove it, but I guess we need it.)

+        if should_keep_attribute_alive:
+            return_js_value_statement = None  # unused
+            self.cpp_includes |= includes_for_type(data_type)
+            self.cpp_includes.add('bindings/v8/V8HiddenPropertyName.h')
+        else:
+            cpp_value = getter_expression(self.interface, attribute)
+            return_js_value_statement = cpp_value_to_js_value_return(data_type, cpp_value, callback_info='info')
         return {
             'name': attribute.name,
             'conditional_string': generate_conditional_string(attribute),
             'cpp_method_name': cpp_method_name(attribute),
-            'cpp_type': cpp_type(attribute.data_type, pointer_type='RefPtr'),
-            'v8_type': v8_type(attribute.data_type),
+            'cpp_type': cpp_type(data_type, pointer_type='RefPtr'),
+            'should_keep_attribute_alive': should_keep_attribute_alive,
+            'return_js_value_statement': return_js_value_statement,
+            'v8_type': v8_type(data_type),
         }
 
     def generate_interface(self):
@@ -308,9 +408,10 @@ class CodeGeneratorV8:
             'v8_class_name': v8_class_name(self.interface),
             'attributes': [self.generate_attribute(attribute) for attribute in self.interface.attributes],
             # Size 0 constant array is not allowed in VC++
-            'number_of_attributes': 'WTF_ARRAY_LENGTH(%sAttributes)' % v8_class_name(self.interface) if self.interface.attributes else '0',
-            'attribute_templates': v8_class_name(self.interface) + 'Attributes' if self.interface.attributes else '0',
+            'number_of_attributes': 'WTF_ARRAY_LENGTH(%sAttrs)' % v8_class_name(self.interface) if self.interface.attributes else '0',
+            'attribute_templates': v8_class_name(self.interface) + 'Attrs' if self.interface.attributes else '0',
         }
+        template_contents['constants'] = generate_constants(self.interface)
         # Add includes afterwards, as they are modified by generate_attribute etc.
         template_contents['header_includes'] = sorted(list(self.header_includes))
         template_contents['cpp_includes'] = sorted(list(self.cpp_includes))