Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15801003:
IDL parser rewrite in Python (Closed)
|
Created:
7 years, 6 months ago by Nils Barth (inactive) Modified:
7 years, 5 months ago CC:
blink-reviews, caseq+blink_chromium.org, aandrey+blink_chromium.org, loislo+blink_chromium.org, jsbell+bindings_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, Rik, apavlov+blink_chromium.org, adamk+blink_chromium.org, Nate Chapin, kojih, dominicc (has gone to gerrit) Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionIDL parser rewrite in Python
The parser etc. steps are:
1. lexer (blink_idl_lexer.py)
2. parser (blink_idl_parser.py), yields AST
3. build Python object (idl_definitions_builder.py)
The lexer is very simple (just tweaks base Pepper lexer).
The parser is just grammar. It is in BNF + PLY's dialect of yacc,
so the syntax looks a bit unfamiliar, but I've got detailed comments
to explain how to read it.
Most of the logic is in idl_definitions_builder.py, which converts
the AST to a Python object. It's straightforward but long.
idl_definitions.py is mostly JSON export (for verification),
which has several Perl hacks to produce identical output.
There's also the extended attribute validation in idl_validator.py.
There is a bit of testing cruft in derived_sources.gyp and idl_compiler
(to make JSON and pickles), which I'll take out before landing.
For reference, the base (Pepper) IDL lexer + parser are:
src/tools/idl_parser/idl_lexer.py
src/tools/idl_parser/idl_parser.py
https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/idl_lexer.py
https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/idl_parser.py
BUG=242795
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154633
Patch Set 1 #Patch Set 2 : [WIP] Full parser #Patch Set 3 : [WIP] Full parser #
Total comments: 14
Patch Set 4 : [WIP] (Reference for build changes) #Patch Set 5 : WIP (pre-rebase) #Patch Set 6 : Ready for review! #Patch Set 7 : Ready for review! (cleaner) #
Total comments: 153
Patch Set 8 : Revised. #Patch Set 9 : Revised. #
Total comments: 88
Patch Set 10 : Final #Patch Set 11 : Final (rebased). #
Total comments: 2
Messages
Total messages: 35 (0 generated)
Hi Haraken, Here's a working lexer + parser, with tests. It should be easy to work incrementally from here to full frontend (make IR) and compiler! This CL breaks up as: * lexer * parser * syntax error fixes (landing in separate CLs, will disappear when rebase) For review and landing it might be easier to split up as lexer in one, and then parser in the other, though we can do them both in one go. The parser is pretty long (470 lines), but it's almost all grammar. The routines starting 'p_' are (actions for) production rules for the grammar. The comments are the BNF rule, and are used by the parser to build the parse tree in yacc; this is literally the Blink IDL spec. The function bodies are the yacc actions, basically "build the AST as you walk the parse tree". The actions will need work to produce the correct AST, but the grammar itself is complete, and good to go!
Can we remove the use of the preprocessor from the IDL files? It seems like we should be able to use [Conditional] attributes instead.
How does the performance of the new parser compare to the old parser?
On 2013/05/28 06:32:33, abarth wrote: > Can we remove the use of the preprocessor from the IDL files? It seems like we > should be able to use [Conditional] attributes instead. Sounds good, assuming that's possible! For now we just want to rewrite in Python, using existing flow (with preprocessor), but once we're in Python I'll look into removing the preprocessor as you suggest.
Makes sense. :)
On 2013/05/28 06:33:54, abarth wrote: > How does the performance of the new parser compare to the old parser? I haven't done real profiling, but probably 3-4x faster, and easy to make extremely fast if we rewrite either part in straight C lex or yacc (or both), which is possible now that it's modular. Here's how long the tests (that lex and parse all 600+ IDL files) take: lexing 0.4 seconds lex+parse 1.0 seconds (1.1 seconds first run, 1.0 on subsequent, because table already generated, so .1 for table generation + .6 for parse) There's no equivalent test for the Perl, but it looks like it takes about 3-4 seconds. Commenting out the code generation and running: touch generate-bindings.pl cd "$CHROMIUM_DIR" && time ninja -C out/Release chrome ...is a bit over 5 seconds, while empty run is .8 seconds (overhead). Stop watch (when build is showing bindings) is about 3.5-4 seconds; there's a bit of preprocessor overhead that's included, and at the end there's (dynamic) linking overhead, so best guess is 3-4 seconds in Perl. Re: C rewrite Per this talk: http://www.dabeaz.com/ply/PLYTalk.pdf ...C can be expected to be up to 50x faster; rewriting just the lexer would be pretty easy and give another about 2x speedup, while parser would be more work but should be straightforward, and would make the lex+parse stage virtually instant; the filesystem and backend code generation would be bottleneck.
That sounds great. :)
Great work! I'm happy to review the details of the patch once high-level issues are resolved. How are you going to land this? We have to somehow ensure that the output of the new IDL parser is exactly the same as the previous one. The idea I have in mind is: (1) Connect IDLParser.pm and CodeGeneratorV8.pm via a JSON file. The JSON file stores parsing trees. (2) Implement blink_idl_parser.py so that the output becomes exactly the same as the JSON file. (3) Swtich IDLParser.pm to blink_idl_parser.py. I'm curious about what you have in mind.
In addition, I'm OK with landing the lexer and the parser for all IDL files at a breath. I don't think it's too big; it's easy to verify correctness. (On the other hand, we want to rewrite CodeGeneratorV8.pm incrementally though.)
I was thinking of moving the incrementalism one level up, and doing IR
incrementally as well;
this will let Koji and I work in parallel (backend and frontend) instead of
blocking on frontend.
With this CL the grammar is complete (parse tree), but the IR (AST/symbol table)
isn't, so:
1. Land this CL (grammar complete, with unit tests)
2. Rewrite generate_bindings.py and stub code_generator_v8.py
Now we can work on the code generator, and Koji is unblocked!
3. Incrementally complete the IR and the code generator in parallel
To the question:
"We have to somehow ensure that the output of the new IDL parser is exactly the
same as the previous one."
We can compare at three stages:
* at the parse tree level,
* at the IR level (via JSON),
* at the ultimate output (.cpp and .h).
The parse tree (concrete syntax tree) level is done in this CL: it parses all
the IDLs, based on the BNF that Sakamoto used to generate the Perl parser (with
tweaks for changes since then), so the internal parse tree is correct -- they
both walk the trees for all files.
To check the IR level (via JSON), IDLParser.pm is a library, so we can just call
it and dump JSON, then call blink_idl_parser.py and dump JSON and compare these.
To check the output, we just check the output files. (^.-)
We can do this on a file-by-file basis, so we can say
"parse tree agrees for 637/637 files, IR agrees for 50/637 files, .cpp/.h agrees
for 20/637 files".
For the stub code_generator_v8.py, we can land it when it produces identical
output .cpp/.h to the existing Perl for *one* file, e.g.,
core/html/TextMetrics.idl, which is:
interface TextMetrics {
readonly attribute float width;
}
At this point Python is working end-to-end, and we can work on backend!
We don't need to have all the IR done to start on the backend -- so long there
are enough files that we can generate the full IR for, it's not blocking code
generation.
Regarding switching to the Python flow:
I think it's easier (and less work) to just have two separate Python and Perl
pipelines, and just feed them separate file lists:
generate-bindings.pl (+ IDLParser.pm and CodeGeneratorV8.pm)
generate_bindings.py (+ blink_idl_parser.py and code_generator_v8.py)
...and just feed individual IDL files into one or the other (Perl for now,
Python once it's ready).
This adds a build rule and a file saying "which is which", but otherwise is very
lightweight.
The Perl scripts work, so we can keep them around until the end, when they're
completely ready for replacement.
However, the frontend IR should get finished well before the backend code
generation, so *at that point* we can switch the parser to Python if we want to.
I don't think we should block on this though: getting the frontend IR perfect
will take a while (e.g., getting SVG right).
Instead of 2 separate pipelines, we could do various hybrids:
remove generate-bindings.pl and instead have the Python generate_bindings.py
call Perl and communicate directly with JSON, then switch the parser to Python
once that's complete, or create intermediate files and break up
generate-bindings.pl into two scripts.
However, this adds complexity, and slows down the build, which people are
already complaining about.
Also, it requires *another* process -- currently generate-bindings.pl uses a
single process to parse and code generate, and another process for the
preprocessor -- doing just parsing in Python, or calling Perl from Python
requires a separate process, either run before or at the same time.
So I think we should land the grammar in one go (this CL),
then do generate_bindings.py and start of code_generator_v8.py in another go,
and then incrementally in parallel complete parser so IR exactly the same (using
JSON dump to check), and code generator so .cpp/.h exactly the same.
Once parser is 100% complete (at IR level), then we can actually try swapping
them and seeing how it goes.
How does this sound?
Agreed re: landing lexer and parser together:
other than the actions (which are a bit dense and hairy), it's very
straightforward, since it's just grammar.
For example, I can read:
Interface : INTERFACE identifier Inheritance '{' InterfaceMembers '}' ';'
CallbackRest : identifier '=' ReturnType '(' ArgumentList ')' ';'
...b/c they are just BNF production rules.
...but reading the actions (basically attribute equations):
p[0] = self.BuildNamed('Interface', p, 2, ListFromConcat(p[3], p[5]))
or:
arguments = self.BuildProduction('Arguments', p, 4, p[5])
p[0] = self.BuildNamed('Callback', p, 1, ListFromConcat(p[3], arguments))
...takes a bit more thinking. (>.<)
So checking the IRs for all 637 IDL files will require lots of care, and I think
should also be done incrementally.
Basically I don't want to introduce unnecessary complexity. (e.g. I don't want to consider IR-level correctness, internal-structure-level correctness etc.) I agree with the two-pipeline approach if it is hard to land the lexer and parser at a breath, but I don't think so. - To unblock koji-san's work, it is enough if the parser and the code generator are connected via a JSON file. You can use the same IR as the current parsing tree structure in IDLParser.pm. (Otherwise, you have to change a lot of things in the deprecated CodeGeneratorV8.pm). Once they are connected via a JSON file, koji-san and you can work in parallel. - You don't need to try to optimize/change the IR format until everything is done. No one will be interested in the intermediate format. My questions are: - How hard is it to connect IDLParser.pm and CodeGeneratorV8.pm via a JSON file? - Once the JSON file is supported, how hard would it be to land the lexer and the parser that outputs exactly the same JSON file at a breath?
OIC -- what you're suggesting is the other way around, so we start work on backend immediately. I.e., Perl *parser* and new Python backend! This works well, and the framework is easy -- I'll do it tomorrow (in a rewritten generate_bindings.py). We don't need to change the existing Perl; we can just call IDLParser.pm and dump JSON, then read it into Python. This will let us start working on backend soon (end of week?)! However, we won't be able to start using Python backend as part of normal build until we get rid of JSON, because old Perl doesn't support JSON without an extra package. To answer your questions: - How hard is it to connect IDLParser.pm and CodeGeneratorV8.pm via a JSON file? Technically no problem -- it's easy to write and read a JSON file (see below). However, it would slow down the build (b/c we'd double the processes), and break the build on Mac and Windows, since JSON.pm isn't standard on old Perl, and we don't currently use it: cd "$CHROMIUM_DIR" && find . -name '*.py' | xargs grep '^use JSON' (We can add a dependency, but I don't know how well this would go over.) Once everything's in Python, it is ok to use JSON, since it's standard in Python 2.6. However, I'm not sure what that buys us. - how hard would it be to land the lexer and the parser that outputs exactly the same JSON file That will probably take weeks; best guess is 2 weeks. That's why I'm suggesting landing this now -- this CL is the syntax, getting the IR right is semantics (action steps in AST) and translating to legacy format (Perl IR). There are two steps: - convert the Pepper AST (for Web IDL) to the existing Perl-based IR. - verify semantics (action steps -- the bodies of the p_ functions) for all the WebKit IDL production rules. These are both straightforward but tedious. Once this is done the Python code can natively call the Python parser instead of calling Perl (or we can generate JSON and then immediately decode it, or serialize "pickle" it, or whatever). Reference (JSON code) Here's code to dump, the Perl IR as JSON, using JSON.pm use JSON; use IDLParser; ... my $targetParser = IDLParser->new(); my $targetDocument = $targetParser->Parse($targetIdlFile); my $json = encode_json $targetDocument; Here's Python code to read JSON: import json python_ir = json.loads(perl_ir_as_json)
I have made some comments. I believe the same comments are applicable in multiple places in many cases. In particular, your comments could be improved. Please address them everywhere and let me know when I should take another look. If something is unclear please ask for clarification before speculatively doing a lot of work. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... File Source/bindings/scripts/ir.py (right): https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:1: # Copyright (C) 2013 Google Inc. All rights reserved. Your comments need work. Fragments do not work well for you. Try writing sentences. Try to write comments for someone who is very smart, very busy, but isn't familiar with this area of the codebase. Try to predict what they need to know and give them some clues about how each part relates to the whole. Be brief, not cryptic. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:29: """Blink IDL Intermediate Representation (IR) classes End the first line of a docstring comment with a period (or other appropriate terminal punctuation.) https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:51: def to_json(self): Is there a better name for this? It doesn't return JSON per se. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:52: """Return a serializable form of the object. Return -> Returns https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:54: Compatible with Perl IR and JSON import. Wrap text; separate paragraphs with blank lines. You have this random line break at the end of this sentence. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:54: Compatible with Perl IR and JSON import. This documentation needs to be improved. "Compatible with Perl IR and JSON import." isn't really a sentence, it is a fragment. It is confusing. Just say what you mean in plain language. You might consider including a path to the definition on the Perl side so people have a clue to what they need to keep in sync if they're changing this. Don't make me guess what "JSON import" is. You seem to be saying this method produces JSON, which is compatible with... JSON. The first rule of tautology club is the first rule of tautology club. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:56: pass It would be better to raise an exception here. If execution reaches here, it is a bug in the derived class, right? https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:59: class TypedIdlObject(BaseIdl): If this class handles typedefs, maybe it should have typedef in the name. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:60: """Auxiliary class for handling typedefs.""" Auxiliary is a meaningless word without more context. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:62: extended_attributes = {} Won't this hash object be shared by all instances of this class?
Thanks for feedback; will fix comments (and other concerns) and ping when done. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... File Source/bindings/scripts/ir.py (right): https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:1: # Copyright (C) 2013 Google Inc. All rights reserved. On 2013/06/26 04:20:53, dominicc wrote: > Your comments need work. Will do. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:51: def to_json(self): On 2013/06/26 04:20:53, dominicc wrote: > Is there a better name for this? It doesn't return JSON per se. "json_serializable" is more precise; will change. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:56: pass On 2013/06/26 04:20:53, dominicc wrote: > It would be better to raise an exception here. If execution reaches here, it is > a bug in the derived class, right? The ABC (Abstract Base Class) library handles this, and even better. The decorator makes this an abstract method, so an exception is raised at object instantiation if it is unimplemented, rather than at method invocation. https://codereview.chromium.org/15801003/diff/17001/Source/bindings/scripts/i... Source/bindings/scripts/ir.py:62: extended_attributes = {} On 2013/06/26 04:20:53, dominicc wrote: > Won't this hash object be shared by all instances of this class? (Just added this to silence lint errors; probably better to just disable the warnings.) This should work, as I assign to instance attributes, so the class attributes never get used. These attributes actually are supposed to be abstract attributes, though I'm not sure the syntax for them (they can be omitted from the declaration; this is just for clarity).
Hi haraken, Here's the parser rewrite CL! It's a bit long, so I've given an outline in the description; hopefully it's clear enough once you can see the code.
The overall structure looks OK. Exciting CL! Let me take a close look later (or tomorrow morning).
This is a great patch!! The overall structure looks good. Below comments are just minor issues. I'll comment on idl_definitions.py and idl_definitions_builder.py tomorrow morning. (Note: The comments that start from "I'm just curious:" are comments just to help me understand your code. I'm not asking you to fix something.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:240: #}, Remove all changes for testing before landing. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:322: # FIXME: testing, so dump JSON I'm OK with leaving this option for debugging purpose until you finish migrating everything to the Python flow. However, more detailed comment please. e.g.: # FIXME: This option is only for debugging and will be removed once we complete migrating all IDL files from the Perl flow to the Python flow. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_lexer.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:35: Blink IDL is identical to Web IDL at the token level, but the base lexer Let's add "FIXME:". https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:51: # pylint: disable=E1101 Is this comment helpful? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:59: sys.path.append(tools_dir) Instead of writing the relative path here, can you pass the path from a GYP file? We want to centralize path handling in GYP files. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:68: # FIXME: Upstream; needed because base doesn't ignore comments You can remove this comment, because you already said this above. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:71: self.AddLines(t.value.count('\n')) I'm just curious: What is this doing? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:75: # FIXME: Upstream I'd remove this comment. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:80: raise RuntimeError('Token "%s" not present, so cannot be removed ' % token) Does this mean that we raise an exception when parsing an IDL file without any comment? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:52: # pylint: disable=E1101 Is this comment helpful? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:60: sys.path.append(third_party) Instead of writing the relative path here, let's pass the path from GYP file. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:65: sys.path.append(tools_dir) Ditto. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:66: # Don't change case of ListFromConcat, for consistency with base parser Nit: I'd remove this comment. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:68: # Change function name, due to different Chromium/Blink convention Ditto. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:69: from idl_parser.idl_parser import ParseFile as parse_file Looks like parse_file is unused. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:87: # Review of yacc: Great summary! This is the easiest yacc tutorial I've ever seen :) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:129: # # Build named node of type NodeType, with name and value p[1]. I'm just curious: What's the difference between a node and a named node? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:151: # You might want to add an explanation for self.BuildAttribute(). https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:156: # [b1.1] for Blink IDL additions, auxiliary rules for [b1] The [X] numbering is fragile. As far as I see the history of the Web IDL spec, it's very likely to change when the spec is updated. You can use the left side name instead. (e.g. [ExtendedAttributes (Should upstream)], [ExtendedAttributes (Blink specific)]) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:214: p[0] = p[2] Don't you need to add p[2].AddChildren(self.BuildTrue('STRINGIFILER')) ? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:240: # [NotEnumerable] DOMString toString(); Why do we need to handle toString() specially. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:244: # [b49] Override base parser: remove comment field, since comments stripped I'm just curious: Would you elaborate on why we need to override the rule for ExtendedAttributeList? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:253: p[0] = ListFromConcat(p[0], attribs) I'm just curious: Why isn't this "p[0] = attribs" ? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:255: # [b50] Allow optional trailing comma Haven't you already removed all trailing commas from Blink IDLs? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:307: if len(p) == 3: Nit: elif https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:315: p[0] = self.BuildNamed('ExtAttribute', p, 1, value) I'm just curious: Where is the identifier of p[1] added to CST? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_compiler.py:44: import pickle Nit: alphabetical order please. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_compiler.py:80: def write_json_and_pickle(definitions, interface_name, output_directory): I'm OK with leaving this option until you finish moving everything. But please add a FIXME about it. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_compiler.py:86: # Pickle export (for Koji) Nit: Remove this comment. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:37: # pylint: disable=W0232, E0203, W0201 Is this comment helpful? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_reader.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_reader.py:44: should_generate_bindings = interface_dependency_resolver.merge_dependencies(definitions, idl_filename, interface_dependencies_filename, additional_idl_filenames, verbose=verbose) Nit: merge_dependencies => resolve_dependencies ?
Comment part 2: https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:31: Also JSON export, using legacy Perl terms and format. Given that we decided to keep deprecated_idl_parser.pm (for performance reasons), there is no need to provide JSON from the new Python flow to the old Perl flow. Thus you can completely remove idl_definitions.py once you verify that the new Python parser generates exactly the same output as the old Perl IDL parser. Is my understanding correct? If my understanding is correct, please update the comment accordintly. (BTW, I'm very sorry that you had to spent time introducing JSON.pm, which was not used in the end.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:29: """Read an IDL file or complete IDL interface, producing an IdlDefinitions object.""" Slightly better: "Build an IdlDefinitions object from AST (which is built by idl_parser)". https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:36: def file_node_to_idl_definitions(node): For readability, I might want to add one more helper method which is exported to idl_compiler.py. def build_idl_definitions_from_ast(ast): file_node_to_idl_definitions(ast) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:67: pass Help me understand: Why can we skip 'Implements'? Even if we resolve the dependency in the later phase, I guess we need to include the information of 'Implements' to IdlDefinitions. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:71: return IdlDefinitions(callback_functions=callback_functions, enumerations=enumerations, exceptions=exceptions, file_name=file_name, interfaces=interfaces, typedefs=typedefs) Nit: One unnecessary space before file_name. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:94: clear_constructor_attributes(attribute.extended_attributes) Would you add the following FIXME? FIXME: This is a hack to support [CustomConstructor] for window.HTMLImageElement. Remove the hack. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:99: extended_attributes = ext_attributes_node_to_extended_attributes(child) Don't you need to call clear_constructor_attributes(extended_attributes) ? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:105: elif child_class == 'ExceptionFieldToString': I don't fully understand why this is needed. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:139: num_children = len(children) Shall we check that num_children > 1 ? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:169: for special_keyword in SPECIAL_KEYWORD_LIST: Why haven't the special keywords parsed in the parser? It seems a bit strange to me that SPECIAL_KEYWORD_LIST appears not in blink_idl_parser but in idl_definitions_buider.py. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:191: def arguments_node_to_arguments(arguments_node): Nit: arguments_node => node (for consistency) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:192: if arguments_node is None: Is this check needed? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:207: # Make all default to False once Perl removed. You can remove this comment, as the same thing is written below. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:264: children = node.GetChildren() Shall we check that len(children) > 0 ? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:342: # However, Named Constructors cannot be overloaded, and thus do not have Actually this should be a FIXME. We want to support overloading for named constructors and remove custom bindings for HTMLImageElement. Given that it looks straightforward to support the overloading in your beautiful parser, I hope you'll fix it in a follow-up CL :) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:406: extended_attributes['Constructor'] = None You can use clear_constructor_attributes(). https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:409: # FIXME: I have no idea why this is necessary. Yeah, I don't understand why either. We shouldn't use [CallWith] and [RaisesException] in constuctors and should just use [ConstructorCallWith] and [ConstructorRaisesException]. Then we no longer need to handle these attributes specially. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:428: extended_attributes['CustomConstructor'] = None You can use clear_constructor_attributes(). https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:435: arguments_node = call_node.GetChildren()[0] Do you want to check that len(children) > 0 ? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:445: del extended_attributes['Constructors'] This del won't be needed, because you're setting None in the next line. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:448: del extended_attributes['CustomConstructors'] Ditto. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:457: if not children: Instead, you can check that len(children) > 0 https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:476: """Auxiliary.""" I'd remove this comment. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:45: return You might want to output an error for this case. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:78: # FIXME: this should be done when parsing the file, rather than after. I think postmortem processing sounds reasonable. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:111: if value not in valid_values: Don't you need to do value.strip() ?
Comment part 3. That's it from me. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:51: __metaclass__ = abc.ABCMeta I'm a bit confused with the comments above. You can remove json_serializable() when removing the Perl flow, but you will still need the classes, __init__'s and apply_typedefs()'s. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:65: class TypedObject(BaseIdl): Actually I don't fully understand what class should inherit BaseIdl and what class should inherit TypedObject. What's the difference? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:67: __metaclass__ = abc.ABCMeta This wouldn't be needed. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:87: raise ValueError('Extended attributes in a typedef are untested!') This will happen in bindings/tests/idls/TestTypedefs.idl, so you can test it. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:94: def __init__(self, callback_functions=None, enumerations=None, exceptions=None, file_name=None, interfaces=None, typedefs=None): It looks like you're explicitly specifying all parameters in the caller side. Would it make sense to get rid of default parameters? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:102: self.apply_typedefs(typedefs) Help me understand: Why do you need to call apply_typedefs() here? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:105: for callback_function in self.callback_functions: callback_functions.itervalues(). The same comment for other parts. You can add .itervalues() here and there. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:374: Shall we use if-else? if type_string.endwith('[]'): is_array = False ... else: is_sequense = False ... https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:446: extended_attributes['Constructor'] = None If you set None, I'm afraid that we cannot distinguish whether [Constructor] is specified or not. To avoid the problem, the current Perl flow uses "VALUE_IS_MISSING" for this case. How can the new Python flow distinguish it?
Thanks for the thoughtful reviews and kind words! Here are the “minor replies” (yup, done, etc.); I'll follow with more substantive ones individually. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:240: #}, On 2013/07/16 14:17:51, haraken wrote: > Remove all changes for testing before landing. Will do! I’ve added FIXMEs, and it’ll be caught by the try bots, but could you please hold off the ok until these are removed? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:322: # FIXME: testing, so dump JSON On 2013/07/16 14:17:51, haraken wrote: > I'm OK with leaving this option for debugging purpose until you finish migrating > everything to the Python flow. > > However, more detailed comment please. Ok, this makes it easier. (Put comment in idl_compiler.py) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_lexer.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:35: Blink IDL is identical to Web IDL at the token level, but the base lexer On 2013/07/16 14:17:51, haraken wrote: > Let's add "FIXME:". Ok, consolidated FIXME at top. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:51: # pylint: disable=E1101 On 2013/07/16 14:17:51, haraken wrote: > Is this comment helpful? Yup, otherwise I get a pylint error, I think on presubmit. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:59: sys.path.append(tools_dir) On 2013/07/16 14:17:51, haraken wrote: > Instead of writing the relative path here, can you pass the path from a GYP > file? We want to centralize path handling in GYP files. (See long reply.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:68: # FIXME: Upstream; needed because base doesn't ignore comments On 2013/07/16 14:17:51, haraken wrote: > You can remove this comment, because you already said this above. Got it, consolidate FIXMEs at top. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:71: self.AddLines(t.value.count('\n')) On 2013/07/16 14:17:51, haraken wrote: > I'm just curious: What is this doing? This tracks the current line number, which is used for error reporting. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:75: # FIXME: Upstream On 2013/07/16 14:17:51, haraken wrote: > I'd remove this comment. Got it, consolidated FIXME. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:80: raise RuntimeError('Token "%s" not present, so cannot be removed ' % token) On 2013/07/16 14:17:51, haraken wrote: > Does this mean that we raise an exception when parsing an IDL file without any > comment? No no, this is used when initializing the lexer, not running it. It means if you try to remove a token class that doesn’t exist, you’ll get an error. E.g., if you say _RemoveToken(‘GREATEREQUAL’), you’ll get an error (b/c >= isn’t a token in Web IDL). https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:52: # pylint: disable=E1101 On 2013/07/16 14:17:51, haraken wrote: > Is this comment helpful? (As above.) Yes, quiets pylint error. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:60: sys.path.append(third_party) On 2013/07/16 14:17:51, haraken wrote: > Instead of writing the relative path here, let's pass the path from GYP file. (See separate response.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:65: sys.path.append(tools_dir) On 2013/07/16 14:17:51, haraken wrote: > Ditto. (Ditto.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:66: # Don't change case of ListFromConcat, for consistency with base parser On 2013/07/16 14:17:51, haraken wrote: > > Nit: I'd remove this comment. Done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:68: # Change function name, due to different Chromium/Blink convention On 2013/07/16 14:17:51, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:69: from idl_parser.idl_parser import ParseFile as parse_file On 2013/07/16 14:17:51, haraken wrote: > Looks like parse_file is unused. parse_file is used in idl_reader (it’s a simple utility function). I’m importing it here so idl_reader only needs to import blink_idl_parser, not the base IDL parser library too. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:87: # Review of yacc: On 2013/07/16 14:17:51, haraken wrote: > Great summary! This is the easiest yacc tutorial I've ever seen :) Thanks! (#^_^#) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:129: # # Build named node of type NodeType, with name and value p[1]. On 2013/07/16 14:17:51, haraken wrote: > I'm just curious: What's the difference between a node and a named node? A named node also has the attribute ‘NAME’ set; that’s the only difference. I've added comments on the helper functions. See: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/generators/i... https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:151: # On 2013/07/16 14:17:51, haraken wrote: > You might want to add an explanation for self.BuildAttribute(). Got it, done. (Also BuildTrue.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:156: # [b1.1] for Blink IDL additions, auxiliary rules for [b1] On 2013/07/16 14:17:51, haraken wrote: > The [X] numbering is fragile. As far as I see the history of the Web IDL spec, > it's very likely to change when the spec is updated. This is for consistency with the base Pepper parser. Also the spec is now at the Candidate Recommendation stage, so it shouldn’t change much, and hasn’t changed in over a year. Anyway we’ll need to revise the parser when the spec changes. However, your point is well taken; I’ve added a link to the specific revision. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:214: p[0] = p[2] On 2013/07/16 14:17:51, haraken wrote: > Don't you need to add p[2].AddChildren(self.BuildTrue('STRINGIFILER')) ? That’s handled in the base parser: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... Anyway we have no stringifiers in Blink IDLs, or in fact in Chromium IDLs at all, but we may as well parse them correctly. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:240: # [NotEnumerable] DOMString toString(); On 2013/07/16 14:17:51, haraken wrote: > Why do we need to handle toString() specially. Operations in Exceptions are not in the Web IDL spec, and there’s only one example, so I didn’t want to allow general Operations here. I’ve renamed this to make it clearer and clarified the comment; how does it look? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:244: # [b49] Override base parser: remove comment field, since comments stripped On 2013/07/16 14:17:51, haraken wrote: > I'm just curious: Would you elaborate on why we need to override the rule for > ExtendedAttributeList? The Pepper IDL parser assumes that each interface member is preceded by a *mandatory* comment, which is used in documentation. In the grammar this is considered part of the (optional) ExtendedAttributeList. Since we don’t require comments, we need to override this: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:253: p[0] = ListFromConcat(p[0], attribs) On 2013/07/16 14:17:51, haraken wrote: > I'm just curious: Why isn't this "p[0] = attribs" ? Good catch! Fixed (and actually don’t need auxiliary ‘attribs’.) I just copied this from idl_parser.py (shifting indices by 1 b/c removing Comment), and there p[1] is used to store the comment, which we don’t have. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:255: # [b50] Allow optional trailing comma On 2013/07/16 14:17:51, haraken wrote: > Haven't you already removed all trailing commas from Blink IDLs? (See separate reply.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:307: if len(p) == 3: On 2013/07/16 14:17:51, haraken wrote: > Nit: elif (>.<) Done. Needs fixing in base parser too. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:315: p[0] = self.BuildNamed('ExtAttribute', p, 1, value) On 2013/07/16 14:17:51, haraken wrote: > I'm just curious: Where is the identifier of p[1] added to CST? p[1] is recorded in the name: that’s what BuildNamed does. (Clearer now with explanation of BuildNamed in comment.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_compiler.py:44: import pickle On 2013/07/16 14:17:51, haraken wrote: > Nit: alphabetical order please. Oops, fixed. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_compiler.py:80: def write_json_and_pickle(definitions, interface_name, output_directory): On 2013/07/16 14:17:51, haraken wrote: > I'm OK with leaving this option until you finish moving everything. But please > add a FIXME about it. (As above.) Done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_compiler.py:86: # Pickle export (for Koji) On 2013/07/16 14:17:51, haraken wrote: > > Nit: Remove this comment. Done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:31: Also JSON export, using legacy Perl terms and format. On 2013/07/17 02:44:49, haraken wrote: > Given that we decided to keep deprecated_idl_parser.pm (for performance > reasons), there is no need to provide JSON from the new Python flow to the old > Perl flow. Right; see separate detailed reply. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:37: # pylint: disable=W0232, E0203, W0201 On 2013/07/16 14:17:51, haraken wrote: > Is this comment helpful? Yup, I get lots of pylint errors and warnings otherwise. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:51: __metaclass__ = abc.ABCMeta On 2013/07/17 05:43:11, haraken wrote: > I'm a bit confused with the comments above. You can remove json_serializable() > when removing the Perl flow, but you will still need the classes, __init__'s and > apply_typedefs()'s. Yup, but we don’t need the BaseIdl class, since it’s only used for JSON export (in the “isinstance” line at the end of the file). We do still need the classes and TypedObject. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:65: class TypedObject(BaseIdl): On 2013/07/17 05:43:11, haraken wrote: > Actually I don't fully understand what class should inherit BaseIdl and what > class should inherit TypedObject. What's the difference? BaseIdl is used for JSON export, so everything in IdlDefinitions needs to inherit from it, while TypedObject is for applying typedefs. Actually, these are orthogonal, so it’s clearer to use multiple inheritance, that way these are clearly separate (it’s confusing to have TypedObject inherit from BaseIdl). Fixed! https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:67: __metaclass__ = abc.ABCMeta On 2013/07/17 05:43:11, haraken wrote: > This wouldn't be needed. Setting metaclass to ABCMeta is needed to indicate that TypedObject is also abstract; anyway I’ve removed the inheritance from BaseIdl. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:87: raise ValueError('Extended attributes in a typedef are untested!') On 2013/07/17 05:43:11, haraken wrote: > This will happen in bindings/tests/idls/TestTypedefs.idl, so you can test it. Got it, removed! (We don’t have examples in the actual Blink IDLs, but since it’s in the tests, that’s fine.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:94: def __init__(self, callback_functions=None, enumerations=None, exceptions=None, file_name=None, interfaces=None, typedefs=None): On 2013/07/17 05:43:11, haraken wrote: > It looks like you're explicitly specifying all parameters in the caller side. > Would it make sense to get rid of default parameters? (Long comment.) We need default parameters to have named parameters in Python, and given the number of parameters, it’s safer to name them. However, your point is well-taken; the calls are quite long; answered in detail in separate reply. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:102: self.apply_typedefs(typedefs) On 2013/07/17 05:43:11, haraken wrote: > Help me understand: Why do you need to call apply_typedefs() here? Typedefs aren’t exposed by the bindings (they’re purely used as a shorthand for referencing the type in the IDL), so typedefs are translated to the actual type, and not stored themselves in the Definitions. I’ve added a comment and reference to the spec: http://www.w3.org/TR/WebIDL/#idl-typedefs https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:105: for callback_function in self.callback_functions: On 2013/07/17 05:43:11, haraken wrote: > callback_functions.itervalues(). > > The same comment for other parts. You can add .itervalues() here and there. Good point. It’s useful to access all of these by name, and it’s more consistent anyway. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:374: On 2013/07/17 05:43:11, haraken wrote: > Shall we use if-else? You’re right, this code was a bit messy and duplicative. I’ve cleaned it up. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:29: """Read an IDL file or complete IDL interface, producing an IdlDefinitions object.""" On 2013/07/17 02:44:49, haraken wrote: > Slightly better: "Build an IdlDefinitions object from AST (which is built by > idl_parser)". Oops, you’re right (this was the idl_reader comment). Fixed. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:36: def file_node_to_idl_definitions(node): On 2013/07/17 02:44:49, haraken wrote: > For readability, I might want to add one more helper method which is exported to > idl_compiler.py. > > def build_idl_definitions_from_ast(ast): > file_node_to_idl_definitions(ast) You’re right, that’s clearer. I actually had this earlier and removed it as part of removing “AST” as much as possible, but for the interface it’s clearer this way. Fixed, with slight refactoring too. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:67: pass On 2013/07/17 02:44:49, haraken wrote: > Help me understand: Why can we skip 'Implements'? Even if we resolve the > dependency in the later phase, I guess we need to include the information of > 'Implements' to IdlDefinitions. We should probably handle Implements differently (also Parents), but that’s a bigger change, as part of fixing the messy dependency resolution. What’s done now is that the code generator knows nothing about implements. If we have “A implements B”, then the interface resolver parses A and B, puts them both into interface “A”, and discards B. So this is on my todo list, but parser ignores implements for now. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:71: return IdlDefinitions(callback_functions=callback_functions, enumerations=enumerations, exceptions=exceptions, file_name=file_name, interfaces=interfaces, typedefs=typedefs) On 2013/07/17 02:44:49, haraken wrote: > Nit: One unnecessary space before file_name. (>.<) Fixed. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:94: clear_constructor_attributes(attribute.extended_attributes) On 2013/07/17 02:44:49, haraken wrote: > Would you add the following FIXME? > > FIXME: This is a hack to support [CustomConstructor] for > window.HTMLImageElement. Remove the hack. Done. Thanks for explaining why we have this! https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:99: extended_attributes = ext_attributes_node_to_extended_attributes(child) On 2013/07/17 02:44:49, haraken wrote: > Don't you need to call clear_constructor_attributes(extended_attributes) ? The clearing is done as part of extended_attributes_to_constructors. You’re right though, it’s clearer to explicitly call clear_constructor_attributes. Changed. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:105: elif child_class == 'ExceptionFieldToString': On 2013/07/17 02:44:49, haraken wrote: > I don't fully understand why this is needed. (See above; this is a single special-case Operation.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:139: num_children = len(children) On 2013/07/17 02:44:49, haraken wrote: > Shall we check that num_children > 1 ? Good point. It would have thrown an error later, but better to make it explicit. Done, and changed passim. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:169: for special_keyword in SPECIAL_KEYWORD_LIST: On 2013/07/17 02:44:49, haraken wrote: > Why haven't the special keywords parsed in the parser? It seems a bit strange to > me that SPECIAL_KEYWORD_LIST appears not in blink_idl_parser but in > idl_definitions_buider.py. The special keywords are handled by the lexer and parser (the base Pepper lexer and parser), but we need this explicit checking because the parser puts all the special keywords as properties (in property_dictionary), rather than as a list. I.e., the parser *flattens* the specials list into properties (along with static etc.), so we need to explicitly reconstruct the list. It would be clearer if there were a specials_node whose children were the specials, so we could just loop over its children instead of hardcoding a list of properties to extract. I’ve added a FIXME. Here’s where the base parser handles specials: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:191: def arguments_node_to_arguments(arguments_node): On 2013/07/17 02:44:49, haraken wrote: > Nit: arguments_node => node (for consistency) (>.<) Done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:192: if arguments_node is None: On 2013/07/17 02:44:49, haraken wrote: > Is this check needed? It makes handling constructors much easier, since it saves us 3 checks in extended_attributes_to_constructors. This is because when the extended attribute [Constructor] (etc.) is parsed, no Arguments node is created. We could fix this in the IDLs by having [Constructor()] instead of [Constructor], which is perhaps clearer, but a bit long. I've added a comment; is it useful? https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:207: # Make all default to False once Perl removed. On 2013/07/17 02:44:49, haraken wrote: > You can remove this comment, as the same thing is written below. Done. (Actual delete the nested comment, so it’s clear we want to change all cases.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:264: children = node.GetChildren() On 2013/07/17 02:44:49, haraken wrote: > Shall we check that len(children) > 0 ? Done. (As above.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:342: # However, Named Constructors cannot be overloaded, and thus do not have On 2013/07/17 02:44:49, haraken wrote: > Actually this should be a FIXME. Added FIXME! https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:406: extended_attributes['Constructor'] = None On 2013/07/17 02:44:49, haraken wrote: > You can use clear_constructor_attributes(). Done. (As above.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:409: # FIXME: I have no idea why this is necessary. On 2013/07/17 02:44:49, haraken wrote: > Yeah, I don't understand why either. We shouldn't use [CallWith] and > [RaisesException] in constuctors and should just use [ConstructorCallWith] and > [ConstructorRaisesException]. Then we no longer need to handle these attributes > specially. Got it - thanks for the explanation! Added FIXME. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:428: extended_attributes['CustomConstructor'] = None On 2013/07/17 02:44:49, haraken wrote: > You can use clear_constructor_attributes(). Done. (As above.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:435: arguments_node = call_node.GetChildren()[0] On 2013/07/17 02:44:49, haraken wrote: > Do you want to check that len(children) > 0 ? Done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:445: del extended_attributes['Constructors'] On 2013/07/17 02:44:49, haraken wrote: > This del won't be needed, because you're setting None in the next line. It deletes the plural Constructor*s* and sets the singular Constructor. This is easy to miss, so I’ve added a comment. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:446: extended_attributes['Constructor'] = None On 2013/07/17 05:43:11, haraken wrote: > If you set None, I'm afraid that we cannot distinguish whether [Constructor] is > specified or not. To avoid the problem, the current Perl flow uses > "VALUE_IS_MISSING" for this case. How can the new Python flow distinguish it? The keys of extended_attributes are the names of the extended attributes that are specified, while their values are the values. Thus if Constructor is not specified, then extended_attributes does not have a Constructor key. If Constructor is specified without any parameters, then extended_attributes['Constructor'] == None (exactly the same as Perl's 'VALUE_IS_MISSING'). If Constructor is specified with parameters, then extended_attributes['Constructor'] == the parameters. (Actually stored in Constructors, but that’s the idea.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:448: del extended_attributes['CustomConstructors'] On 2013/07/17 02:44:49, haraken wrote: > Ditto. Ditto. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:457: if not children: On 2013/07/17 02:44:49, haraken wrote: > Instead, you can check that len(children) > 0 In general “if not some_list:” is considered more Pythonic than “if len(some_list) > 0:”, (it's in the style guide), but since we’re explicitly checking maximum length too, it’s clearer to have a single length check; done. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:476: """Auxiliary.""" On 2013/07/17 02:44:49, haraken wrote: > I'd remove this comment. Good point; not the world’s most useful comment. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_reader.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_reader.py:44: should_generate_bindings = interface_dependency_resolver.merge_dependencies(definitions, idl_filename, interface_dependencies_filename, additional_idl_filenames, verbose=verbose) On 2013/07/16 14:17:51, haraken wrote: > Nit: merge_dependencies => resolve_dependencies ? That’s clearer; done! (Internal functions that merge a specific interface or list are still called “merge”, but resolution + merging is “resolve”.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:45: return On 2013/07/17 02:44:49, haraken wrote: > You might want to output an error for this case. Fixed by refactoring. This “return” is because the IDL extended attributes file is optional (ditto with dependencies resolution), and we’re doing all the checking at the bottom. I moved the checks that there’s a filename into read_idl_definitions, which makes these functions clearer and makes it explicit that the dependency resolution and attribute validation can be skipped. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:78: # FIXME: this should be done when parsing the file, rather than after. On 2013/07/17 02:44:49, haraken wrote: > I think postmortem processing sounds reasonable. (See longer reply.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:111: if value not in valid_values: On 2013/07/17 02:44:49, haraken wrote: > Don't you need to do value.strip() ? Nope, since the parser has already handled whitespace: the values are tokens, hence no whitespace. (We need to handle whitespace ourselves when reading the extended attributes file though.) Actually, I don’t even need to do values_string.strip(), so removed for consistency!
(Trailing commas?) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:255: # [b50] Allow optional trailing comma On 2013/07/16 14:17:51, haraken wrote: > Haven't you already removed all trailing commas from Blink IDLs? I removed them, but people kept putting them back. find . -name '*.idl' | xargs grep -l -Pzo ',\s*]' ...currently yields 9 files. After thinking about it more, this looks like one place where the spec should change. All our other changes are Blink-specific hacks, but this is a useful syntax change. I filed a bug at W3C, and this might change: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22156 ...so in retrospect I shouldn’t have removed the trailing commas - sorry!
(Python module path.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_lexer.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:59: sys.path.append(tools_dir) On 2013/07/16 14:17:51, haraken wrote: > Instead of writing the relative path here, can you pass the path from a GYP > file? We want to centralize path handling in GYP files. I don’t think there’s an easy way to specify the Python module search path from GYP. In Perl we can just use -I<dir> to specify include directories, but in Python they don’t want you to do that. To change the Python search path, you can either: * modify sys.path inside the script, or * change the environment variable PYTHONPATH: http://docs.python.org/2/tutorial/modules.html#the-module-search-path I don’t see how to set environment variables inside GYP, and no .gyp file uses PYTHONPATH (0 hits): https://code.google.com/p/chromium/codesearch#search/&q=PYTHONPATH%20file:.*%... ...so I believe this is not possible or discouraged. This path handling is copied directly from the base Pepper parser idl_lexer.py/idl_parser.py; I was aiming to be consistent: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... ...and changing sys.path is very common in the Chromium codebase (782 hits): https://code.google.com/p/chromium/codesearch#search/&q=sys.path%20lang:%5Epy... Anyway, we still need to list the included files explicitly as ‘inputs’ (so GYP computes dependencies correctly), so if anything moves we’ll detect it immediately on regyping; this shouldn’t bite us. FWIW, if this were all one huge Python package, we could use: from .....tools.idl_parser.idl_lexer import IDLLexer ...but we can’t use this across separate packages.
(When to do Extended Attribute validation?) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:78: # FIXME: this should be done when parsing the file, rather than after. On 2013/07/17 02:44:49, haraken wrote: > I think postmortem processing sounds reasonable. It’s not a big deal either way, but validating while parsing would let us report the exact line number of an error, and would have simpler code: we’d just add a check in p_ExtendedAttribute, avoiding having to walk the tree or special-case the merged attributes. I’ll look into this later when simplifying the parser (moving things upstream), and if it turns out to be messier that way, I’ll just remove this comment (ok if I leave for now?).
(Remove JSON once Perl gone.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:31: Also JSON export, using legacy Perl terms and format. On 2013/07/17 02:44:49, haraken wrote: > > Given that we decided to keep deprecated_idl_parser.pm (for performance > reasons), there is no need to provide JSON from the new Python flow to the old > Perl flow. Thus you can completely remove idl_definitions.py once you verify > that the new Python parser generates exactly the same output as the old Perl IDL > parser. Is my understanding correct? > > If my understanding is correct, please update the comment accordintly. Exactly, or more precisely: we still need the class definitions (except for BaseIDL), but can remove all the JSON export code. It’s safer to keep the JSON export (and JSON.pm) around until the Perl parser is gone though so we can verify in case some .idl or parser change breaks something. > (BTW, I'm very sorry that you had to spent time introducing JSON.pm, which was > not used in the end.) No problem; it’s still useful for verifying compatibility, but in the end we didn’t need to get it into the trunk.
(Object construction design discussion.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:94: def __init__(self, callback_functions=None, enumerations=None, exceptions=None, file_name=None, interfaces=None, typedefs=None): On 2013/07/17 05:43:11, haraken wrote: > It looks like you're explicitly specifying all parameters in the caller side. > Would it make sense to get rid of default parameters? (Continuing: default parameters needed for named parameters.) (Discussion on design of idl_definitions vs. idl_definitions_builder.) Your point is well-taken; the calls are quite long. We could think of idl_definitions_builder as a bunch of constructors for the various IDL objects. For example, interface_node_to_idl_interface could instead be IdlInterface.__init__. Then we wouldn’t need the long initializer calls and the function names would be simpler, but the AST to object code would be in the class definitions, which is really messy. Also, if we can actually get rid of the AST to object code entirely by putting the object construction in the base parser, we’d need these explicit long constructors, so long-term we’d be better keeping these. Once the Perl is gone and I’ve upstreamed the parser tweaks to the Pepper one, I’ll see if we can push all the object code into the base parser, in which case our code would be very simple (just the Blink IDL tweaks). But that's some time off.
BTW, that's it for my replies and revisions for this round. A bunch of clarifications, and some refactoring and clearer code, (thanks for the good catches!) though no major changes. Thanks again for your assiduousness!
Thanks for all the detailed replies! All make sense to me with some nits below. (I'll start a thorough re-review of your latest patch from now.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_lexer.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:59: sys.path.append(tools_dir) Given that this is a limitation of Python, I'm OK with the hard-coded paths. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:156: # [b1.1] for Blink IDL additions, auxiliary rules for [b1] Makes sense. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:214: p[0] = p[2] Ah, got it. But it looks inconsistent. Given that STRINGIFILER is a part of the AttributeOrOperation rule, shouldn't we handle STRINGIFIER in the AttributeOrOperation rule? (This will require a change in the base Pepper parser though.) https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:240: # [NotEnumerable] DOMString toString(); Looks good, thanks! https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:255: # [b50] Allow optional trailing comma Thanks, fixing the spec side sounds reasonable to me. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:66: """Object with a Type, such as an Attribute or Operation (return value).""" You might also want to add "The type name can be modified by typedef's". https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:94: def __init__(self, callback_functions=None, enumerations=None, exceptions=None, file_name=None, interfaces=None, typedefs=None): Agreed, thanks for the clarification. Let's keep the default parameters. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:169: for special_keyword in SPECIAL_KEYWORD_LIST: Got it. Given that static, const etc are stored in property_dictionary, it will also make sense to store special-keywords in property_dictionary. Namely, your current implementation looks good. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:192: if arguments_node is None: Makes sense. Given that the spec allows both [Constructor] and [Constructor()], we should support both as well. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:78: # FIXME: this should be done when parsing the file, rather than after. Sounds like a plan. I don't have a strong opinion about where the validation check is conducted.
LGTM. Below are final comments. I'm doing a Blink gardening til Thu, so feel free to land it when I'm online. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_lexer.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:71: self.AddLines(t.value.count('\n')) Don't you need to write 'return t' at the end? idl_ppapi_lexer.py has 'return t'. Actually I still don't understand how t_XXX works (Probably I just don't understand Python syntax). def t_XXX(self, t): r'yyy' # This just defines a raw string, that's it. How can it affect behavior of the lexer? self.AddLines(...) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:80: # * The docstring is the production rule in BNF (grammar). Help me understand: The docstring is actually used by the parser, right? I mean, if we drop the docstring, it will break the parser. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:126: # * BuildNamedProduction: Same as BuildProduction, and sets the 'NAME' BuildNamedProduction => BuildNamed https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:222: p[0] = p[2] As mentioned in the previous comment, it looks inconsistent that we add STRINGIFIER when parsing the StringifierAttributeOrOperation rule. Shall we add a FIXME? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:265: # [b50] Allow optional trailing comma Shall we add a FIXME that says this is currently a dialect of Blink IDL but will be soon fixed in the spec? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:270: if len(p) > 2: Nit: len(p) > 3 https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:305: if len(p) > 1: Nit: len(p) > 3 https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:321: """ExtendedAttributeIdentAndOrIdent : identifier '=' identifier '&' identifier Help me understand: How is [X=A&B&C] parsed? I thought this rule could be "identifier = identifier & identifier*s*". https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:68: """Object with a Type, such as an Attribute or Operation (return value).""" As mentioned in the previous comment, let's add "the type name can be modified by typedef's". https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:70: data_type = None Shall we add "extended_attributes = None" ? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:74: """Applies Typedefs to object itself (e.g., return type of function). function => operation https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:76: Override for instance if calling on arguments of function.""" Nit: I'd remove this comment. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:85: new_extended_attributes = replacement_type.extended_attributes Nit: new_extended_attributes => additional_extended_attributes https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:36: def idl_definitions_from_ast(node): Nit: build_idl_definitions_from_ast ? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:79: # Interface # Definitions for Interface components https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:87: extended_attributes = None Nit: extended_attributes = {} https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:88: functions = [] Probably we should rename 'function' to 'operation' per the spec. The same comment for other parts of this CL. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:151: data_type = type_node_inner_to_type(type_node) Shouldn't this be type_node_to_type ? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:159: extended_attributes = None Nit: extended_attributes = {} https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:174: # FIXME: AST should have a specials_node so don't need to hard-code list Sorry about my previous comment. Given that 'constant' and 'static' are handled in the idl_definitions_builder layer, it makes sense to handle special-keywords in the idl_definitions_builder layer. So I'd remove the FIXME. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:181: extended_attributes = None extentded_attributes = {} https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:199: def arguments_node_to_arguments(node): arguments_node_to_idl_arguments ? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:202: # special-case this. Just to confirm: [Constructor] and [Constructor()] are equivalent, right? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:220: is_optional = node.GetProperty('OPTIONAL') Nit: is_optional = node.GetProperty('OPTIONAL') or False https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:221: is_variadic = None Nit: is_variadic = False https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:227: is_nullable = child.GetProperty('NULLABLE') Nit: is_nullable = child.GetProperty('NULLABLE') or False https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:229: extended_attributes = ext_attributes_node_to_extended_attributes(child) 'ext_attributes' and 'extended_attributes' are mixed. We might want to unify them to 'ext_attributes' before starting rewriting the code generator (because the code generator has to write a lot of 'ext_attributes'). You can do it in a follow-up CL. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:240: # Minor definitions # Definitions of non-Interface components https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:270: def exception_operation_node_to_idl_operation(node): Shall we add a FIXME that says this is just for compatibility with Mozilla? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:291: extended_attributes = None Nit: extended_attributes = {} https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:315: extended_attributes = None Nit: extended_attributes = {} https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:484: if node_class in ['PrimitiveType', 'Typeref']: Help me understand: What is Typeref? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:510: member_type = type_node_inner_to_type(member_type_node) Shouldn't this be type_node_to_type ? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:96: def validate_name_value_string(self, name, values_string): Nit: values_string => value_string (for consistency with the method name and the parameter name in the caller side.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:110: raise IDLInvalidExtendedAttributeError('Invalid value "{value}" found in extended attribute [{name}={value_string}]'.format(**locals())) Help me understand: What is .format(**locals()) for? https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/interface_dependency_resolver.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:46: """Raised if (partial) interface not found in target.""" Slightly better: "Raised if (partial) interface not found in the target IDL file." https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:56: """Resolves dependencies, merging them into an existing IDL document. Slightly better: "Resolves interface dependencies of 'partial' and 'implements', merging them into the IDL definition of the base interface." https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:81: interface = definitions.interfaces[interface_name] Nit: interface => target_interface (for clarity) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:136: def merge_interface_dependencies(interface, idl_filename, dependency_idl_filenames, verbose=False): Nit: interface => target_interface https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:137: """Merge partial interfaces in dependency_idl_files into target_document. dependency_idl_files => dependency_idl_filename target_document => target_interface https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:139: No return: modifies target_document in place. target_document => target_interface https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:158: def merge_dependency_interface(interface, dependency_interface, dependency_interface_name): Nit: interface => target_interface https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:159: """Merge dependency_interface into interface. Ditto. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:161: No return: modifies interface in place. Ditto.
Thanks for thorough comments; I've addressed and have posted a hopefully final CL. Will do final checks then commit. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:214: p[0] = p[2] On 2013/07/21 14:31:50, haraken wrote: > Ah, got it. But it looks inconsistent. Given that STRINGIFILER is a part of the > AttributeOrOperation rule, shouldn't we handle STRINGIFIER in the > AttributeOrOperation rule? (This will require a change in the base Pepper parser > though.) Good point, that would be clearer: just AddChildren(self.BuildTrue('STRINGIFIER')) However, the grammar for stringifiers is pretty complex, due to allowing anonymous stringifiers, and the base parser treats different production rules differently, so it needs to do the processing in the StringifierAttributeOrOperation rule; see: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... Notably, all the following are valid, and the latter two are semantically equivalent (!), since you can omit the signature for an anonymous stringifier: stringifier DOMString foo(); stringifier DOMString (); stringifier; Anyway, we’re not using stringifiers yet so we have no tests, but we should (for toString; see below), and when we do, we’ll need to handle this properly, and at that point I’ll see if we can simplify the parsing. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:240: # [NotEnumerable] DOMString toString(); On 2013/07/21 14:31:50, haraken wrote: > Looks good, thanks! Looking into this more, this “toString()” function is exactly what stringifiers are for. So I’ve added a “replace toString() with stringifier” to my to-do list. This will also make the grammar for Exceptions clearer. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:255: # [b50] Allow optional trailing comma On 2013/07/21 14:31:50, haraken wrote: > Thanks, fixing the spec side sounds reasonable to me. I've added a link to the bug too: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22156 https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:66: """Object with a Type, such as an Attribute or Operation (return value).""" On 2013/07/21 14:31:50, haraken wrote: > You might also want to add "The type name can be modified by typedef's". Good point: “type” can be a “Type” (actual type) or a “Typedef” (shorthand). Typedefs can be resolved to actual types by calling resolve_typedefs. (Spec uses “resolving typedefs” for “replace Typedefs with actual type”.) I’ve reworded the comment, and renamed to "resolve_typedefs" throughout. https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:192: if arguments_node is None: On 2013/07/21 14:31:50, haraken wrote: > Makes sense. Given that the spec allows both [Constructor] and [Constructor()], > we should support both as well. Got it. Didn’t realize that [Constructor] was one of the spec-specified extended attributes, not one of the (many) Blink-only ones. I've added a comment and link to spec: http://dev.w3.org/2006/webapi/WebIDL/#es-extended-attributes https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/27001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:78: # FIXME: this should be done when parsing the file, rather than after. On 2013/07/21 14:31:50, haraken wrote: > Sounds like a plan. I don't have a strong opinion about where the validation > check is conducted. Got it; we’ll see what makes sense when actually trying it out. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_lexer.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_lexer.py:71: self.AddLines(t.value.count('\n')) On 2013/07/22 01:50:23, haraken wrote: > Don't you need to write 'return t' at the end? idl_ppapi_lexer.py has 'return > t'. We don’t want to “return t” because we’re discarding comments. > Actually I still don't understand how t_XXX works (Probably I just don't > understand Python syntax). There are two issues: Python syntax, and PLY syntax. 1. Python docstrings In general in Python, you document by having a docstring, a string literal as the first statement of a definition (function etc.): http://www.python.org/dev/peps/pep-0257/ “A docstring is a string literal that occurs as the first statement in a module, function, class, or method definition. Such a docstring becomes the __doc__ special attribute of that object.” 2. PLY syntax PLY uses the docstring as the pattern to match: a regex in the lexer, a production rule (in BNF) in the parser. That is, the PLY lexer framework goes through the attributes of the lexer object, and extracts the pattern by checking their __doc__ attribute; likewise for the parser. There’s a good comment in the base lexer: https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i... # Lex assumes any value or function in the form of 't_<TYPE>' represents a # regular expression where a match will emit a token of type <TYPE>. In the # case of a function, the function is called when a match is made. These # definitions come from WebIDL. ...so e.g., in: def t_string(self, t): r'"[^"]*"' ...this defines a token type “string”, matching the pattern r'"[^"]*"'; sequences that match this token are then passed to this function, which processes it (in this case the function strips the quotes and returns the stripped string). I’ve added a more detailed comment in the parser, which should help, and I’ll hopefully eliminate the Blink-specific lexer soon. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... File Source/bindings/scripts/blink_idl_parser.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:80: # * The docstring is the production rule in BNF (grammar). On 2013/07/22 01:50:23, haraken wrote: > Help me understand: The docstring is actually used by the parser, right? I mean, > if we drop the docstring, it will break the parser. Exactly (see above, in lexer). I’ve written a more detailed comment. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:126: # * BuildNamedProduction: Same as BuildProduction, and sets the 'NAME' On 2013/07/22 01:50:23, haraken wrote: > BuildNamedProduction => BuildNamed (>.<) Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:222: p[0] = p[2] On 2013/07/22 01:50:23, haraken wrote: > As mentioned in the previous comment, it looks inconsistent that we add > STRINGIFIER when parsing the StringifierAttributeOrOperation rule. Shall we add > a FIXME? Yup, done! https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:265: # [b50] Allow optional trailing comma On 2013/07/22 01:50:23, haraken wrote: > > Shall we add a FIXME that says this is currently a dialect of Blink IDL but will > be soon fixed in the spec? Done. Not sure if it will be fixed, since Cameron rejected previous suggestion, but he sounds more open now. Bug link enough for details. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:270: if len(p) > 2: On 2013/07/22 01:50:23, haraken wrote: > Nit: len(p) > 3 Technically > 2 is ok (since length is 1, 2, or 4), but you’re right, it’s clearer and safer to accord with the indexing - thanks, fixed! https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:305: if len(p) > 1: On 2013/07/22 01:50:23, haraken wrote: > Nit: len(p) > 3 Ditto, done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/b... Source/bindings/scripts/blink_idl_parser.py:321: """ExtendedAttributeIdentAndOrIdent : identifier '=' identifier '&' identifier On 2013/07/22 01:50:23, haraken wrote: > Help me understand: How is [X=A&B&C] parsed? I thought this rule could be > "identifier = identifier & identifier*s*". Currently we only ever have 2 flags: A|B or A&B, so to keep the grammar simple I’ve only supported this. I’ve added a FIXME to support an arbitrary number, and we’ll need test cases. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:68: """Object with a Type, such as an Attribute or Operation (return value).""" On 2013/07/22 01:50:23, haraken wrote: > > As mentioned in the previous comment, let's add "the type name can be modified > by typedef's". Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:70: data_type = None On 2013/07/22 01:50:23, haraken wrote: > Shall we add "extended_attributes = None" ? You’re right, that’s better. Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:74: """Applies Typedefs to object itself (e.g., return type of function). On 2013/07/22 01:50:23, haraken wrote: > function => operation (>.<) Done. (Fixed throughout.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:76: Override for instance if calling on arguments of function.""" On 2013/07/22 01:50:23, haraken wrote: > > Nit: I'd remove this comment. Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions.py:85: new_extended_attributes = replacement_type.extended_attributes On 2013/07/22 01:50:23, haraken wrote: > Nit: new_extended_attributes => additional_extended_attributes Good point, that’s clearer. Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_definitions_builder.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:36: def idl_definitions_from_ast(node): On 2013/07/22 01:50:23, haraken wrote: > > Nit: build_idl_definitions_from_ast ? Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:79: # Interface On 2013/07/22 01:50:23, haraken wrote: > # Definitions for Interface components Done. Slight wording change to stay closer to spec. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:87: extended_attributes = None On 2013/07/22 01:50:23, haraken wrote: > Nit: extended_attributes = {} That’s handled by IdlDefinitions.__init__ -- I’m using “None” to mean “none found”, which is different from “empty extended attributes found”: interface foo {}; vs. [] interface foo {}; They're semantically the same, but that's not the parser's (AST to object builder) job. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:88: functions = [] On 2013/07/22 01:50:23, haraken wrote: > Probably we should rename 'function' to 'operation' per the spec. The same > comment for other parts of this CL. (>.<) Missed this one. Fixed throughout. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:151: data_type = type_node_inner_to_type(type_node) On 2013/07/22 01:50:23, haraken wrote: > Shouldn't this be type_node_to_type ? No, “inner” is intentional: ConstType is simpler than Type, so we have a simpler tree and use the simpler function. I’ve added a comment to clarify. (However, for UnionType we should use the full one, so I've fixed that one.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:159: extended_attributes = None On 2013/07/22 01:50:23, haraken wrote: > Nit: extended_attributes = {} (See above.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:174: # FIXME: AST should have a specials_node so don't need to hard-code list On 2013/07/22 01:50:23, haraken wrote: > Sorry about my previous comment. Given that 'constant' and 'static' are handled > in the idl_definitions_builder layer, it makes sense to handle special-keywords > in the idl_definitions_builder layer. So I'd remove the FIXME. n/p; got it, done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:181: extended_attributes = None On 2013/07/22 01:50:23, haraken wrote: > extentded_attributes = {} (See above.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:199: def arguments_node_to_arguments(node): On 2013/07/22 01:50:23, haraken wrote: > arguments_node_to_idl_arguments ? That’s potentially confusing, since it suggests that there’s an IdlArgument*s* class, so I’m keeping *_to_idl_* for when there’s a class. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:202: # special-case this. On 2013/07/22 01:50:23, haraken wrote: > Just to confirm: [Constructor] and [Constructor()] are equivalent, right? Yup, per spec: http://www.w3.org/TR/WebIDL/#Constructor This is handled implicitly (unspecified argument list = empty argument list in IdlOperation.__init__), but it’s clearer to do this (and state this) explicitly; fixed. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:220: is_optional = node.GetProperty('OPTIONAL') On 2013/07/22 01:50:23, haraken wrote: > Nit: is_optional = node.GetProperty('OPTIONAL') or False Nonono, these are all hacks for Perl compatibility; see the FIXME. This doesn't affect the Python logic (assuming Koji doesn't distinguish None and False), but it does change the JSON output, so it means I can check "exactly the same". It sure is ugly though, and I'm really looking forward to fixing this once the Perl compiler is completely gone. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:221: is_variadic = None On 2013/07/22 01:50:23, haraken wrote: > Nit: is_variadic = False Ditto (Perl hack). https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:227: is_nullable = child.GetProperty('NULLABLE') On 2013/07/22 01:50:23, haraken wrote: > Nit: is_nullable = child.GetProperty('NULLABLE') or False Ditto (Perl hack). https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:229: extended_attributes = ext_attributes_node_to_extended_attributes(child) On 2013/07/22 01:50:23, haraken wrote: > 'ext_attributes' and 'extended_attributes' are mixed. We might want to unify > them to 'ext_attributes' before starting rewriting the code generator (because > the code generator has to write a lot of 'ext_attributes'). You can do it in a > follow-up CL. This is following the spec: the grammar rule is called ExtAttributes, but the actual name is “extended attributes”. Understood re: legibility in code generator, given how much it’s used. As a rule we don’t abbreviate in Blink, but “ext_attributes” or even “ext_attrs” is rather easier than “extended_attributes”. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:240: # Minor definitions On 2013/07/22 01:50:23, haraken wrote: > > # Definitions of non-Interface components Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:270: def exception_operation_node_to_idl_operation(node): On 2013/07/22 01:50:23, haraken wrote: > Shall we add a FIXME that says this is just for compatibility with Mozilla? I’ve added a note and a FIXME. I don’t know if we can remove this, or use a stringifier instead, but I’ll look into this. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:291: extended_attributes = None On 2013/07/22 01:50:23, haraken wrote: > Nit: extended_attributes = {} (See above.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:315: extended_attributes = None On 2013/07/22 01:50:23, haraken wrote: > Nit: extended_attributes = {} (See above.) https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:484: if node_class in ['PrimitiveType', 'Typeref']: On 2013/07/22 01:50:23, haraken wrote: > Help me understand: What is Typeref? ! Good catch! It’s a typo for Type*d*ef in the base parser. I’ve added a FIXME. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_definitions_builder.py:510: member_type = type_node_inner_to_type(member_type_node) On 2013/07/22 01:50:23, haraken wrote: > Shouldn't this be type_node_to_type ? Good point. I gave a simple definition for UnionMemberType, but you’re right, they actually should be the same as normal Type (unlike ConstType, which is simpler). Fixed. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/idl_validator.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:96: def validate_name_value_string(self, name, values_string): On 2013/07/22 01:50:23, haraken wrote: > Nit: values_string => value_string (for consistency with the method name and the > parameter name in the caller side.) Oops, good point -- this was inconsistent (elsewhere too). I’ve actually changed to values_string to emphasize that it needs splitting. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/idl_validator.py:110: raise IDLInvalidExtendedAttributeError('Invalid value "{value}" found in extended attribute [{name}={value_string}]'.format(**locals())) On 2013/07/22 01:50:23, haraken wrote: > Help me understand: What is .format(**locals()) for? It’s string formatting, so we can write {name} to substitute the value of the variable ‘name’, and corresponds to “$name” in Bash or Perl. The .format(**locals()) is ugly, but it’s the standard way to do it in Python, and means that the format template string itself is clear. Actually though, this is a hint that Python doesn't want you to do this. (Lint can't see what variables you're using, for instance.) I've replaced this with explicit variables instead (which is consistent with other uses). For background: The **locals() expression says “make named parameters from the dictionary of local variables”; this ** is useful when passing named parameters by passing a dictionary. The alternatives are to either: 1. use %s in the format string and listing the variables outside, which is a bit indirect, or 2. use %(foo)s format strings and then % locals() afterwards, which makes the format string a little harder to read. The %s solution is the simplest and most explicit, so I've changed to that. Some (long) reference: http://stackoverflow.com/questions/1550479/python-is-using-vars-locals-a-good... https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... File Source/bindings/scripts/interface_dependency_resolver.py (right): https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:46: """Raised if (partial) interface not found in target.""" On 2013/07/22 01:50:23, haraken wrote: > > Slightly better: "Raised if (partial) interface not found in the target IDL > file." Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:56: """Resolves dependencies, merging them into an existing IDL document. On 2013/07/22 01:50:23, haraken wrote: > Slightly better: "Resolves interface dependencies of 'partial' and 'implements', > merging them into the IDL definition of the base interface." I've changed comment, rewording a bit. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:81: interface = definitions.interfaces[interface_name] On 2013/07/22 01:50:23, haraken wrote: > Nit: interface => target_interface (for clarity) Fixed throughout. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:136: def merge_interface_dependencies(interface, idl_filename, dependency_idl_filenames, verbose=False): On 2013/07/22 01:50:23, haraken wrote: > > Nit: interface => target_interface Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:137: """Merge partial interfaces in dependency_idl_files into target_document. On 2013/07/22 01:50:23, haraken wrote: > > dependency_idl_files => dependency_idl_filename > target_document => target_interface Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:139: No return: modifies target_document in place. On 2013/07/22 01:50:23, haraken wrote: > > target_document => target_interface Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:158: def merge_dependency_interface(interface, dependency_interface, dependency_interface_name): On 2013/07/22 01:50:23, haraken wrote: > > Nit: interface => target_interface Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:159: """Merge dependency_interface into interface. On 2013/07/22 01:50:23, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/15801003/diff/42001/Source/bindings/scripts/i... Source/bindings/scripts/interface_dependency_resolver.py:161: No return: modifies interface in place. On 2013/07/22 01:50:23, haraken wrote: > > Ditto. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/15801003/60001
Message was sent while issue was closed.
Change committed as 154633
Message was sent while issue was closed.
You did!
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/15801003/diff/60001/Source/bindings/de... File Source/bindings/derived_sources.gyp (right): https://chromiumcodereview.appspot.com/15801003/diff/60001/Source/bindings/de... Source/bindings/derived_sources.gyp:265: '../../../../tools/idl_parser/idl_parser.py', Consider using <(DEPTH) to find the top-level directory.
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/15801003/diff/60001/Source/bindings/de... File Source/bindings/derived_sources.gyp (right): https://chromiumcodereview.appspot.com/15801003/diff/60001/Source/bindings/de... Source/bindings/derived_sources.gyp:265: '../../../../tools/idl_parser/idl_parser.py', On 2013/07/22 17:57:54, abarth wrote: > Consider using <(DEPTH) to find the top-level directory. Thanks, that's much better! Posted CL fixing this: Issue 19476005: Use <(DEPTH) in derived_sources.gyp for saner paths https://codereview.chromium.org/19476005/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
