Generate binding code for VoidCallback.idl with code generator in python

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 d2f933044a5a171b5f1109ef2dc94fa95f660842..1dd1a8d598e696fd19458fd482b1e27130fe7b85 100644
--- a/Source/bindings/scripts/code_generator_v8.py
+++ b/Source/bindings/scripts/code_generator_v8.py
@@ -30,12 +30,127 @@
 
 Input: An object of class IdlDefinitions, containing an IDL interface X
 Output: V8X.h and V8X.cpp
-
-FIXME: Currently a stub, as part of landing the parser and code generator
-incrementally. Only implements generation of dummy .cpp and .h files.
 """
 
-import os.path
+import os
+import posixpath
+import re
+import sys
+
+import idl_definitions
+
+# jinja2 is in chromium's third_party directory.
+module_path, module_name = os.path.split(__file__)
+third_party = os.path.join(module_path, os.pardir, os.pardir, os.pardir, os.pardir)
+sys.path.append(third_party)
+import jinja2
+
+
+def apply_template(path_to_template, contents):
+    dirname, basename = os.path.split(path_to_template)
+    jinja_env = jinja2.Environment(trim_blocks=True, loader=jinja2.FileSystemLoader([dirname]))
+    template = jinja_env.get_template(basename)
+    return template.render(contents)
+
+
+def v8_class_name(interface):
+    return 'V8' + interface.name
+
+
+def cpp_class_name(interface_or_function):

[haraken, 2013/07/26 07:47:11]

interface_or_function => interface (or interface_or_operation if it's used for
operations as well)

[do-not-use, 2013/07/26 07:55:41]

The naming cpp_class_name() only makes sense if the argument is an interface.
Otherwise we could use something more generic like implementation_name() to
cpp_impl_name().

In this CL, it seems to be used only for interfaces but I don't know about
what's coming later.

[kojih, 2013/07/26 10:04:31]

This function will return [ImplementedAs] value or its original name in the
following patch.
http://www.chromium.org/blink/webidl#TOC-ImplementedAs-i-m-a-

So I think it will be
'def implemented_name(interface_or_operation_or_attribute)'.
But I'll drop this function since [ImplementedAs] is not included in this CL.

+    return interface_or_function.name
+
+
+def cpp_type(data_type):
+    """Returns the cpp type corresponds to the IDL type."""

[haraken, 2013/07/26 07:47:11]

Nit: corresponds => corresponding

[Nils Barth (inactive), 2013/07/26 10:27:47]

cpp → C++
(Only need cpp in variable names, not comments.)

+    if data_type == 'boolean':
+        return 'bool'
+    raise Exception('Not supported')
+
+
+def callback_argument_declaration(operation):
+    parameters = ['%s %s' % (cpp_type(parameter.data_type), parameter.name) for parameter in operation.arguments]

[haraken, 2013/07/26 07:47:11]

Did you decide to use "argument" in the IDL side and "parameter" in the C++
side?

I don't have a strong opinion here but it might be OK to use "argument" in the
C++ side as well. In case of "operation" and "method", we want to use "method"
in the C++ side because "operation" is too strange for C++. On the other hand,
"argument" is not too strange for C++.

[kojih, 2013/07/26 10:04:31]

ok I'll use 'argument' also in C++.

[Nils Barth (inactive), 2013/07/26 10:27:47]

This function's names + signature is weird:
usually one names a function after its *return* value,
so this would be:
callback_parameters_declaration

A comment is in order:
# "arguments" used in Web IDL, replace with more standard "parameters"


Also, for consistency, it's clearer make the loop variable agree with the
existing name:
parameter in operation.arguments
...is confusing
argument in operation.arguments
...is clear.

So here the renaming clearly happens with the new variable:
parameters = ['%s %s' % (cpp_type(argument.data_type), argument.name) for 
     argument in operation.arguments]
Agree that argument/parameter is not a big deal, but since we're changing names
*anyway*, it's a bit better to make it clear and consistent, and might avoid
confusion.

("parameter" is better than "argument", since "argument" is strictly speaking
 the *values* of a parameter)

+    return ', '.join(parameters)
+
+
+class CodeGeneratorV8:
+    def __init__(self, definitions, interface_name, output_directory, idl_directories, verbose=False):
+        self.idl_definitions = definitions
+        self.interface_name = interface_name
+        self.idl_directories = idl_directories
+        self.output_directory = output_directory
+        self.verbose = verbose
+        self.header_file_text = ''
+        self.cpp_file_text = ''

[haraken, 2013/07/26 07:47:11]

Do you need to make these instance variables?

[kojih, 2013/07/26 10:04:31]

No, made those local variables.

[Nils Barth (inactive), 2013/07/26 10:27:47]

Agreed, these should just be local variables in generate_header_and_cpp.
This is cruft from Perl, which has separate GenerateInterface and WriteData
methods, which isn't a useful distinction.
Koji has already merged the functions, so no need for these attributes.
(The only thing we do with this text is write it.)

+        self.interface = None
+
+    def cpp_class_header(self):

[haraken, 2013/07/26 07:47:11]

Nit: cpp_class_header_filename ?

[kojih, 2013/07/26 10:04:31]

done.

[Nils Barth (inactive), 2013/07/26 10:27:47]

In fact, how about putting this in __init__ as well?
(It's static data)
self.cpp_class_header_filename = cpp_class_header(file_name, interface_name)
...where cpp_class_header is just an auxiliary function.

We're going to provide it in the parser, so you'll want to extract it in init
anyway, as: 
self.cpp_class_header_filename = self.interface.rel_path_posix
...so may as well put it in __init__ now.

[kojih, 2013/07/29 03:43:21]

I'd like to leave this part as it is.
Because I think using self.interface.rel_path_posix directly is better than
using self.cpp_class_header_filename, which is just a copy of
self.interface.rel_path_posix.

[Nils Barth (inactive), 2013/07/29 04:11:09]

Got it -- once we compute the path name in the parser,
we can remove this function and computation entirely.

+        """Returns relative path from bindings/ of webcore header of the interface"""
+        # FIXME: parser will prepare posix form relative path from Source/bindings in IdlInterface.file_name

[Nils Barth (inactive), 2013/07/26 10:27:47]

Exactly.
I'll probably call it IdlInterface.rel_path_posix, if you want to be precise,
but that's fine.

+        idl_filename = self.idl_definitions.file_name
+        idl_rel_path_local = os.path.relpath(idl_filename)
+        idl_rel_path_posix = idl_rel_path_local.replace(os.path.sep, posixpath.sep)
+
+        idl_dir_posix = posixpath.join('bindings', posixpath.dirname(idl_rel_path_posix))
+        return posixpath.join(idl_dir_posix, cpp_class_name(self.interface) + '.h')

[Nils Barth (inactive), 2013/07/26 10:27:47]

self.interface_name instead of cpp_class_name(self.interface) ?

+
+    def generate_header_and_cpp(self):

[Nils Barth (inactive), 2013/07/26 10:27:47]

Shall we call this (and the generate_dummy function)
*write*_header_and_cpp
...to make it clear that these are being written to disk?

You have generate_callback_functions below,
which just computes the functions but doesn't write anything.

[kojih, 2013/07/29 03:43:21]

ok.

+        if self.interface_name not in self.idl_definitions.interfaces:
+            raise Exception('%s not in IDL definitions' % self.interface_name)
+        self.interface = self.idl_definitions.interfaces[self.interface_name]

[haraken, 2013/07/26 07:47:11]

How about moving these 3 lines into __init__ ?

[kojih, 2013/07/26 10:04:31]

done.

+        if self.interface.is_callback:
+            template_contents = {}
+            template_contents.update(self.generate_callback_functions())

[Nils Barth (inactive), 2013/07/26 10:27:47]

How about:
template_contents = self.generate_callback_functions()

[kojih, 2013/07/29 03:43:21]

done.
(but I'll use template_contents.update again because there will be
common_contents which includes conditional string for example)

+            self.header_file_text = apply_template('templates/callback.h', template_contents)
+            self.cpp_file_text = apply_template('templates/callback.cpp', template_contents)
+        else:
+            # FIXME: Implement.
+            self.header_file_text = ""
+            self.cpp_file_text = ""
+        header_filename = os.path.join(self.output_directory, v8_class_name(self.interface) + '.h')

[haraken, 2013/07/26 07:47:11]

You might want to factor out the code to write_generated_code(). I'm sorry I'm
asking you revert/restore your code back and forth.

[kojih, 2013/07/26 10:04:31]

done.

[Nils Barth (inactive), 2013/07/26 10:27:47]

BTW, this is clearest as nested functions at the top of 
generate_header_and_cpp: there's no need for methods.

def write_header(text):
    header_basename = v8_class_name(self.interface) + '.h'
    header_filename = os.path.join(self.output_directory, header_basename)
    with open(header_filename, 'w') as header_file:
        header_file.write(text)

...and then just call with:
write_header(header_file_text)

(Ditto for write_cpp.)

Also, you already have the interface name, so it seems a bit clearer to write:
'V8%s.h' % self.interface_name
or:
'V8' + self.interface_name + '.h'
...for the header_basename.

+        with open(header_filename, 'w') as header_file:
+            header_file.write(self.header_file_text)
+        cpp_filename = os.path.join(self.output_directory, v8_class_name(self.interface) + '.cpp')
+        with open(cpp_filename, 'w') as cpp_file:
+            cpp_file.write(self.cpp_file_text)
+
+    def generate_callback_functions(self):

[haraken, 2013/07/26 07:47:11]

generate_callback_functions => generate_callback_interface

[kojih, 2013/07/26 10:04:31]

done.

+        methods = []

[Nils Barth (inactive), 2013/07/26 10:27:47]

I'd put methods = [] *directly* before the loop:
the other variables are constants, while methods is generated by the loop.

Actually, more Pythonic would be to have a function operation_to_method
...and then use a list comprehension:
methods = [operation_to_method(operation) for operation in
self.interface.operations]

Basically any time you have a loop that does
some_list = []
for x in other_list:
    y = ...
    some_list.append(y)
...you're writing functional code and it's clearer (and faster) as a list
comprehension.

This also simplifies things b/c you can use 'return' in the function.
So you can write:
def operation_to_method(operation):
    if 'Custom' in operation.extended_attributes:
        return {}
    if ...
        raise ...
    if ...
        raise ...
    return { ... }
...which is rather clearer, no?

[kojih, 2013/07/29 03:43:21]

I agree, I think list comprehension has a good point that people can easily
recognize there is no strange things(dependency between elements or tricky break
or sharing status variables with other element ...)
Introduced operation_to_method().

+        cpp_includes = set([
+            'core/dom/ScriptExecutionContext.h',
+            'bindings/v8/V8Binding.h',
+            'bindings/v8/V8Callback.h',
+            'wtf/Assertions.h',
+        ])
+        header_includes = set([
+            'bindings/v8/ActiveDOMCallback.h',
+            'bindings/v8/DOMWrapperWorld.h',
+            'bindings/v8/ScopedPersistent.h',
+        ])
+        header_includes.add(self.cpp_class_header())
+        for operation in self.interface.operations:
+            custom = 'Custom' in operation.extended_attributes

[haraken, 2013/07/26 07:47:11]

Remove this.

+            method = {}
+            if 'Custom' not in operation.extended_attributes:
+                if operation.data_type != 'boolean':
+                    raise Exception("We don't yet support callbacks that return non-boolean values.")
+                if len(operation.arguments):
+                    raise Exception('Not supported')
+                method = {
+                    'return_type': cpp_type(operation.data_type),

[haraken, 2013/07/26 07:47:11]

nbarth: Probably do you want to rename operation.data_type to
operation.return_type? Either way, you can do it in a follow-up patch.

[Nils Barth (inactive), 2013/07/26 10:27:47]

Good point. Will do (in followup).

+                    'name': operation.name,
+                    'parameter_declaration': callback_argument_declaration(operation),
+                    'custom': custom,

[haraken, 2013/07/26 07:47:11]

'custom': None

+                }
+            methods.append(method)
+        template_contents = {
+            'cpp_class_name': cpp_class_name(self.interface),

[Nils Barth (inactive), 2013/07/26 10:27:47]

self.interface_name?

+            'v8_class_name': v8_class_name(self.interface),

[Nils Barth (inactive), 2013/07/26 10:27:47]

'V8' + self.interface_name ?

+            'cpp_includes': sorted(list(cpp_includes)),
+            'header_includes': sorted(list(header_includes)),
+            'methods': methods,
+        }
+        return template_contents
 
 
 def generate_dummy_header_and_cpp(target_interface_name, output_directory):