Add Python flow to bindings generation, move dummy-generating IDL files over

To transition to bindings generation in Python rather than Perl,
we're adding a parallel flow in GYP, and moving IDL files one-by-one
as the generator is able to handle them.

This CL contains the necessary build changes and basic Python scripts,
but does not include the parser or code generator, which will come later.

The build generates exactly the same bindings files before and after this CL.

In detail, this CL:
* Splits the existing GYP target 'bindings_sources' in two:
  - 'perl_bindings_sources' (rename existing),
  - 'python_bindings_sources' (new one, just copy-paste Perl one and change script),
  ...and splits the various 'idl_files' lists into 'perl_...' and 'python_...',
  so we can just move files one-by-one from the Perl list to the Python list.

* Adds generate_bindings.py, the main Python bindings build script
  Almost complete, just does not call the parser or the real code generator.

* Adds a stub build_ir.py
  This will eventually be the main AST to IR converter, but for now just does
  dependency resolution and raises an IdlNotFoundError exception,
  instructing generate_bindings.py to make dummy files.

* Adds a stub code_generator_v8.py, which only generates dummy files
  (.cpp and .h only needed to provide 'outputs' for build).

* Moves the 37 IDL files that generate dummy files over to Python.


Also a few cleanups I noticed on the way (commented inline).

BUG=239771
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153878

[Nils Barth (inactive), 2013-07-08 05:24:37]

Hi haraken,
Here's the "Python build changes" CL!
(No parser or code generator yet, but handles the dummy .cpp & .h files.)

(Minor comments below.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (left):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:190: '--include', '../core',
(Consistent order with overall IDL lists.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:56: 'bindings_output_dir':
'<(SHARED_INTERMEDIATE_DIR)/webkit/bindings',
(Added bindings_output_dir for clarity and consistency.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
File Source/bindings/scripts/generate-bindings.pl (left):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate-bindings.pl:30: 
This comment is out of date.
generate-bindings.pl/generate_bindings.py are now meaningful scripts that call
the parser and code generator, and are in turn called off of build rules driven
off the IDL files (that's what the GYP Rules do!).

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate-bindings.pl:46: my $filename;
Unused, remove.

[haraken, 2013-07-08 06:06:29]

This is a great step to move things forward!

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:54: ],

Shall we rename:

  perl_idl_files => idl_files_for_deprecated_perl
  python_idl_files => idl_files_for_python

to make it clear that the perl flow is deprecated? The same comment for other
target names.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:217: 'scripts/generate-bindings.pl',
Would you rename:

  generate-bindings.pl => deprecated_generate_bindings.pl
  IDLParser.pm => deprecated_idl_parser.pm
  CodeGeneratorV8.pm => deprecated_code_generator.pm

to make it clear that the perl flow is being deprecated?

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
File Source/bindings/scripts/build_ir.py (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:1: # Copyright (C) 2013 Google Inc. All
rights reserved.

Actually I don't see a benefit in splitting build_ir.py and
generate-bindings.py. Shall we put the contents of these two files into one file
and call it idl_compiler.py? Then the architecture becomes clearer:

  idl_compiler.py : driver
  idl_parser.py : parser
  code_generator_v8.py : code generator

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:29: """Convert generic Web IDL AST to Blink
IR.

As commented below, I'd avoid using the word "IR" since the meaning is
ambiguous. You can use "parsing tree" instead.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:33: and validates extended attributes.

Where is this implemented?

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:47: # Generic Web IDL AST to Blink IR

Nit: Remove this comment.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:50: def read_merged_idl(idl_filename,
supplemental_dependency_file=None, additional_idl_files=None):

read_merged_idl => resolve_idl_dependency ?

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:60: # Merge supplemental interfaces
('partial interface' and 'implements')

Nit: Remove this comment.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:64: """Raised if IDL not found in
dependencies file or additional files list."""

Slightly better:

  Raised if the IDL file is a partial IDL file.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:68: def
compute_supplemental_idl_files(target_idl_basename,
supplemental_dependencies_filename, additional_idl_files):

compute_supplemental_idl_files => compute_partial_idl_files
supplemental_dependencies_filename => partial_dependencies_filename

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:74: file) nor in the additional IDL files
list, raise an IdlNotFoundError.

Slightly better:

- Returns a list of partial IDL files on which a given IDL file depends.
- Returns an empty list if the given IDL file is an additional IDL file.
- Raises IdlNotFoundError otherwise. This happens when the given IDL file is a
partial IDL file, for which we don't want to generate .h and .cpp files.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:84: # DOMWindow.idl is supplemented by
P.idl, Q.idl, and R.idl,

P.idl, Q.idl and R.idl are partial files of DOMWindow.idl.

The word "supplemental" is deprecated. Please replace it with "partial" in all
other parts in this CL.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:92: return []

This should raise an exception.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:31: Given an IR of the IDL files
for an interface (produced by the frontend),

I'd avoid using the work "IR" and "frontend" since it's not clear what they
mean. You can just say:

  Input: A parsing tree of a given interface X
  Output: V8X.h and V8X.cpp

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:34: FIXME: Currently a stub, as
part of landing the parser and code generator

Let's explain our working plan here. In other words, you can explain:

- We temporarily have two build flows:
  [1] deprecated_idl_parser.pl => deprecated_code_generator_v8.pm
  [2] idl_parser.py => code_generator_v8.py

- We will move IDL files from the build flow [1] to the build flow [2]
incrementally.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:51: */

You might want to use the jinja template for this. Please ask koji-san how to
use jinja.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
File Source/bindings/scripts/generate_bindings.py (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate_bindings.py:42: parser.add_option('--defines')

Is this still needed? Now we no longer need preprocessor.

[Nils Barth (inactive), 2013-07-09 08:48:16]

Thanks for detailed comments haraken!
Revised as per comments.
Changes (from previous) are almost all naming and comments
(and replies to comments); hopefully clearer.
Only code change is removing defines, since no longer used.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:54: ],
On 2013/07/08 06:06:29, haraken wrote:
> Shall we rename:
>   perl_idl_files => idl_files_for_deprecated_perl
> to make it clear that the perl flow is deprecated?

That's pretty clear, done (here and throughout)!

However, we have to keep _files as a suffix, as that's a magic extension
in GYP that means these are treated as path names:
https://code.google.com/p/gyp/wiki/InputFormatReference#Pathname_Relativization

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:217: 'scripts/generate-bindings.pl',
On 2013/07/08 06:06:29, haraken wrote:
> Would you rename:
> 
>   generate-bindings.pl => deprecated_generate_bindings.pl
>   IDLParser.pm => deprecated_idl_parser.pm
>   CodeGeneratorV8.pm => deprecated_code_generator.pm
> 
> to make it clear that the perl flow is being deprecated?

Done.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
File Source/bindings/scripts/build_ir.py (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:1: # Copyright (C) 2013 Google Inc. All
rights reserved.
On 2013/07/08 06:06:29, haraken wrote:
> Actually I don't see a benefit in splitting build_ir.py and
> generate-bindings.py.

In the context of the full parsing, these are quite separate,
though admittedly it's not clear just from this initial CL.

build_ir.py is actually a major file (700 lines in local branch);
this is just a stub.
You can look at the full file here:
https://codereview.chromium.org/15801003/

Agree that the current file is pretty small, but it's the start of a big
library.

build_ir.py does three things:
1. Convert the AST produced by the parser into the IR that the code generator
uses. This is the bulk of the work.
2. Merge dependencies into the main interface.
3. Validates IDL extended attributes.

Once these are all done, we have a complete, validated IR that can
be passed on to the code generator.

Architecturally, read_merged_idl is the main function, which
reads in an IDL file and does all the necessary work.
It's important that this be in a separate library from our compiler,
since other people (notably Dart, and in future bindings) want to
be able to read IDL files using our code, so they need to be able
to call the merging and validation code.

> Shall we put the contents of these two files into one file
> and call it idl_compiler.py?

idl_compiler.py is a clearer name than generate_bindings.py;
changed!

> Then the architecture becomes clearer:
> 
>   idl_compiler.py : driver
>   idl_parser.py : parser
>   code_generator_v8.py : code generator

This actually is the architecture, but the "parser" is actually
in 3 stages, hence why I'm referring to it as "frontend":
* blink_idl_lexer.py : lexer (makes token stream)
* blink_idl_parser.py : parser (makes AST)
* build_ir.py : converts AST to IR, merges dependencies, validates attributes
(we need the "blink_" prefixes b/c the underlying Pepper parser uses
idl_lexer.py and idl_parser.py itself).

So the architecture is:
* driver
* frontend: (lexer, parser, AST to IR + etc.)
* backend: code generator
...as you suggest!

(We don't need to use the term "backend" here, since we only have one, but Dart
uses 3 backends, for instance, and extensions may use another one.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:29: """Convert generic Web IDL AST to Blink
IR.
On 2013/07/08 06:06:29, haraken wrote:
> As commented below, I'd avoid using the word "IR" since the meaning is
> ambiguous. You can use "parsing tree" instead.

The code generator takes as input a Python object (of class IdlDocument), which
is our IR. So the term "IR" is unambiguous, but unclear without more context –
I've clarified the comments.
It's clear when you look at ir.py (in the full parser CL), which shows
the full class hierarchy, but it should be clear from this comment as well.

The term "parsing tree" (Concrete Syntax Tree, CST, corresponding 
directly to the production rules of the grammar) isn't accurate.

Rather:
The parser itself (blink_idl_parser.py) walks the CST and produces an AST
(discarding useless grammar rules, building lists, etc.).
It's then a lot of code to convert this AST to the Python object,
which is most of what build_ir.py actually does (eventually, once it lands).

That's why I'm using the term "IR": we've processed the tree and produced
something actually usable, so the code generator doesn't need to
walk the tree itself.
(This architecture is following the existing Perl: make an object.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:33: and validates extended attributes.
On 2013/07/08 06:06:29, haraken wrote:
> Where is this implemented?

The validation isn't included in this CL, which is just the minimum
necessary for a testable build change.
The conversion of the AST to the IR also isn't included.

The comment is "what this library will eventually include",
and there's a "FIXME: Currently a stub" comment below.
Should I make that more prominent (put the FIXME first?),
or add a comment like "NOT YET LANDED", or otherwise reword?

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:47: # Generic Web IDL AST to Blink IR
On 2013/07/08 06:06:29, haraken wrote:
> Nit: Remove this comment.

Done.
(The full file is rather long, so I have comments to break up
the sections, but certainly now these are excessive.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:50: def read_merged_idl(idl_filename,
supplemental_dependency_file=None, additional_idl_files=None):
On 2013/07/08 06:06:29, haraken wrote:
> read_merged_idl => resolve_idl_dependency ?

This actually calls the parser, converts to IR, does merging, and validation –
it's the major function – but I've currently removed all of these, so all that's
left is resolving the dependencies.
It's probably clearer if I include these other actions but commented out: see
revised CL.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:60: # Merge supplemental interfaces
('partial interface' and 'implements')
On 2013/07/08 06:06:29, haraken wrote:
> 
> Nit: Remove this comment.

Done.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:64: """Raised if IDL not found in
dependencies file or additional files list."""
On 2013/07/08 06:06:29, haraken wrote:
> Slightly better:
> 
>   Raised if the IDL file is a partial IDL file.

Dependencies now include both partial IDL files and implements,
neither of which we generate bindings for.
It is unclear though; I've reworded it for clarity.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:68: def
compute_supplemental_idl_files(target_idl_basename,
supplemental_dependencies_filename, additional_idl_files):
On 2013/07/08 06:06:29, haraken wrote:
> compute_supplemental_idl_files => compute_partial_idl_files
> supplemental_dependencies_filename => partial_dependencies_filename

Changed!
(Actually to "dependencies" because also includes 'implements'.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:74: file) nor in the additional IDL files
list, raise an IdlNotFoundError.
On 2013/07/08 06:06:29, haraken wrote:
> Slightly better:
> 
> - Returns a list of partial IDL files on which a given IDL file depends.
> - Returns an empty list if the given IDL file is an additional IDL file.
> - Raises IdlNotFoundError otherwise. This happens when the given IDL file is a
> partial IDL file, for which we don't want to generate .h and .cpp files.

I've reworded following this, trying to avoid redundancy with the
description of the exception.
We actually *do* want to generate .h and .cpp files for the dependencies,
but they're supposed to be dummy ones.
I've said "we don't want to generate bindings", though we could say
"don't want to generate *real* .h and .cpp files" (which sounds a bit
more confusing to me).

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:84: # DOMWindow.idl is supplemented by
P.idl, Q.idl, and R.idl,
On 2013/07/08 06:06:29, haraken wrote:
> The word "supplemental" is deprecated. Please replace it with "partial" in all
> other parts in this CL.

Replaced with "dependencies", since includes both partials and implements.

Done, and fixed everywhere in other CL:
Issue 18341025: Rename deprecated "supplemental_dependencies" and clean up IDL
dependency script
https://codereview.chromium.org/18341025/

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:92: return []
On 2013/07/08 06:06:29, haraken wrote:
> This should raise an exception.

The dependencies file is optional, as is the list of additional IDL files
(defaults to empty).
(They're both currently optional in Perl.)
We could change this and make the dependencies file required.
It's slightly cleaner to be optional, as it means we don't depend on
compute_dependencies.py unless we have to (so you can compile a single
IDL file).

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:31: Given an IR of the IDL files
for an interface (produced by the frontend),
On 2013/07/08 06:06:29, haraken wrote:
> I'd avoid using the work "IR" and "frontend" since it's not clear what they
> mean.
I've clarified above, where the terms are useful, but we don't need
them here – we can just say "object".

> You can just say:
> 
>   Input: A parsing tree of a given interface X
>   Output: V8X.h and V8X.cpp

I've reworded slightly (object, not parsing tree), but other have this.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:34: FIXME: Currently a stub, as
part of landing the parser and code generator
On 2013/07/08 06:06:29, haraken wrote:
> Let's explain our working plan here.

Done!
(Please feel free to suggest rewordings.)

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:51: */
On 2013/07/08 06:06:29, haraken wrote:
> You might want to use the jinja template for this.
> Please ask koji-san how to use jinja.

That's a really good point – we don't want manual templates here.

I should actually be able to get rid of these dummy files by
making some changes to the build (so the dependency files are
dependencies but not compiled), so this should go away soon.

So for now let's just use this function, as it's short and going to die;
it's mostly useful because it gives us real files to run in the
Python flow.
(We don't want to also introduce Jinja in this CL!)

OTOH, if Koji would like to switch this to a Jinja template as a
first step of landing his Jinja-based code generator, I'm happy
to leave it around for a while.

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
File Source/bindings/scripts/generate_bindings.py (right):

https://codereview.chromium.org/18190004/diff/3001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate_bindings.py:42: parser.add_option('--defines')
On 2013/07/08 06:06:29, haraken wrote:
> Is this still needed? Now we no longer need preprocessor.

Nope, we can remove it – done!
(Just checked with Koji.)
It's actually not needed for Perl either, since we've removed the
defines from all the IDL files, so I've removed it from Perl and GYP too!
(Checked: no changes to output.)

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (left):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:34: '../core/features.gypi',
Defines have been removed from IDLs, so can remove this include
(and --defines from Perl).

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_generate_bindings.pl (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_generate_bindings.pl:5: #
Presubmit check complained about trailing whitespace, so fix. =P

[haraken, 2013-07-09 11:16:47]

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:259:
'<(SHARED_INTERMEDIATE_DIR)/supplemental_dependency.tmp',

I think this was renamed to InterfaceDependencies.txt.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/bu...
File Source/bindings/scripts/build_ir.py (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:67: class IdlNotFoundError(Exception):

Instead of raising IdlNotFoundError, can we just return [] from read_merged_idl?
Exception gives me an impression that something unexpected happens.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:86: interfaces that the given interface
implements.

Nit: Let's put these two sentences in one paragraph. In this comment, we want to
explain one return value in one paragraph.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/co...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:37: The plan is as follows.

Shall we put this comment in idl_compiler.py since idl_compiler.py is a driver
script?

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
Source/bindings/scripts/idl_compiler.py:45:
parser.add_option('--supplemental-dependency-file')

--interface-dependency-file ?

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
Source/bindings/scripts/idl_compiler.py:75: # pylint: disable=E1111

Remove the part after "Ignore E111...". This part looks uninformative for
others.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
Source/bindings/scripts/idl_compiler.py:76: blink_ir =
build_ir.read_merged_idl(target_idl_filename,
supplemental_dependency_file=options.supplemental_dependency_file,
additional_idl_files=options.additional_idl_files)

I feel that read_merged_idl() and methods in read_merged_idl() are not well
organized. How about organizing them like this?

def main():
  options = parse_options()
  parse_tree_of_idl = parse_idl(options, ...)
  check_attributes(parse_tree_of_idl)
  parse_tree_of_interface = resolve_idl_dependency(parse_tree_of_idl, ...)
  generate_code(parse_tree_of_interface, ...)

def parse_idl():  # This can be used in other bindings like Dart.
  tokens = lex_idl(...)
  abstract_tree = build_abstract_tree(tokens)
  parse_tree = build_parse_tree(abstract_tree)
  return parse_tree

def generate_code():
  ...

 (It's up to you how the methods are split into files.)

[Nils Barth (inactive), 2013-07-10 02:38:31]

Revised per comments.

Other than comment and name changes, reorganized main call to
read the IDL (more function calls in main(), simpler functions
in read_idl.py library).
This meant a few other changes due to different logic flow.

Also added a dummy IdlDocument class;
this will be a full class hierarchy when the parser lands,
but for now we just need to return something when we read a file.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:259:
'<(SHARED_INTERMEDIATE_DIR)/supplemental_dependency.tmp',
On 2013/07/09 11:16:48, haraken wrote:
> I think this was renamed to InterfaceDependencies.txt.

Fixed in other CL; I've rebased so it's fixed here too.
(Needed to fix a few other places here too, which is done.)

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/bu...
File Source/bindings/scripts/build_ir.py (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:67: class IdlNotFoundError(Exception):
On 2013/07/09 11:16:48, haraken wrote:
> Instead of raising IdlNotFoundError, can we just return [] from
read_merged_idl?
> Exception gives me an impression that something unexpected happens.

It is a bit more explicit to return None, since this isn't actually
unexpected. Done!
(This also avoids having a long description of the error.)

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/bu...
Source/bindings/scripts/build_ir.py:86: interfaces that the given interface
implements.
On 2013/07/09 11:16:48, haraken wrote:
> 
> Nit: Let's put these two sentences in one paragraph. In this comment, we want
to
> explain one return value in one paragraph.

Done.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/co...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/co...
Source/bindings/scripts/code_generator_v8.py:37: The plan is as follows.
On 2013/07/09 11:16:48, haraken wrote:
> Shall we put this comment in idl_compiler.py since idl_compiler.py is a driver
> script?

Good point (overview in main file); done!

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
Source/bindings/scripts/idl_compiler.py:45:
parser.add_option('--supplemental-dependency-file')
On 2013/07/09 11:16:48, haraken wrote:
> --interface-dependency-file ?

Oops (>,<). Fixed (throughout, plus rebase).

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
Source/bindings/scripts/idl_compiler.py:75: # pylint: disable=E1111
On 2013/07/09 11:16:48, haraken wrote:
> Remove the part after "Ignore E111...". This part looks uninformative for
> others.

We don't need this anymore now that we return a dummy IdlDocument (from
read_idl_file)
and bool (from merge_interface_dependencies), which fixes this.

https://codereview.chromium.org/18190004/diff/8001/Source/bindings/scripts/id...
Source/bindings/scripts/idl_compiler.py:76: blink_ir =
build_ir.read_merged_idl(target_idl_filename,
supplemental_dependency_file=options.supplemental_dependency_file,
additional_idl_files=options.additional_idl_files)
On 2013/07/09 11:16:48, haraken wrote:
> I feel that read_merged_idl() and methods in read_merged_idl() are not well
> organized. How about organizing them like this?

Got it, changed; how does it look?

I wanted to have a single function that wrapped all the steps (read file,
resolve dependencies, check attributes), but admittedly it results in
a function with a huge signature.
Instead, we can have a few more lines in main(), and simpler calls.

This required a few other changes to the code:
* Dummy IdlDocument (will be full object for interface once parser lands),
  since parsing base file before resolving dependencies.
* Returning bool indicating whether bindings should be generated or not,
  since not raising exception.

(I think I can simplify this even more in future, but that requires
some GYP changes too, so I'll do it later.)


> def parse_idl():  # This can be used in other bindings like Dart.
>   tokens = lex_idl(...)
>   abstract_tree = build_abstract_tree(tokens)
>   parse_tree = build_parse_tree(abstract_tree)
>   return parse_tree

Pedantry:
"parse tree" is the CST, and comes before the AST:
the flow is CST > AST > IR:
* CST is aka "parse tree" or "parsing tree" = exactly the grammar,
* AST is aka "syntax tree".
These names are very confusing, hence why I always use "CST" and "AST".

Anyway, I've replaced all references in the code to "AST" or "IR"
with more explicit names of "node" (in AST) or "idl_document",
since the IR is an object of type IdlDocument.
I've left the generic terms "AST" and "IR" in the intro comment to read_idl.py
(which seems a better name than the cryptic "build_ir.py"),
since they're useful for an overview.
However, in the actual code more explicit names are clearer.

[haraken, 2013-07-10 04:06:50]

Thanks for the iterative improvement! I think it's almost ready to go.

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_compiler.py:37: frontend (lexer => parser =>
read_idl.py) => code_generator_v8.py

read_idl.py (lexer => parser => validater) => code_generator_v8.py ?

- Let's stop using abstract wording like "frontend". It's not clear for
newcomers what it means.

- I'd rename read_idl.py to parse_idl_file.py, but it's just a bikeshedding.

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_compiler.py:91: if not should_generate_bindings:

I'd prefer to write it as follows:

  idl_document = read_idl.read_idl_file(idl_filename,
options.interface_dependencies_file, options.additional_idl_files,
verbose=verbose)
  if not idl_document:
    ...

It seems to me that there is no benefit to expose both read_idl_file and
merge_interface_dependencies to outside of read_idl.py. What people are
interested in would be just a result after the dependency is resolved.

(I'd prefer "parse_idl.parse_idl_file" rather than "read_idl.read_idl_file", but
it's just a bike-shedding.)

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
File Source/bindings/scripts/read_idl.py (right):

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
Source/bindings/scripts/read_idl.py:81: idl_filename: filename of main IDL file

Nit: idl_filename => idl_file (for consistency with other parameter names)

BTW, how about passing an interface name to this method instead of a file name?

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
Source/bindings/scripts/read_idl.py:99: def
compute_dependency_idl_files(target_idl_basename,
interface_dependencies_filename, additional_idl_files):

compute_dependency_idl_files => compute_interface_dependency ?

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
Source/bindings/scripts/read_idl.py:118: # Window.idl Q.idl R.idl S.idl

Probably the format of the dependency file should be:

  Document P
  Event
  Window Q R S

because what we want to describe here is not dependencies of IDL files but
dependencies of interfaces.

You can fix it before or after this CL.

[haraken, 2013-07-10 04:09:28]

> Anyway, I've replaced all references in the code to "AST" or "IR"
> with more explicit names of "node" (in AST) or "idl_document",
> since the IR is an object of type IdlDocument.
> I've left the generic terms "AST" and "IR" in the intro comment to read_idl.py
> (which seems a better name than the cryptic "build_ir.py"),
> since they're useful for an overview.
> However, in the actual code more explicit names are clearer.

This is good point. We prefer explicit names rather than abstract wording which
is sometimes not clear to newcomers.

[Nils Barth (inactive), 2013-07-10 04:21:03]

Quick wording question.

On 2013/07/10 04:06:50, haraken wrote:
> frontend (lexer => parser => read_idl.py) => code_generator_v8.py
> 
> read_idl.py (lexer => parser => validater) => code_generator_v8.py ?

The steps are actually:
lex => parse => convert-ast-to-object => merge-interfaces => validate
(convert-ast-to-object is ~500 lines, and is the big step that comes after the
parser).

Should I write all of these, say as verbs as:
lex => parse => construct object => merge interfaces => validate

or as nouns like:
lexer => parser => object constructor => interface merger => validator
...or maybe simply:
lexer => parser => constructor
?


> - I'd rename read_idl.py to parse_idl_file.py, but it's just a bikeshedding.

This file is the object constructor, not the parser;
the parser is the step before (which is not being checked in yet).

The 3 files used to read an IDL file are:
* blink_idl_lexer.py
* blink_idl_parser.py
* read_idl.py

read_idl.py takes the output from the parser itself, constructs an object,
and also merges interfaces and validates.

The usual terms for what read_idl.py does would be an "object constructor"
+ "dependency importer" + "semantic analyzer", but that's really verbose.

We could call it build_idl_document.py ?

Then we'd have:
* blink_idl_lexer.py
* blink_idl_parser.py
* build_idl_document.py

How's that?

[haraken, 2013-07-10 04:40:21]

> or as nouns like:
> lexer => parser => object constructor => interface merger => validator

This sounds nicer. Nits:

  lexer => IDL lexer
  parser => IDL parser
  object constructor => Python object builder
  interface merger => interface dependency resolver
  validator => IDL syntax validator

> > - I'd rename read_idl.py to parse_idl_file.py, but it's just a bikeshedding.
> 
> This file is the object constructor, not the parser;
> the parser is the step before (which is not being checked in yet).
> 
> The 3 files used to read an IDL file are:
> * blink_idl_lexer.py
> * blink_idl_parser.py
> * read_idl.py
> 
> read_idl.py takes the output from the parser itself, constructs an object,
> and also merges interfaces and validates.
> 
> The usual terms for what read_idl.py does would be an "object constructor"
> + "dependency importer" + "semantic analyzer", but that's really verbose.
> 
> We could call it build_idl_document.py ?
> 
> Then we'd have:
> * blink_idl_lexer.py
> * blink_idl_parser.py
> * build_idl_document.py
> 
> How's that?

My point is that we could organize the entire steps as follows:

- idl_compiler
  - idl_parser (Note: "parser" might be confusing)
    - blink_idl_lexer
    - blink_idl_parser
    - python_object_builder
    - interface_dependency_resolver
  - idl_validator
  - code_generator_v8

The point is that we want to encapsulate the flow of lexer => parser => object
builder => dependency resolver into one module (i.e. idl_parser), because no one
(Blink, Dart, Extensions etc) is not interested in the intermediate state inside
the idl_parser. What do you think?

[Nils Barth (inactive), 2013-07-10 04:56:44]

On 2013/07/10 04:40:21, haraken wrote:
> > or as nouns like:
> > lexer => parser => object constructor => interface merger => validator
> 
> This sounds nicer. Nits:
> 
>   lexer => IDL lexer
>   parser => IDL parser
>   object constructor => Python object builder
>   interface merger => interface dependency resolver
>   validator => IDL syntax validator

It's actually semantic validator (it checks *values*, namely Extended Attribute
names and values);
with that change, we get:
IDL lexer => IDL parser => Python object builder => interface dependency
resolver => IDL semantic validator


> My point is that we could organize the entire steps as follows:
> 
> - idl_compiler
>   - idl_parser (Note: "parser" might be confusing)
>     - blink_idl_lexer
>     - blink_idl_parser
>     - python_object_builder
>     - interface_dependency_resolver
>   - idl_validator
>   - code_generator_v8
> 
> The point is that we want to encapsulate the flow of lexer => parser => object
> builder => dependency resolver into one module (i.e. idl_parser), because no
one
> (Blink, Dart, Extensions etc) is not interested in the intermediate state
inside
> the idl_parser. What do you think?

Agreed, we want a simple API.
"idl_parser" is confusing here, b/c that's also one part of the flow (and anyway
the name is already used by Pepper for the base module).
Currently the post-parser steps are all in one module.
I'll break out the object_builder, dependency resolution, and validation into
smaller separate modules, and have an overall module as you suggest.
Validation is really part of the "reading in" (final check), as these are
parse-time errors (bad symbol names).

So how's this:
- idl_compiler
  - idl_reader
    - blink_idl_lexer
    - blink_idl_parser
    - idl_document_builder
    - idl_dependency_resolver
    - idl_validator
  - code_generator_v8
?

[haraken, 2013-07-10 05:00:29]

> It's actually semantic validator (it checks *values*, namely Extended
Attribute
> names and values);
> with that change, we get:
> IDL lexer => IDL parser => Python object builder => interface dependency
> resolver => IDL semantic validator

Even nicer.

> So how's this:
> - idl_compiler
>   - idl_reader
>     - blink_idl_lexer
>     - blink_idl_parser
>     - idl_document_builder
>     - idl_dependency_resolver
>     - idl_validator
>   - code_generator_v8
> ?

Looks like a good structure. When integrating other customers (Extenstions, Dart
etc), we will need to reorganize the flow a bit, but we can concern about future
problems in the future :)

[Nils Barth (inactive), 2013-07-10 05:03:31]

On 2013/07/10 05:00:29, haraken wrote:
> > So how's this:
<snip>
> Looks like a good structure.

Will do!

> When integrating other customers (Extenstions, Dart etc), we will need to
> reorganize the flow a bit, but we can concern about future problems in the
future :)

Yup, tomorrow can take care of itself. Or, as it says in Matthew 6:34:
Therefore do not worry about tomorrow, for tomorrow will worry about itself.
Each day has enough trouble of its own.

Or more poetically (KJV, King James Version):
Take therefore no thought for the morrow: for the morrow shall take thought for
the things of itself. Sufficient unto the day is the evil thereof.

[Nils Barth (inactive), 2013-07-10 06:00:14]

Revised per comments.
2 "new" files are due to refactoring for clearer interface
(so not all crammed into one library).

The interface_dependencies_file format needs cleaning,
but that's involved, hence will do in future CL.

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_compiler.py:37: frontend (lexer => parser =>
read_idl.py) => code_generator_v8.py
Done, discussed in comments.

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_compiler.py:91: if not should_generate_bindings:
On 2013/07/10 04:06:50, haraken wrote:
> I'd prefer to write it as follows:
<snip>
> It seems to me that there is no benefit to expose both read_idl_file and
> merge_interface_dependencies to outside of read_idl.py. What people are
> interested in would be just a result after the dependency is resolved.

Yeah, exactly; I've change the interface around as indicated,
which makes it cleaner.

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
File Source/bindings/scripts/read_idl.py (right):

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
Source/bindings/scripts/read_idl.py:81: idl_filename: filename of main IDL file
On 2013/07/10 04:06:50, haraken wrote:
> Nit: idl_filename => idl_file (for consistency with other parameter names)

Ack, I've been using _file for the File object itself, and _filename for the
string,
but it's inconsistent. I've made it consistent.

> BTW, how about passing an interface name to this method instead of a file
name?

That's a good clean-up, and it's on my list.
The problem is that dependencies are keyed by *full path*, not interface.
We can fix this by keying these by interface, but that requires changing
several scripts.
I'll do this in a followup.

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
Source/bindings/scripts/read_idl.py:99: def
compute_dependency_idl_files(target_idl_basename,
interface_dependencies_filename, additional_idl_files):
On 2013/07/10 04:06:50, haraken wrote:
> compute_dependency_idl_files => compute_interface_dependency ?

This actually returns the file names:
since partial interfaces don't have distinct interface names,
we need the actual file names.
(See details below.)

https://codereview.chromium.org/18190004/diff/29001/Source/bindings/scripts/r...
Source/bindings/scripts/read_idl.py:118: # Window.idl Q.idl R.idl S.idl
On 2013/07/10 04:06:50, haraken wrote:
> Probably the format of the dependency file should be:
> 
>   Document P
>   Event
>   Window Q R S
> 
> because what we want to describe here is not dependencies of IDL files but
> dependencies of interfaces.

Yeah, this definitely needs cleaning up, but it's a bit messy.

The dependencies include both partial interfaces, which only have a
filename (the interface name is the same as for the main file),
*and* 'implements' statements, for which the interface name is enough,
but for which we're storing the full path.
This also is used to lookup full paths, given an interface name.
It's really a mess.

What we really want is a dictionary mapping:
interface_name -> main_idl_filename, [partial_idl_filenames],
[implemented_interface_names]

This should be generated by compute_dependencies.py,
and would make a lot of code a lot clearer, since we wouldn't
have to keep passing IDL filenames around.
The only time we actually need a filename is when reading a file,
but the full path is currently used as a key several places.

> You can fix it before or after this CL.

This requires fixing a few other scripts, so I'll do it after
(better to land this and the parser first, since those are blocking,
then do cleanup).

[haraken, 2013-07-10 06:22:47]

The final round of comments!

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_compiler.py:47: moving IDL files to the Python build
flow as the code generator is completed.

I'd remove the part from "First we will...". You can explain the plan in a bug
and add a link to the bug. (I'm sorry that the criteria for where to add how
much comments is ambiguous. Basically, we prefer to being concise rather than
verbose, since we really want to avoid the situation where comments become
out-dated.)

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_dependency_resolver.py (right):

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_dependency_resolver.py:29: """Resolve IDL
dependencies, producing a merged IdlDocument object.

This should be 'interface dependencies', not 'IDL dependencies'. You might want
to rename the file name to interface_dependency_resolver.py.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_dependency_resolver.py:31: This library computes IDL
dependencies (partial interfaces and implements),

interface dependencies

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_dependency_resolver.py:33: main IDL file, producing
an IdlDocument representing the entire interface.

IdlDocument => InterfaceDefinition
main IDL file => main interface

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_reader.py (right):

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:43: parser, which currently produces a
generic AST instead.

I'd remove this comment at this point, since none of them has implemented.

Basically, comments in this CL are a bit verbose. You don't need to add comments
to each method in the future CLs. They are very likely to be out-dated, which we
really want to avoid.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:57: class IdlDocument():

There is no concept of "IdlDocument" in the spec. You might want to rename it to
"InterfaceDefinition".

Ideally, we should support InterfaceDefinition*s*, but we don't yet support
multiple interfaces written in one IDL file. So, for now, I think we can just
assume one interface per IDL file and just use "InterfaceDefinition" here.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:61: a set of IDL fragments (i.e., the
contents of .idl files)."""

Would you add a link to the exact position of the spec?

[Nils Barth (inactive), 2013-07-10 07:04:10]

Revised:
* Rename IdlDocument to IdlDefinitions
("Document" isn't in the spec, "Definitions" is; details below)
* Trim comments some.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_compiler.py:47: moving IDL files to the Python build
flow as the code generator is completed.
On 2013/07/10 06:22:48, haraken wrote:
> I'd remove the part from "First we will...". You can explain the plan in a bug
> and add a link to the bug.

Done, at the tracker bug:
Issue 239771: Rewrite the IDL compiler in Python
http://crbug.com/239771#c37

> (I'm sorry that the criteria for where to add how much comments is ambiguous.)

No problem – I understand it's a balancing act.

I'll try to make sure time-sensitive comments ("We're going to do X.")
only occur exactly where the change will occur!

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_dependency_resolver.py (right):

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_dependency_resolver.py:29: """Resolve IDL
dependencies, producing a merged IdlDocument object.
On 2013/07/10 06:22:48, haraken wrote:
> 
> This should be 'interface dependencies', not 'IDL dependencies'. You might
want
> to rename the file name to interface_dependency_resolver.py.

Done.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_dependency_resolver.py:31: This library computes IDL
dependencies (partial interfaces and implements),
On 2013/07/10 06:22:48, haraken wrote:
> 
> interface dependencies

Done.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_dependency_resolver.py:33: main IDL file, producing
an IdlDocument representing the entire interface.
On 2013/07/10 06:22:48, haraken wrote:
> IdlDocument => InterfaceDefinition

How about IdlDefinitions, which is closer to the spec?
It's confusing to use "Interface", since this contains interfaces
(see discussion below).

> main IDL file => main interface

This actually is a filename:
it's the filename of the main IDL file for the interface
(I've reworded to make it clearer).
It would be cleaner if we could use the interface name,
but this is the "interface dependencies are keyed by *filename*"
problem that I'll clean up later.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_reader.py (right):

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:43: parser, which currently produces a
generic AST instead.
On 2013/07/10 06:22:48, haraken wrote:
> I'd remove this comment at this point, since none of them has implemented.

Got it; will add when actually implemented.

> Basically, comments in this CL are a bit verbose. You don't need to add
comments
> to each method in the future CLs. They are very likely to be out-dated, which
we
> really want to avoid.

Yeah, the comments have a mix of "overview" (top of file),
"what's not implemented yet" (but is about to), and
usual "interface/what this function does".
A bit of overview and interface documentation is helpful,
but in landing the parser itself let's make sure there's no cruft.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:57: class IdlDocument():
On 2013/07/10 06:22:48, haraken wrote:
> There is no concept of "IdlDocument" in the spec. You might want to rename it
to
> "InterfaceDefinition".

Closest to the spec would be "IdlDefinitions", as Definitions
include interfaces, callbacks, enums, dictionaries, exceptions, etc.

Currently (Perl IDL) we have an IdlDocument object (which I'm
following),
which contains lists of callback_functions, enumerations, and interfaces, so it
would be confusing to use "Interface" for the
whole object too.

We definitely need to do some renaming: "DomFunction" is nowhere
in the *IDL* spec!


> Ideally, we should support InterfaceDefinition*s*, but we don't yet support
> multiple interfaces written in one IDL file. So, for now, I think we can just
> assume one interface per IDL file and just use "InterfaceDefinition" here.

We only have one interface, but callbacks and enums are also definitions,
so it is accurate to call it "IdlDefinition*s*".

Also, we probably also need to read parent interfaces and include them here,
so the code generator can refer to the parent interfaces without
having to call the parser itself!
(The Perl code generator reads parent interfaces.)

So even though we only have one interface per file,
we need to read several other interfaces to generate the code
for a single interface.

https://codereview.chromium.org/18190004/diff/55001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:61: a set of IDL fragments (i.e., the
contents of .idl files)."""
On 2013/07/10 06:22:48, haraken wrote:
> 
> Would you add a link to the exact position of the spec?

Done.

[haraken, 2013-07-10 07:13:45]

LGTM. This is a good start of the new Python flow!

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_reader.py (right):

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:43: class IdlDefinitions():

Your comment makes sense. IdlDefinitions sounds better.

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:54: def read_idl_interface(idl_filename,
interface_dependencies_filename, additional_idl_filenames):

You might want to rename it to read_idl_definitions.

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:55: """Returns an IdlDefinitions object
for an IDL interface, including all dependencies."""

for an IDL interface => for a given IDL file

As I commented before, we've been confused with concepts of IDL interfaces, IDL
definitions and IDL files (e.g. we should list not IDL files but IDL interfaces
in InterfaceDependencies.txt). Let's clean up all of these confusions in a
follow-up CL.

[Nils Barth (inactive), 2013-07-10 07:35:36]

Thanks for patient review; final revision, committing.
(On to the parser!)

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
File Source/bindings/scripts/idl_reader.py (right):

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:54: def read_idl_interface(idl_filename,
interface_dependencies_filename, additional_idl_filenames):
On 2013/07/10 07:13:46, haraken wrote:
> You might want to rename it to read_idl_definitions.

Good point, done.

https://codereview.chromium.org/18190004/diff/61001/Source/bindings/scripts/i...
Source/bindings/scripts/idl_reader.py:55: """Returns an IdlDefinitions object
for an IDL interface, including all dependencies."""
On 2013/07/10 07:13:46, haraken wrote:
> for an IDL interface => for a given IDL file

Done.

> As I commented before, we've been confused with concepts of IDL interfaces,
IDL
> definitions and IDL files (e.g. we should list not IDL files but IDL
interfaces
> in InterfaceDependencies.txt). Let's clean up all of these confusions in a
> follow-up CL.

Yeah, there's a lot of confusion, old hacks, and cruft.
Will fix!

[commit-bot: I haz the power, 2013-07-10 07:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/18190004/64001

[commit-bot: I haz the power, 2013-07-10 07:36:37]

Failed to apply patch for Tools/Scripts/webkitpy/bindings/main.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Tools/Scripts/webkitpy/bindings/main.py
  Hunk #1 FAILED at 52.
  Hunk #2 FAILED at 75.
  Hunk #3 FAILED at 85.
  Hunk #4 FAILED at 141.
  Hunk #5 FAILED at 160.
  Hunk #6 FAILED at 185.
  Hunk #7 FAILED at 196.
  7 out of 7 hunks FAILED -- saving rejects to file
Tools/Scripts/webkitpy/bindings/main.py.rej

Patch:       Tools/Scripts/webkitpy/bindings/main.py
Index: Tools/Scripts/webkitpy/bindings/main.py
diff --git a/Tools/Scripts/webkitpy/bindings/main.py
b/Tools/Scripts/webkitpy/bindings/main.py
index
52959e73659ffa40058ca88cf8bb5ad919c9c405..1e7519ab3125fd95b4c25054203a778e0eee0f46
100644
--- a/Tools/Scripts/webkitpy/bindings/main.py
+++ b/Tools/Scripts/webkitpy/bindings/main.py
@@ -52,16 +52,16 @@ class BindingsTests:
         self.reset_results = reset_results
         self.executive = executive
 
-    def generate_from_idl(self, idl_file, output_directory,
supplemental_dependency_file):
+    def generate_from_idl(self, idl_file, output_directory,
interface_dependencies_file):
         cmd = ['perl', '-w',
                '-Ibindings/scripts',
                '-Icore/scripts',
                '-I../../JSON/out/lib/perl5',
-               'bindings/scripts/generate-bindings.pl',
+               'bindings/scripts/deprecated_generate_bindings.pl',
                # idl include directories (path relative to
generate-bindings.pl)
                '--include', '.',
                '--outputDir', output_directory,
-               '--supplementalDependencyFile', supplemental_dependency_file,
+               '--interfaceDependenciesFile', interface_dependencies_file,
                '--idlAttributesFile', 'bindings/scripts/IDLAttributes.txt',
                idl_file]
 
@@ -75,7 +75,7 @@ class BindingsTests:
             exit_code = e.exit_code
         return exit_code
 
-    def generate_supplemental_dependency(self, input_directory,
supplemental_dependency_file, window_constructors_file,
workerglobalscope_constructors_file, sharedworkerglobalscope_constructors_file,
dedicatedworkerglobalscope_constructors_file, event_names_file):
+    def generate_interface_dependencies(self, input_directory,
interface_dependencies_file, window_constructors_file,
workerglobalscope_constructors_file, sharedworkerglobalscope_constructors_file,
dedicatedworkerglobalscope_constructors_file, event_names_file):
         idl_files_list = tempfile.mkstemp()
         for input_file in os.listdir(input_directory):
             (name, extension) = os.path.splitext(input_file)
@@ -85,9 +85,9 @@ class BindingsTests:
         os.close(idl_files_list[0])
 
         cmd = ['python',
-               'bindings/scripts/preprocess_idls.py',
+               'bindings/scripts/compute_dependencies.py',
                '--idl-files-list', idl_files_list[1],
-               '--supplemental-dependency-file', supplemental_dependency_file,
+               '--interface-dependencies-file', interface_dependencies_file,
                '--window-constructors-file', window_constructors_file,
                '--workerglobalscope-constructors-file',
workerglobalscope_constructors_file,
                '--sharedworkerglobalscope-constructors-file',
sharedworkerglobalscope_constructors_file,
@@ -141,7 +141,7 @@ class BindingsTests:
                 changes_found = True
         return changes_found
 
-    def run_tests(self, input_directory, reference_directory,
supplemental_dependency_file, event_names_file):
+    def run_tests(self, input_directory, reference_directory,
interface_dependencies_file, event_names_file):
         work_directory = reference_directory
 
         passed = True
@@ -160,7 +160,7 @@ class BindingsTests:
 
             if self.generate_from_idl(os.path.join(input_directory,
input_file),
                                       work_directory,
-                                      supplemental_dependency_file):
+                                      interface_dependencies_file):
                 passed = False
 
             if self.reset_results:
@@ -185,7 +185,7 @@ class BindingsTests:
         input_directory = os.path.join('bindings', 'tests', 'idls')
         reference_directory = os.path.join('bindings', 'tests', 'results')
 
-        supplemental_dependency_file = provider.newtempfile()
+        interface_dependencies_file = provider.newtempfile()
         window_constructors_file = provider.newtempfile()
         workerglobalscope_constructors_file = provider.newtempfile()
         sharedworkerglobalscope_constructors_file = provider.newtempfile()
@@ -196,11 +196,11 @@ class BindingsTests:
         else:
             event_names_file = provider.newtempfile()
 
-        if self.generate_supplemental_dependency(input_directory,
supplemental_dependency_file, window_constructors_file,
workerglobalscope_constructors_file, sharedworkerglobalscope_constructors_file,
dedicatedworkerglobalscope_constructors_file, event_names_file):
-            print 'Failed to generate a supplemental dependency file.'
+        if self.generate_interface_dependencies(input_directory,
interface_dependencies_file, window_constructors_file,
workerglobalscope_constructors_file, sharedworkerglobalscope_constructors_file,
dedicatedworkerglobalscope_constructors_file, event_names_file):
+            print 'Failed to generate interface dependencies file.'
             return -1
 
-        if not self.run_tests(input_directory, reference_directory,
supplemental_dependency_file, event_names_file):
+        if not self.run_tests(input_directory, reference_directory,
interface_dependencies_file, event_names_file):
             all_tests_passed = False
 
         print ''

[commit-bot: I haz the power, 2013-07-10 07:43:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/18190004/69001

[commit-bot: I haz the power, 2013-07-10 10:18:34]

Change committed as 153878