IDL compiler: [Default] arguments

Source/bindings/scripts/unstable/v8_methods.py

Index: Source/bindings/scripts/unstable/v8_methods.py
diff --git a/Source/bindings/scripts/unstable/v8_methods.py b/Source/bindings/scripts/unstable/v8_methods.py
index a20d28938ecd3693a623a86f8ecaa5ed530de7ad..db8bc4b659599ea132a35feb576c6e484a095c13 100644
--- a/Source/bindings/scripts/unstable/v8_methods.py
+++ b/Source/bindings/scripts/unstable/v8_methods.py
@@ -65,6 +65,7 @@ def generate_argument(method, argument, index):
         'cpp_method': cpp_method(method, index),
         'cpp_type': v8_types.cpp_type(idl_type),
         'enum_validation_expression': v8_utilities.enum_validation_expression(idl_type),
+        'has_default': 'Default' in extended_attributes,
         'idl_type': argument.idl_type,
         'index': index,
         'is_clamp': 'Clamp' in extended_attributes,
@@ -104,7 +105,9 @@ def custom_signature(arguments):
             return 'V8PerIsolateData::from(isolate)->rawTemplate(&V8{idl_type}::wrapperTypeInfo, currentWorldType)'.format(idl_type=idl_type)
         return 'v8::Handle<v8::FunctionTemplate>()'
 
-    if (any(argument.is_optional for argument in arguments) or
+    if (any(argument.is_optional and
+            'Default' not in argument.extended_attributes
+            for argument in arguments) or
         all(not v8_types.is_wrapper_type(argument.idl_type)
             for argument in arguments)):
         return None
@@ -112,11 +115,17 @@ def custom_signature(arguments):
 
 
 def v8_value_to_local_cpp_value(argument, index):
+    extended_attributes = argument.extended_attributes
     idl_type = argument.idl_type
     name = argument.name
     if argument.is_variadic:
         return 'V8TRYCATCH_VOID(Vector<{cpp_type}>, {name}, toNativeArguments<{cpp_type}>(args, {index}))'.format(
                 cpp_type=v8_types.cpp_type(idl_type), name=name, index=index)
+    if (argument.is_optional and
+        extended_attributes.get('Default') == 'NullString'):
+        v8_value = 'argumentOrNull(args, %s)' % index

[haraken, 2013/10/31 06:05:14]

We might want add an ASSERT to verify that argument.idl_type is 'DOMString'.
[Default=NullString] works only for DOMString parameters.

[Nils Barth (inactive), 2013/10/31 06:26:28]

(In this case I'll explicitly add a check for IDL type.)

I've been avoiding putting validation into the CG,
since there are a huge number of actual constraints
(if we implemented all of the MUST and MUST NOT in the spec),
it clutters the code a lot, and I'm not sure there's actual benefit.

For example, to do this properly we'd need to make it into an auxiliary function
(v8_value) and write:
if 'Default' in extended_attributes:
    if not argument.is_optional:
        raise 'Can only use [Default] on optional arguments'
    if extended_attributes['Default'] == 'NullString':
        if argument.idl_type != 'DOMString':
            raise 'Can only use [Default=NullString] on DOMString arguments'
        return 'argumentOrNull(args, %s)' % index
return 'args[%s]' % index

...which is a lot of overhead, and obscures the main point,
which is |args[%s]| vs. |argumentOrNull(args, %s)|.

We *do* validate extended attribute names and values, since these
have been a source of bugs (since silently ignored if invalid),
but other ones generally just make the code fail, right?

Longer-term, we could have a separate validation step that checks
compatibility of extended attributes with IDL types etc.,
which gives us the benefits without cluttering the CG,
and we could even run it separately (say as PRESUBMIT),
which should speed up the build some.

How does this sound?
(I'm happy to add checks if it's useful, but I want to avoid gratuitous cruft.)

+    else:
+        v8_value = 'args[%s]' % index
     return v8_types.v8_value_to_local_cpp_value(
-        idl_type, argument.extended_attributes,
-        'args[%s]' % index, name, 'args.GetIsolate()', index=index)
+        idl_type, argument.extended_attributes, v8_value, name,
+        'args.GetIsolate()', index=index)