Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19607011:
Generate binding code for VoidCallback.idl with code generator in python (Closed)
|
Created:
7 years, 5 months ago by kojih Modified:
7 years, 4 months ago CC:
blink-reviews, jsbell+bindings_chromium.org, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, do-not-use Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionThis is the first step to migrate binding code generator
from perl to python (with jinja template engine).
R=haraken@chromium.org
BUG=262365
TEST=no tests. refactoring.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155080
Patch Set 1 #
Total comments: 65
Patch Set 2 : Smaller patch which support only VoidCallback.idl #Patch Set 3 : fix double -> single quote etc #
Total comments: 111
Patch Set 4 : smaller #
Total comments: 40
Patch Set 5 : nits #
Total comments: 6
Patch Set 6 : nits #
Messages
Total messages: 30 (0 generated)
r? This patch is subset of the new generator in my local branch which generate more IDLs: https://chromiumcodereview.appspot.com/17572008/#ps99001 This is still long, thanks in advance!
Note: generated code is exactly same as before.
https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:59: return dirs return reversed(dirs) ? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:74: return os.sep.join(relpath) Shouldn't this be '/'.join(relpath)? We don't want to use backslashes for includes on Windows. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:77: def get_path(dic, keys): This function is a bit obscure and does not appear to be used. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:123: conditions = dict([(expression, True) for expression in conditional.split(operator)]) Can we use a set to avoid duplicated conditions? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:161: if name not in name_to_operations: I believe you can write these 3 lines as one: name_to_operations.setdefault(name, []).append(operation) https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:190: "Uint8Array": ["unsigned char", "v8::kExternalUnsignedByteArray"], Seems like we could use tuples instead of lists? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:201: primitive_types = { Maybe we can use a set? We don't really need the associated value. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:223: non_wrapper_types = { Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:234: dom_node_types = { Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:256: matched = re.match("^sequence<([\w\d_\s]+)>.*", data_type) Why do we accept whitespace in the sequence type? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:259: return "" Why not return None? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:263: matched = re.match("^([\w\d_\s]+)\[\]", data_type) White space in array type? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:266: return "" Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:376: fullpath = root + os.sep + filename could use os.path.join()
https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:56: dirs.append(basename) Or we could simply dir.insert(0, baseline) to avoid having to reverse later on. Not sure how much performance matters here. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:94: def create_arguments(arguments): something like create_arguments_string(arguments) might be clearer. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:165: operation.overload_index = len(name_to_operations[name]) What is overload_index for? Seems it is just the len(operation.overloads)? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:405: return ", ".join(parameters) You could reuse create_arguments() https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:440: arguments_string = ", ".join(arguments) You could reuse create_arguments() https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:765: includes.append(os.path.dirname(idlRelPath) + os.sep + impl_class_name + ".h") We likely want '/' not os.sep for include paths https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:790: header_filename = self.output_directory + os.sep + get_v8_class_name(self.interface) + ".h" Could use os.path.join() https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:794: implementation_filename = self.output_directory + os.sep + get_v8_class_name(self.interface) + ".cpp" Could use os.path.join()
Woo-hoo! This looks fabulous Koji! Various comments inline, though generally looks fine and pretty clear, considering the complexity. One style point: Single quotes for strings: 'foo' (Basically to reduce use of shift key, I think.) However, double quotes for multi-line strings: """foo bar """ https://codereview.chromium.org/19607011/diff/1/Source/bindings/derived_sourc... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/derived_sourc... Source/bindings/derived_sources.gyp:276: 'templates/callback.cpp', How about using a variable for this (declared in the top 'variables' section), so you can write: '<@(jinja_template_files)', instead? (You need the '_files' suffix so GYP knows these are paths.) If you're going to have many templates, that would probably clearer? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:37: import os.path If you import os, you don't need to also import os.path. (Actually import os.path is the same as import os; they both import os, and the submodule os.path.) https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:46: sys.path.append(os.path.join(current_dir, *([os.pardir] * 4))) Slick! Maybe a bit too slick (compared with just listing them); whatever you decide, could you make sure this and the sys.path.append in blink_idl_parser and blink_idl_lexer are consistent? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:72: relpath.append("..") os.pardir instead of '..' ? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:256: matched = re.match("^sequence<([\w\d_\s]+)>.*", data_type) You need to use raw strings for regexes with backslashes (or double-escape), so: r'^sequence<([\w\d_\s]+)>.*' https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:283: return False This long string of if's would be clearer as a single boolean expression: if (is_union_type(data_type) or get_array_type(data_type) or ...) return False Indeed, I think it's even clearer as: return not(is_union_type(data_type) or get_array_type(data_type) or non_wrapper_types.get(data_type)) https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:291: def is_ref_ptr_type(data_type): Ditto re: boolean expression https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:310: if data_type in primitive_types: return data_type in primitive_types https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:315: def is_enum_type(data_type): Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:321: def is_callback_function_type(data_type): Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:327: def is_union_type(data_type): Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:333: def skip_include_header(data_type): Ditto. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:388: if filename is None: 'if not filename' is preferred (we're not distinguishing None vs. '', as both are invalid filenames). Style guide: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:570: # should be returned instead. Spec link? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:679: return "float" How about: if data_type in ['float', 'double', 'long long', 'unsigned long long']: return data_type ...to make it clear that these are represented as themselves? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:683: return "int" How about: if data_type in ['int', 'long', 'short', 'byte']: return 'int' ? https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/idl_c... File Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/idl_c... Source/bindings/scripts/idl_compiler.py:113: code_generator = code_generator_v8.code_generator_v8(definitions, interface_name, options.output_directory, idl_directories=options.idl_directories, verbose=options.verbose) This can be made shorter in a few ways: * verbose instead of options.verbose (assignment above, since it's used a lot; I forgot to use it everywhere, but I've fixed this) * You don't need the 'idl_directories=' (it's in the right position). * In fact, if you reorder the parameters to __init__ so verbose comes before generate_cpp and generate_h, you can remove the verbose= too. https://codereview.chromium.org/19607011/diff/1/Source/bindings/templates/cal... File Source/bindings/templates/callback.h (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/templates/cal... Source/bindings/templates/callback.h:1: /* BTW, this diff is against: tests/results/V8TestCallback.h ...which shows how the template is really clear! (It's just the actual code, genericized.)
BTW, in case anyone's counting: this CL moves 32 IDL files over!
Thanks nbarth and christophe. I'm not familiar with Python syntax, so your reviews are precious :) Let me take a detailed look tomorrow morning.
At a first glance, this CL looks too big. I'm concerned that this CL might cause unexpected failures on build bots. For example, using "/" instead of os.path.join() will break Windows build bots. Other things might break Android bots. To avoid those concerns, you might want to consider landing things more incrementally.
Thanks Christophe, Nils, haraken. First I'll try smaller patch which support only VoidCallback.idl.
On 2013/07/25 00:53:06, haraken wrote: > At a first glance, this CL looks too big. The bulk of the complexity is code_generator_v8.py, though the basic framework is the Jinja templating. Koji, would it be possible to cut out some of code_generator_v8 if you're only processing a few simple IDL files? > I'm concerned that this CL might cause unexpected failures on build bots. The path handling does look tricky; should be caught on try bots though. Koji, could you clearly distinguish (with comments etc.) when you want an actual filesystem path and when you want a path string for a header file? For strings in header files, you actually want to use the posixpath module explicitly, since os.path depends on the platform you're running on. (See detailed comments.)
A few more comments. Most important is: Easier to explicitly use posixpath for C++ include paths, rather than os.path or manual string manipulations. The code could also be made more OO in parts (the Perl wasn't very OO), but that shouldn't block; that's exactly what refactoring CLs are for! https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:37: import os.path For manipulating paths for C++ #include statements, you want to use the module posixpath directly, since these are always in 'a/b.h' format: you *don't* want behavior to depend on the platform. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:62: def abs2rel(path, basedir): os.path.relpath ...or rather: posixpath.relpath ...since this is used for header strings, right? There's also os.path.commonprefix, which should also be useful. The standard library is quite featureful, and hopefully sufficient for what you want to do: http://docs.python.org/2/library/os.path.html https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:70: relpath = [] Naming-wise (if we still need this), relpath_dirs, right? (B/c a list of dirs.) https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:190: "Uint8Array": ["unsigned char", "v8::kExternalUnsignedByteArray"], On 2013/07/24 10:56:20, Christophe Dumez wrote: > Seems like we could use tuples instead of lists? Good point. We could even use dictionaries or objects, with keys cpp_data_type and v8_data_type to make it really clear what's going on. (The values are not used yet, but when they are...) https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:217: enum_types = { enum_types and callback_function_types should properly be attributes of the CodeGeneratorV8 object (rather than module variables), since they depend on the definitions. This can be done in a follow-up CL though, since it works ok given how we're using it (i.e., a single call of the module). (I love OOP. Really I do. =P) The Perl code was not very OO; I've had to change the structure substantially to make it clearer and more OO. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:345: class code_generator_v8: Class names are in CamelCase: http://www.chromium.org/blink/coding-style#TOC-Python https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:763: assert idl_filename Assert is only for internal consistency, not catching runtime errors (asserts are turned off in optimized mode). Instead, one should check and raise an exception, like you in fact do for the other call to idl_file_for_interface! (Which is more consistent anyway ;-) http://wiki.python.org/moin/UsingAssertionsEffectively
Reflected comments and uploaded smaller patch. https://codereview.chromium.org/19607011/diff/1/Source/bindings/derived_sourc... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/derived_sourc... Source/bindings/derived_sources.gyp:276: 'templates/callback.cpp', Thanks, done. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:46: sys.path.append(os.path.join(current_dir, *([os.pardir] * 4))) I'll use code below from blink_idl_parser.py: 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) https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:56: dirs.append(basename) I'll use dir.insert(0, basename) here. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:62: def abs2rel(path, basedir): Oh thanks! https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:94: def create_arguments(arguments): ok. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:123: conditions = dict([(expression, True) for expression in conditional.split(operator)]) done. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:165: operation.overload_index = len(name_to_operations[name]) overload_index is identifier among set of overloaded(same named) functions. That will be used later but unnecessary in this CL. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:190: "Uint8Array": ["unsigned char", "v8::kExternalUnsignedByteArray"], I'll use class NativeTypeAndJSType https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:201: primitive_types = { done. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:217: enum_types = { moved to CodeGeneratorV8. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:256: matched = re.match("^sequence<([\w\d_\s]+)>.*", data_type) On 2013/07/24 10:56:20, Christophe Dumez wrote: > Why do we accept whitespace in the sequence type? whitespace is necessary to deal with "sequence<unsigned long>". https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:256: matched = re.match("^sequence<([\w\d_\s]+)>.*", data_type) On 2013/07/24 11:41:20, Nils Barth wrote: > You need to use raw strings for regexes with backslashes > (or double-escape), so: > r'^sequence<([\w\d_\s]+)>.*' done. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:283: return False ok. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:570: # should be returned instead. It seems: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interf... (I just copied the comment from perl generator) I'll add the link in the following patch since the first CL does not contains this function. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:679: return "float" ok. later. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:763: assert idl_filename ok. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:765: includes.append(os.path.dirname(idlRelPath) + os.sep + impl_class_name + ".h") done. https://codereview.chromium.org/19607011/diff/1/Source/bindings/scripts/code_... Source/bindings/scripts/code_generator_v8.py:790: header_filename = self.output_directory + os.sep + get_v8_class_name(self.interface) + ".h" done.
Updated: " -> '
On 2013/07/25 09:08:37, kojih wrote: > Reflected comments and uploaded smaller patch. Thanks for all the fixes; style is much clearer, and shorter patch much easier to review!
A few more style notes. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:61: def get_impl_name(interface_or_function): This could just be: return interface_or_function.extended_attributes.get('ImplementedAs', interface_or_function.name) In other contexts, the last 3 lines could just be: return implementedAs or interface_or_function.name https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:70: if conditional is None: if not conditional: return '' ...and don't need the else afterwards, as you've returned. Alternative coding is explicitly checking the key, rather than for the default value: if 'Conditional' not in interface_or_attribute_or_operation.extended_attributes: return '' conditional = interface_or_attribute_or_operation.extended_attributes['Conditional'] https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:72: else: No need for an else, since you've just returned. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:74: if '&' in conditional: This seems clearer with a for loop (anyway, no need for the last else): for operator in ['&', '|']: if operator in conditional: # Avoid duplicated conditions. conditions = set(conditional.split(operator)) return (' %s%s ' % (operator, operator)).join(['ENABLE(%s)' % expression for expression in sorted(conditions)]) return 'ENABLE(%s)' % conditional https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:155: Return native data_type corresponds to IDL data_type. Syntax: no initial linebreak, but linebreak after first line. Style: 'Return*s*', etc. Also some wording changes: """Returns native data_type corresponding to IDL data_type. @param[in] data_type ... IDL data_type @param[in] called_by_webcore @param[in] used_as_parameter """ Also, I don't understand the comment; could you elaborate? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:184: self.enum_types = dict([[enum.name, enum.values] for enum in self.idl_definitions.enumerations.values()]) This would probably be easier at the parser side. May be hard with Perl around, but I can do this in future. Could you put a FIXME here? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:219: data_type == 'any' or data_type == 'DOMString' or A bit clearer to consistently have each on the same line. ...and probably better to have the literal checks first, then the function ones. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:228: self.is_enum_type(data_type) or \ Style: use parentheses for implicit line continuation instead of explicit line continuation with backslashes. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Line_length https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:303: raise Exception('We don''t yet support callbacks that return non-boolean values.') BTW, it's fine to use "" for quotes if there are single quotes inside, as it makes it clearer. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:305: if len(operation.arguments) > 0: No need for > 0.
I'm sorry about too many comments below. Overall, I think we have to migrate things to the new Python flow incrementally in order not to migrate existing messes in the old Perl flow. I'm sorry that I have to say a lot of "Drop xxx from this CL", but I'd guess that "landing only Python code that is really needed to migrate IDL files in each CL" would be the best way to keep the new Python generater clean. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:58: 'jinja_template_files': [ Nit: code_generator_template_files https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:39: import re Nit: Alphabetical order please. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:50: def apply_template(path_to_template, params): params => parameters Probably 'contents' might be better. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:57: def get_v8_class_name(interface): get_v8_class_name => v8_class_name Omitting "get" is a Blink convention. It's not good that the Perl code generator is using GetV8ClassName(). https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:61: def get_impl_name(interface_or_function): Good point! You're a Python expert :) Other comments: get_impl_name => cpp_class_name In the new code generator, I suggest using "cpp" instead of "implementation" or "impl" or "native". Having v8_class_name and cpp_class_name would be clearer than having v8_class_name and impl_name. Would you do the "implementation/impl/native" => "cpp" renaming in other places as well? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:68: def get_conditional_string(interface_or_attribute_or_operation): get_conditional_string => conditional_string https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:74: if '&' in conditional: Looks better! https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: return sorted(list(set(includes))) How about assuming that includes is a set? There would be no reason we want to manage includes in a list. Then you can remove this helper method, since you can just sort includes in the caller sites. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:90: class NativeTypeAndJSType: CPPTypeAndV8Type Actually I don't think this class is needed. As far as I see the Perl code generator, the cpp type is not used. We will need only V8 type. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:95: # IDL data_type: [element's C++ data_type, V8 data_type] I'd remove this comment. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:98: 'ArrayBufferView': NativeTypeAndJSType(), As far as I see the Perl code generator, these two items are not used. We don't want to migrate unnecessary code to the new Python flow. (This is the great benefit of the rewrite!) This is the reason why I want to land things incrementally. Let's migrate things to the new Python flow incrementally when we find they're really necessary. In that sense, you might want to drop all unused code from this CL. For example, typed_arrays wouldn't be needed in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:110: primitive_types = set([ It looks like that methods and global variables are placed randomly in the code generator. (This is an existing problem in the Perl code generator.) Please consider organizing the order of all methods and global variables for better readability. For example, you can put global variables at the beginning of the file and gather helper methods at the end of the file. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:127: def get_sequence_type(data_type): Drop this method in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:134: def get_array_type(data_type): Ditto. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:141: def is_typed_array_type(data_type): Ditto. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:149: def is_union_type(data_type): Ditto. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:153: def get_native_type(data_type): get_native_type => native_type https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:155: Return native data_type corresponds to IDL data_type. native => cpp > @param[in] data_type ... IDL data_type > @param[in] called_by_webcore > @param[in] used_as_parameter I'd remove this part. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:165: def get_callback_parameter_declaration(operation): get_callback_parameter_declaration => callback_argument_declaration We might want to rename "parameter" to "argument" for consistency. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:171: def __init__(self, definitions, interface_name, output_directory, idl_directories, verbose=False, generate_h=True, generate_cpp=True): Do we need generate_h and generate_cpp ? In case where we don't need h and cpp, we can just directly call generate_dummy_header_and_cpp from idl_compiler.py. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:180: self.implementation = '' implementation => cpp https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:181: self.cached_interfaces = {} Drop this in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:184: self.enum_types = dict([[enum.name, enum.values] for enum in self.idl_definitions.enumerations.values()]) I'd drop this in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:185: self.callback_function_types = self.idl_definitions.callback_functions Shall we rename "function" to "method"? I think we're using "method" in generated code. By the way, why do you call this not self.callback_functions but self.callback_function_types? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:187: def is_enum_type(self, data_type): Drop this in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:196: includes = [] Don't you want to use a set for includes? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:202: includes.append('bindings/v8/ScriptValue.h') Are these needed in this CL? Header includes in the Perl code generator are much messier than is necessary. We're including a bunch of unnecessary files and want to clean them up in the rewrite. So let's add #include when it becomes really necessary in your CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:207: def get_includes_for_parameter(self, parameter): parameter => argument The same comment to other places. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:208: includes = [] Ditto. Can you use a set? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:211: if self.is_ref_ptr_type(array_or_sequence_type): I don't understand what this branch is for. Maybe it's needed but maybe it's not needed. There are a lot of branches in the Perl code generation that are never taken. So let's clean them up by migrating really-necessary branches. So you might want to drop this branch if it's not needed in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:226: def skip_include_header(self, data_type): skip_include_header => is_skip_include https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:232: def header_files_for_interface(self, interface_name, impl_class_name): get_includes_for_interface (for naming consistency with other methods) Actually I don't fully understand why we need all of get_includes_for_{type,parameter,interface}. We might clean them up when migrating to the new Python flow. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:233: includes = [] Can you use a set? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:237: # FIXME: parser will prepare posix form relative path from Source/bindings in IdlInterface.file_name Yeah, we should do that. nbarth: could you do that? :) https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:248: self.common_template_parameters = { Nit: common_template_parameters => common_template_contents parameter is a bit confusing with parameters of DOM operations. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:270: with open(header_filename, 'w') as f: Nit: f => file https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:273: implementation_filename = os.path.join(self.output_directory, get_v8_class_name(self.interface) + '.cpp') implementation_filename => cpp_filename https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:274: with open(implementation_filename, 'w') as f: Nit: f => file https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:277: raise Exception('%s not in IDL definitions' % self.interface_name) I'd prefer: if not self.interface_name in self.idl_definitions.interfaces: raise Exception('%s not in IDL definitions' % self.interface_name) ... ... to avoid deep indentations. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:279: def generate_callback_template_parameters(self): generate_callback_template_parameters => generate_callback_functions Note: here we might want to use "function" instead of "method" because "callback function" is a concept of the Web IDL spec. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:283: implementation_includes = [ Can you use a set for the includes? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:283: implementation_includes = [ implementation_includes => cpp_includes https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:289: header_includes = [ Ditto. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:298: if not custom: if not 'Custom' in operation.extended_attributes: https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:311: 'prepare_js_parameters': '', Drop this in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:312: 'handles': ',\n'.join(['%sHandle' % parameter.name for parameter in operation.arguments]), You don't need to add "\n". https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:313: 'parameters': parameters, Nit: parameters => arguments https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:319: template_parameters = { template_parameters => template_contents https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:324: 'header_includes': header_includes, If header_includes is a set, you can just say: sort(list(header_includes)) https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:325: 'functions': functions, functions => methods As christophe commented in the previous CL, we should use "operation" in the IDL world and use "method" in the cpp world. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:330: # TODO: implement TODO => FIXME https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:333: def generate_implementation(self): generate_implementation => generate_cpp https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:334: # TODO: implement TODO => FIXME https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... File Source/bindings/templates/callback.cpp (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:2: This file is part of the Blink open source project. Ditto. Please use a proper copyright. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:29: {% endfor %} Nit: One empty line please below this line. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:43: // Functions Nit: I'd remove this comment. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:62: {{parameter["native_to_js_value_statement"] | indent(4)}} Looks like "native_to_js_value_statement" is not defined in the code generator. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:68: {% endfor %} Nit: Wrong indentation. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:79: return !invokeCallback(m_callback.newLocal(isolate), {{function["parameters"]|length}}, argv, callbackReturnValue, scriptExecutionContext()); Nit: Spaces are needed around '|'. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:83: {% endfor %} Nit: You might want to add a jinja comment about what {% for %} corresponds to this {% endfor %}. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... File Source/bindings/templates/callback.h (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:2: This file is part of the Blink open source project. Please use a new copyright in Blink. You can copy the copyright of idl_definitions_builder.py. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:3: This file has been auto-generated by CodeGeneratorV8.pm. DO NOT MODIFY! But let's leave this line. You can leave this line as a comment outside of the copyright. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:27: {% for filename in header_includes -%} Help me understand: What is "-%"? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:36: class {{v8_class_name}} : public {{interface_name}}, public ActiveDOMCallback { interface_name => cpp_class_name Remember that we have three concepts: interface_name, v8_class_name and cpp_class_name.
A few more style points. Also agreeing with haraken that can probably strip this further to get just the minimal (and minimally crufty) code needed for these IDLs. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:171: def __init__(self, definitions, interface_name, output_directory, idl_directories, verbose=False, generate_h=True, generate_cpp=True): On 2013/07/26 02:03:30, haraken wrote: > Do we need generate_h and generate_cpp ? I was wondering the same thing. If we really want to separate these, we should just have generate_h and generate_cpp *methods*: there's no need for these to be parameters. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:184: self.enum_types = dict([[enum.name, enum.values] for enum in self.idl_definitions.enumerations.values()]) On 2013/07/26 02:03:30, haraken wrote: > I'd drop this in this CL. Good point. Koji, no enums in these IDLs, right? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:185: self.callback_function_types = self.idl_definitions.callback_functions On 2013/07/26 02:03:30, haraken wrote: > Shall we rename "function" to "method"? I think we're using "method" in > generated code. This is an important general point: the parser strictly follows the Web IDL naming, but in the C++ code we use standard C++ names (e.g., "method" rather than "operator", "parameters" rather than "arguments"). Renaming when you read from the parsed data is probably the clearest way to handle this. > By the way, why do you call this not self.callback_functions but > self.callback_function_types? I think this is (primarily?) used just to check types (so it could be a set), but you're right, it's actually a full dict. Koji? https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:237: # FIXME: parser will prepare posix form relative path from Source/bindings in IdlInterface.file_name On 2013/07/26 02:03:30, haraken wrote: > > Yeah, we should do that. > > nbarth: could you do that? :) Will do! It's a bit tricky to do properly, since we want to export the legacy form for JSON comparison with Perl, but I'll do it cleanly, leaving a compatibility hack for later removal. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:270: with open(header_filename, 'w') as f: On 2013/07/26 02:03:30, haraken wrote: > Nit: f => file Even better, header_file For clarity, I've been consistently using *_filename for the string and *_file for the corresponding file, so you have: with open(header_filename, 'w') as header_file: ... ...which is clear. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:274: with open(implementation_filename, 'w') as f: On 2013/07/26 02:03:30, haraken wrote: > > Nit: f => file => cpp_file https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:277: raise Exception('%s not in IDL definitions' % self.interface_name) BTW: if key not in dict: ... ...is a bit clearer and preferred to: if not key in dict: ... (Go syntactic sugar!) https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:298: if not custom: On 2013/07/26 02:03:30, haraken wrote: > if not 'Custom' in operation.extended_attributes: Even better: if 'Custom' not in operation.extended_attributes:
Thanks for a lot of comments! revised version. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:58: 'jinja_template_files': [ done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:61: def get_impl_name(interface_or_function): I agree. done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:74: if '&' in conditional: Thanks, done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: return sorted(list(set(includes))) Good idea! done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:98: 'ArrayBufferView': NativeTypeAndJSType(), ok. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:110: primitive_types = set([ ok, done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:165: def get_callback_parameter_declaration(operation): done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:171: def __init__(self, definitions, interface_name, output_directory, idl_directories, verbose=False, generate_h=True, generate_cpp=True): oh this was for rewriting work. (for example, generate only h and do diff) removed. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:184: self.enum_types = dict([[enum.name, enum.values] for enum in self.idl_definitions.enumerations.values()]) exactly. removed. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:185: self.callback_function_types = self.idl_definitions.callback_functions On 2013/07/26 03:02:40, Nils Barth wrote: > On 2013/07/26 02:03:30, haraken wrote: > > Shall we rename "function" to "method"? I think we're using "method" in > > generated code. > > This is an important general point: > the parser strictly follows the Web IDL naming, but in the C++ code > we use standard C++ names (e.g., "method" rather than "operator", > "parameters" rather than "arguments"). > Renaming when you read from the parsed data is probably the clearest way to > handle this. > > > By the way, why do you call this not self.callback_functions but > > self.callback_function_types? > > I think this is (primarily?) used just to check types > (so it could be a set), > but you're right, it's actually a full dict. > Koji? Yeah now this is unnecessary. This was primarily straight-forward translation of the perl code: %callbackFunctionTypeHash = map { $_->name => $_ } @{$idlDocument->callbackFunctions}; https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:219: data_type == 'any' or data_type == 'DOMString' or I'll do that in the following patches. (this part is dropped in this CL) https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:228: self.is_enum_type(data_type) or \ I'll do that in the following patches. (this part is dropped in this CL) https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:232: def header_files_for_interface(self, interface_name, impl_class_name): Good point......... HeaderFilesForInterface actually returns webcore header of the specified type(interface name). On the other hand AddIncludesForType returns binding header of the specified type. So I rename 'header_files_for_interface' to 'cpp_class_header(data_type)' and add 'v8_class_header(data_type)' later. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:233: includes = [] done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:248: self.common_template_parameters = { done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:248: self.common_template_parameters = { done. replaced all template_parameters with template_contents/ https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:270: with open(header_filename, 'w') as f: done. we can't use type, file as variable name in python :( https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:273: implementation_filename = os.path.join(self.output_directory, get_v8_class_name(self.interface) + '.cpp') done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:277: raise Exception('%s not in IDL definitions' % self.interface_name) done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:298: if not custom: done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:303: raise Exception('We don''t yet support callbacks that return non-boolean values.') done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:305: if len(operation.arguments) > 0: done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:312: 'handles': ',\n'.join(['%sHandle' % parameter.name for parameter in operation.arguments]), ok, but dropped in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:313: 'parameters': parameters, ok, but dropped in this CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... File Source/bindings/templates/callback.cpp (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.cpp:62: {{parameter["native_to_js_value_statement"] | indent(4)}} Removed this block since parameters are not supported in the first CL. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... File Source/bindings/templates/callback.h (right): https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:2: This file is part of the Blink open source project. done. since idl_definitions_builder.py is a python file, I used the one: http://www.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:3: This file has been auto-generated by CodeGeneratorV8.pm. DO NOT MODIFY! done. https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:27: {% for filename in header_includes -%} With '-' before '%}', whitespaces between '%}' and the next non-whitespace will be removed. http://jinja.pocoo.org/docs/templates/#whitespace-control But since I use trim_blocks=True in template set up, '-%' here is unnecessary. trim_blocks=True means the first newline after a block is removed. http://jinja.pocoo.org/docs/api/#high-level-api https://codereview.chromium.org/19607011/diff/20001/Source/bindings/templates... Source/bindings/templates/callback.h:36: class {{v8_class_name}} : public {{interface_name}}, public ActiveDOMCallback { done.
Thanks for the update! The CL looks much easier for reviewing. Almost looks good. nbarth might want to do a final check. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:60: def cpp_class_name(interface_or_function): interface_or_function => interface (or interface_or_operation if it's used for operations as well) https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:65: """Returns the cpp type corresponds to the IDL type.""" Nit: corresponds => corresponding https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:72: parameters = ['%s %s' % (cpp_type(parameter.data_type), parameter.name) for parameter in operation.arguments] 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++. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:84: self.cpp_file_text = '' Do you need to make these instance variables? https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: def cpp_class_header(self): Nit: cpp_class_header_filename ? https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:100: self.interface = self.idl_definitions.interfaces[self.interface_name] How about moving these 3 lines into __init__ ? https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:110: header_filename = os.path.join(self.output_directory, v8_class_name(self.interface) + '.h') 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:117: def generate_callback_functions(self): generate_callback_functions => generate_callback_interface https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:132: custom = 'Custom' in operation.extended_attributes Remove this. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:140: 'return_type': cpp_type(operation.data_type), 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:143: 'custom': custom, 'custom': None https://codereview.chromium.org/19607011/diff/33001/Source/bindings/templates... File Source/bindings/templates/callback.cpp (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/templates... Source/bindings/templates/callback.cpp:69: Nit: Remove this line. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/templates... File Source/bindings/templates/callback.h (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/templates... Source/bindings/templates/callback.h:44: Nit: Remove this line. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/templates... Source/bindings/templates/callback.h:73: Nit: Remove the empty line.
Nice. Much simpler indeed. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:60: def cpp_class_name(interface_or_function): On 2013/07/26 07:47:11, haraken wrote: > > interface_or_function => interface (or interface_or_operation if it's used for > operations as well) 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.
revised. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:60: def cpp_class_name(interface_or_function): 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:72: parameters = ['%s %s' % (cpp_type(parameter.data_type), parameter.name) for parameter in operation.arguments] ok I'll use 'argument' also in C++. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:84: self.cpp_file_text = '' No, made those local variables. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: def cpp_class_header(self): done. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:100: self.interface = self.idl_definitions.interfaces[self.interface_name] done. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:110: header_filename = os.path.join(self.output_directory, v8_class_name(self.interface) + '.h') done. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:117: def generate_callback_functions(self): done.
LGTM but let haraken and nils take a look as well.
LGTM. Given that you already have a big CL, it would be flustrating for you to split the CL into small pieces. But we might want to do that to make the new Python flow clean. Thanks for your efforts! https://codereview.chromium.org/19607011/diff/42001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/42001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:134: method = { You might want to set 'method.arguments': [] https://codereview.chromium.org/19607011/diff/42001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:137: 'parameter_declaration': callback_argument_declaration(operation), parameter_declaration => argument_declaration https://codereview.chromium.org/19607011/diff/42001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:151: def generate_dummy_header_and_cpp(target_interface_name, output_directory): You can put this method inside the CodeGeneratorV8 class. https://codereview.chromium.org/19607011/diff/42001/Source/bindings/templates... File Source/bindings/templates/callback.cpp (right): https://codereview.chromium.org/19607011/diff/42001/Source/bindings/templates... Source/bindings/templates/callback.cpp:55: {{method.return_type}} {{v8_class_name}}::{{method.name}}({{method.parameter_declaration}}) Ditto. https://codereview.chromium.org/19607011/diff/42001/Source/bindings/templates... Source/bindings/templates/callback.cpp:72: return !invokeCallback(m_callback.newLocal(isolate), {{method["parameters"] | length}}, argv, callbackReturnValue, scriptExecutionContext()); parameters => arguments https://codereview.chromium.org/19607011/diff/42001/Source/bindings/templates... File Source/bindings/templates/callback.h (right): https://codereview.chromium.org/19607011/diff/42001/Source/bindings/templates... Source/bindings/templates/callback.h:56: virtual {{method.return_type}} {{method.name}}({{method.parameter_declaration}}); parameter_declaration => argument_declaration
LGTM from me as well. Some refactoring and naming suggestions, but nothing substantive. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:65: """Returns the cpp type corresponds to the IDL type.""" On 2013/07/26 07:47:11, haraken wrote: > Nit: corresponds => corresponding cpp → C++ (Only need cpp in variable names, not comments.) https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:72: parameters = ['%s %s' % (cpp_type(parameter.data_type), parameter.name) for parameter in operation.arguments] On 2013/07/26 07:47:11, haraken wrote: > Did you decide to use "argument" in the IDL side and "parameter" in the C++ > side? 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] > 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++. 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) https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:84: self.cpp_file_text = '' On 2013/07/26 07:47:11, haraken wrote: > Do you need to make these instance variables? 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.) https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: def cpp_class_header(self): 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:89: # FIXME: parser will prepare posix form relative path from Source/bindings in IdlInterface.file_name Exactly. I'll probably call it IdlInterface.rel_path_posix, if you want to be precise, but that's fine. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:95: return posixpath.join(idl_dir_posix, cpp_class_name(self.interface) + '.h') self.interface_name instead of cpp_class_name(self.interface) ? https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:97: def generate_header_and_cpp(self): 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:103: template_contents.update(self.generate_callback_functions()) How about: template_contents = self.generate_callback_functions() https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:110: header_filename = os.path.join(self.output_directory, v8_class_name(self.interface) + '.h') On 2013/07/26 07:47:11, haraken wrote: > 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. 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:118: methods = [] 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? https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:140: 'return_type': cpp_type(operation.data_type), On 2013/07/26 07:47:11, haraken wrote: > 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. Good point. Will do (in followup). https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:147: 'cpp_class_name': cpp_class_name(self.interface), self.interface_name? https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:148: 'v8_class_name': v8_class_name(self.interface), 'V8' + self.interface_name ?
https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: def cpp_class_header(self): 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. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:97: def generate_header_and_cpp(self): ok. https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:103: template_contents.update(self.generate_callback_functions()) done. (but I'll use template_contents.update again because there will be common_contents which includes conditional string for example) https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:118: methods = [] 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().
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kojih@chromium.org/19607011/50001
https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/19607011/diff/33001/Source/bindings/scripts/c... Source/bindings/scripts/code_generator_v8.py:87: def cpp_class_header(self): On 2013/07/29 03:43:21, kojih wrote: > 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. Got it -- once we compute the path name in the parser, we can remove this function and computation entirely.
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kojih@chromium.org/19607011/50001
Message was sent while issue was closed.
Change committed as 155080 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
