Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16296004:
JSON export/import in generate-bindings.pl (Closed)
|
Created:
7 years, 6 months ago by Nils Barth (inactive) Modified:
7 years, 6 months ago CC:
blink-reviews, jsbell+bindings_chromium.org, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, kojih, dominicc (has gone to gerrit) Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionInitial CL using JSON export/import in bindings generation.
Roundtrips to/from JSON in usual compile so code used;
does not write intermediate files (that's a build change, next CL).
Roundtrip makes no semantic difference to generated code.
(Changes comments in 3 files, changing order of extended attributes.)
BUG=242795
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152399
Patch Set 1 #Patch Set 2 : Add JSON.pm directory to include paths #
Total comments: 4
Patch Set 3 : Python JSON import #Patch Set 4 : IDL: Perl to/from JSON export/import #
Total comments: 33
Patch Set 5 : Revised #Patch Set 6 : Revised again #
Total comments: 24
Patch Set 7 : Re-revised #
Total comments: 11
Patch Set 8 : Finalized #
Messages
Total messages: 31 (0 generated)
Hi haraken, Here's a small patch implementing JSON export. It depends on JSON.pm so it's blocked by Issue 15736030: Add JSON.pm to third_party http://crrev.com/15736030 ...but it can be reviewed, and tested on Linux.
Just checked on trybot to confirm that we do in fact need JSON.pm, as otherwise get a "Can't locate JSON.pm in @INC" error, as per below: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/307... [62/6146] RULE Generating binding from ../core/css/CSSKeyframeRule.idl FAILED: cd ../../third_party/WebKit/Source/bindings; export BUILT_PRODUCTS_DIR=/Volumes/data/b/build/slave/mac_layout/build/src/out/Debug; export CONFIGURATION=Debug; export PRODUCT_NAME=bindings_derived_sources; export SDKROOT=/Developer/SDKs/MacOSX10.6.sdk; export SRCROOT=/Volumes/data/b/build/slave/mac_layout/build/src/out/Debug/../../third_party/WebKit/Source/bindings; export SOURCE_ROOT="${SRCROOT}"; export TARGET_BUILD_DIR=/Volumes/data/b/build/slave/mac_layout/build/src/out/Debug; export TEMP_DIR="${TMPDIR}";perl -w -Iscripts -I../core/scripts scripts/generate-bindings.pl --outputHeadersDir ../../../../out/Debug/gen/webkit/bindings --outputDir ../../../../out/Debug/gen/webcore/bindings --idlAttributesFile scripts/IDLAttributes.txt --defines "\"ENABLE_3D_PLUGIN=1\" \"ENABLE_BATTERY_STATUS=0\" \"ENABLE_CANVAS_USES_MAILBOX=0\" \"ENABLE_CSS3_TEXT=0\" \"ENABLE_CSS_DEVICE_ADAPTATION=0\" \"ENABLE_CSS_EXCLUSIONS=1\" \"ENABLE_CSS_REGIONS=1\" \"ENABLE_CUSTOM_SCHEME_HANDLER=0\" \"ENABLE_ENCRYPTED_MEDIA_V2=1\" \"ENABLE_SVG_FONTS=1\" \"ENABLE_TOUCH_ICON_LOADING=0\" \"ENABLE_WEBGL=1\" \"ENABLE_XHR_TIMEOUT=0\" \"WTF_USE_CONCATENATED_IMPULSE_RESPONSES=1\" \"ENABLE_CALENDAR_PICKER=1\" \"ENABLE_INPUT_SPEECH=1\" \"ENABLE_INPUT_TYPE_COLOR=1\" \"ENABLE_INPUT_MULTIPLE_FIELDS_UI=1\" \"ENABLE_LEGACY_NOTIFICATIONS=1\" \"ENABLE_MEDIA_CAPTURE=0\" \"ENABLE_NAVIGATOR_CONTENT_UTILS=1\" \"ENABLE_NOTIFICATIONS=1\" \"ENABLE_ORIENTATION_EVENTS=0\" \"ENABLE_PRINTING=1\" \"ENABLE_WEB_AUDIO=1\" \"ENABLE_8BIT_TEXTRUN=1\" \"ENABLE_RUBBER_BANDING=1\"" --include ../modules --include ../core --include ../../../../out/Debug/gen/webkit --supplementalDependencyFile ../../../../out/Debug/gen/supplemental_dependency.tmp --additionalIdlFiles "../core/testing/Internals.idl ../core/testing/InternalSettings.idl ../core/testing/MallocStatistics.idl ../core/testing/TypeConversions.idl \"../../../../out/Debug/gen/webkit/InternalSettingsGenerated.idl\" \"../../../../out/Debug/gen/webkit/InternalRuntimeFlags.idl\"" "../core/css/CSSKeyframeRule.idl" --preprocessor "/usr/bin/gcc -E -P -x c++" --write-file-only-if-changed 1 Can't locate JSON.pm in @INC (@INC contains: scripts ../core/scripts /Library/Perl/Updates/5.10.0 /System/Library/Perl/5.10.0/darwin-thread-multi-2level /System/Library/Perl/5.10.0 /Library/Perl/5.10.0/darwin-thread-multi-2level /Library/Perl/5.10.0 /Network/Library/Perl/5.10.0/darwin-thread-multi-2level /Network/Library/Perl/5.10.0 /Network/Library/Perl /System/Library/Perl/Extras/5.10.0/darwin-thread-multi-2level /System/Library/Perl/Extras/5.10.0 . ../../core/scripts) at scripts/IRToFromJSON.pm line 45. BEGIN failed--compilation aborted at scripts/IRToFromJSON.pm line 45. Compilation failed in require at scripts/generate-bindings.pl line 41. BEGIN failed--compilation aborted at scripts/generate-bindings.pl line 41. ninja: build stopped: subcommand failed.
Basically the approach looks good. Let's get back to this CL once JSON.pm is landed. https://codereview.chromium.org/16296004/diff/4001/Source/bindings/derived_so... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/16296004/diff/4001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:198: '-I../../../JSON/out/lib/perl5', Nit: You can remove this line once JSON.pm is landed, right? Let's land JSON.pm first. https://codereview.chromium.org/16296004/diff/4001/Source/bindings/scripts/IR... File Source/bindings/scripts/IRToFromJSON.pm (right): https://codereview.chromium.org/16296004/diff/4001/Source/bindings/scripts/IR... Source/bindings/scripts/IRToFromJSON.pm:47: sub IRToJSON Nit: I don't want to make one file just for one method (even temporary!). Can you write this in generate-bindings.pl?
(Need JSON.pm to land before we can test; detailed comments follow) https://codereview.chromium.org/16296004/diff/4001/Source/bindings/derived_so... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/16296004/diff/4001/Source/bindings/derived_so... Source/bindings/derived_sources.gyp:198: '-I../../../JSON/out/lib/perl5', On 2013/06/03 06:49:14, haraken wrote: > Nit: You can remove this line once JSON.pm is landed, right? Actually, this line is only useful *after* JSON.pm lands. This include line is necessary because otherwise Perl on Mac doesn't know where to find the library (we don't have a standard third-party Perl module location). > Let's land JSON.pm first. Agreed -- otherwise we can't even test that it works on Mac! https://codereview.chromium.org/16296004/diff/4001/Source/bindings/scripts/IR... File Source/bindings/scripts/IRToFromJSON.pm (right): https://codereview.chromium.org/16296004/diff/4001/Source/bindings/scripts/IR... Source/bindings/scripts/IRToFromJSON.pm:47: sub IRToJSON On 2013/06/03 06:49:14, haraken wrote: > Nit: I don't want to make one file just for one method (even temporary!). Can > you write this in generate-bindings.pl? Agreed that this looks a bit silly now (it's not worth all the boilerplate to avoid putting a 10-line function in generate-bindings), but the import code is going to be significantly longer and messier. This is because JSON is a POD, but our Perl data structure is an object, so there's a lot of undoing the serialization. You can see what's involved in importing to Python (60 lines), where we're just pruning the excess Perl info: https://codereview.chromium.org/15959019/diff/2001/Source/bindings/scripts/bl... I anticipate it'll be about 80 lines of Perl, due to restoring the objects (blessing etc.) -- we have to make sure the imported JSON is *exactly* the same as the original Perl. Basically I expect this to get much longer and messier, and wanted to plan ahead to reduce churn in generate-bindings.pl. If you'd prefer, I can put this initially in generate-bindings.pl, and then if the import code is long (as I expect), we can factor it out.
> You can see what's involved in importing to Python (60 lines), where we're just > pruning the excess Perl info: > https://codereview.chromium.org/15959019/diff/2001/Source/bindings/scripts/bl... > I anticipate it'll be about 80 lines of Perl, due to restoring the objects > (blessing etc.) -- we have to make sure the imported JSON is *exactly* the same > as the original Perl. > > Basically I expect this to get much longer and messier, and wanted to plan ahead > to reduce churn in generate-bindings.pl. > > If you'd prefer, I can put this initially in generate-bindings.pl, and then if > the import code is long (as I expect), we can factor it out. This question is also related to https://codereview.chromium.org/15984007/, but what is your plan of rewriting generate-bindings.pl? In my understanding: - We don't want to make unnecessary changes to generate-bindings.pl. It's going to be removed soon. - What we need in the Python flow would be: --- idl_parser.py : A dialect of the Pepper IDL parser. --- dump_parsing_tree.py : A driver script of the idl_parser.py. It validates semantices of the parsing tree, and then outputs a JSON file. --- code_generator_v8.py : Read the JSON file and outputs generated code.
On 2013/06/03 07:53:54, haraken wrote: > This question is also related to https://codereview.chromium.org/15984007/, but > what is your plan of rewriting generate-bindings.pl? My plan is the Description and Patch at: Issue 15959019: Rewrite generate-bindings.pl in Python https://codereview.chromium.org/15959019/ It follows the plan you describe below, but the CL does it all, and thus is too long and complex to review, so I'm breaking it up -- that's why there's all the refactoring now. > In my understanding: > > - We don't want to make unnecessary changes to generate-bindings.pl. It's going > to be removed soon. That was exactly my thinking -- I initially made no changes to generate-bindings.pl (I didn't want to touch it either!) -- but the resulting CL was too big because I had to port everything in there. Once it's refactored, it's very easy to port incrementally; the refactoring is just preparation. > - What we need in the Python flow would be: > > --- idl_parser.py : A dialect of the Pepper IDL parser. > --- dump_parsing_tree.py : A driver script of the idl_parser.py. It validates > semantices of the parsing tree, and then outputs a JSON file. > --- code_generator_v8.py : Read the JSON file and outputs generated code. Yup, that's exactly what I've done in these 2 CLs: Issue 15801003: [WIP] IDL lexer and parser rewrite in Python https://codereview.chromium.org/15801003/ Issue 15959019: Rewrite generate-bindings.pl in Python https://codereview.chromium.org/15959019/ The specific files are rather: generate_bindings.py : Driver script blink_idl_parser.py (avoid name clash with Pepper IDL parser) semantic_analyzer.py & interface_merger.py : post-parser processing: handle dependencies and validates semantics code_generator_v8.py ...with the IDL dumping and reading either in a separate library or in generate_bindings.py. (The parser and code generator don't need to know about JSON: the driver script calls them and does the conversion.) The main scripts are, as currently in Perl: generate_bindings.py blink_idl_parser.py code_generator_v8.py ...but connected via JSON. I'm sticking as close to the existing code as possible, since changing the structure while porting makes it hard to review. Does this clarify what I'm doing?
> Does this clarify what I'm doing? Yes, thanks for the clarification. It looks like we should discuss more before landing patches. > generate_bindings.py : Driver script blink_idl_parser.py (avoid name clash with Pepper IDL parser) > semantic_analyzer.py & interface_merger.py : post-parser processing: handle dependencies and validates semantics > code_generator_v8.py ...with the IDL dumping and reading either in a separate library or in > generate_bindings.py. (The parser and code generator don't need to know about JSON: the driver script calls them and does the conversion.) I don't see any benefit of splitting the files so much. If it's just for landing patches incrementally or modularization for something we don't yet know, then let's stop it. In particular, I don't at all see any benefit in splitting existing Perl files. The reason I said the original CL (https://codereview.chromium.org/15959019/) was hard to review is that the CL was doing to many things some of which are unclear to me. The fact itself that the CL is big is not a problem. If that's a reasonable size for the CL, I'm fine with reviewing. Basically, we want to make one CL as small as possible but each CL should be testable. Thus, for example, landing only semantic_analyzer.py or interface_merger.py doesn't make sense, because it won't be used at the point where we land them (i.e. it isn't testable). Specifically, regarding rewriting generate-bindings.pl, I don't think you need to split it into pieces.
On 2013/06/03 09:39:45, haraken wrote: > I don't see any benefit of splitting the files so much. The "lexer / parser / semantic analyzer" modules are the usual components of a compiler frontend. We could put interface_merger.py in the semantic analyzer if that helps. > In particular, I don't at all see any benefit in splitting existing Perl files. Got it; if you're fine with reviewing the Python rewrite incrementally, instead of breaking the Perl into pieces and having one patch per piece, that's cool. > Thus, for example, landing only semantic_analyzer.py or interface_merger.py doesn't make sense, > because it won't be used at the point where we land them (i.e. it isn't testable). Right, semantic_analyzer.py and interface_merger.py are only testable after we've landed generate_bindings.py, so we can check that doing the work on the Python side is the same as doing it on the Perl side. I've posted a slimmed down rewrite at: Issue 15722005: Simple rewrite of generate-bindings.pl in Python https://codereview.chromium.org/15722005/
OK, thanks. > Got it; if you're fine with reviewing the Python rewrite incrementally, > instead of breaking the Perl into pieces and having one patch per piece, > that's cool. So let's keep the current generate-bindings.pl as is. You can upload a patch that dumps a JSON file from generate-bindings.pl and reads the JSON file from CodeGeneratorV8.pm. You can do it in one go (otherwise, we cannot test the behavior). > > I don't see any benefit of splitting the files so much. > > The "lexer / parser / semantic analyzer" modules are the usual components of a > compiler frontend. > We could put interface_merger.py in the semantic analyzer if that helps. Bikeshedding: I don't have a strong opinion here but I'd choose put them in one file. Even not in one file, let's minimize the file count. Splitting 200 lines of script into multiple files will just decrease code readability.
On 2013/06/04 06:26:52, haraken wrote: > You can upload a patch that dumps a JSON file from generate-bindings.pl > and reads the JSON file from CodeGeneratorV8.pm. > You can do it in one go (otherwise, we cannot test the behavior). Done! How does it look?
No. (1) Please wait for the JSON.pm patch being landed. (2) Upload a patch that connects generate-binding.pl and CodeGeneratorV8.pm via a JSON file. Specifically, the patch will do: - Output a JSON file from generate-binding.pl. - Read a JSON file from CodeGeneratorV8.pm by using JSON.pm. - Remove unnecessary code from generate-binding.pl. You don't need to introduce any Python file at this point. (There are a couple of issues we have to resolve before introducing the Pytohn flow. e.g. JSON file dependency problem. Regarding the issues, let's pass a baton to koji-san.)
Ok, read JSON back into Perl. Will do!
Informal review. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... File Source/bindings/scripts/IRToFromJSON.pm (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:56: # Blessed references for objects, canonical to order (so can compare) What does this mean? "Blessed references for objects"--which objects? Blessed as? "canonical to order"--what does it mean? I understand the words but not the phrase. I think this can be stated more simply. "so can compare"--cryptic. I doubt most readers know what you mean. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:61: sub rawIRToCleanIR How about a brief comment (one line) explaining what this does in a way that makes it clear what "raw" and "clean" are? For pedagogy it might be better to keep IRToJSON and JSONToIR together, and have this function last. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:78: my $classAndKeyFirst = (keys %{$rawIR})[0]; I think this code can be made much simpler. This variable name is misleading. It is really "classAndFirstKey". However sometimes it does not contain a class. I think a few blank lines for grouping and more intensional variable names would make this easier to understand. For example testing for '::' and putting all of the handling of class/object together would make it clearer what is going on. Some helper functions would also simplify the control flow: if (... is X ...) { return do the thing for X(); } elsif (... is Y ...) { return do the thing for Y(); } ... etc. Is it just a coincidence that all class names come before 'o' of 'overloadedIndex' in sort order? If so relying on that seems gross. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:79: if ($classAndKeyFirst eq "overloadedIndex") { Why special-case overloadedIndex *only* if it is the first one? https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:85: if ($class ne $classAndKeyFirst) { # object I'm having trouble understanding this. It looks like you're comparing the wrong kinds of things here since "class" and "classAndKey..." look like different kinds of things. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... File Source/bindings/scripts/generate-bindings.pl (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:125: my $targetDocument; Can we instead wrap up the thing that sources the abstract syntax--either the parser or the JSON reader--into a couple of functions. And then just do if (...looks like we're doing JSON...) { ...read JSON... } else { ...parse IDL... } do work (stuff I read I care not from whence it came); It might be nice to deport them to separate files. Then when Python is done you can just delete one of said files. It will be very clear what is going on. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:203: # If dumping JSON, generate JSON output and exit I will say this again, but I think you should do the actual file reading writing part in a separate change when they will be actually used by a new step in gyp.
Is the patch set 4 the patch you want us to review? I hope you clean up the patch according to the plan you described in https://code.google.com/p/chromium/issues/detail?id=242795#c14
On 2013/06/11 23:51:44, haraken wrote: > Is the patch set 4 the patch you want us to review? Yes. > I hope you clean up the patch according to the plan you described in > https://code.google.com/p/chromium/issues/detail?id=242795#c14 This CL implements step (1) JSON Export/Import.
Revised per Dominic's comments - thanks! Haraken, do you want to take a look at the latest Patch Set (#5)? https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... File Source/bindings/scripts/IRToFromJSON.pm (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:56: # Blessed references for objects, canonical to order (so can compare) On 2013/06/11 22:56:30, dominicc wrote: > What does this mean? Clarified. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:61: sub rawIRToCleanIR On 2013/06/11 22:56:30, dominicc wrote: > How about a brief comment (one line) explaining what this does in a way that > makes it clear what "raw" and "clean" are? I've renamed "rawIR" and "cleanIR" to "jsonIR" and "perlIR" which should clarify some. I've added a (two-line) comment explaining that we're rebuilding objects; I could make it one line, but it would be a bit terse. > For pedagogy it might be better to keep IRToJSON and JSONToIR together, and have > this function last. Done. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:78: my $classAndKeyFirst = (keys %{$rawIR})[0]; On 2013/06/11 22:56:30, dominicc wrote: > I think this code can be made much simpler. Rewritten, with helper function. It's still a bit ugly, due to special case, but should be clearer. > Is it just a coincidence that all class names come before 'o' of > 'overloadedIndex' in sort order? Yes, but we don't rely on that. overloadedIndex only occurs in domFunction, where it's alphabetically after the other keys (domFunction::key1 ...). This is the order in the JSON, since we've specified that JSON is written in alphabetical order. However, key order in Perl is not guaranteed, hence test for this. We could use a "sort(keys ...)" instead to ensure overloadedIndex never comes first, but that adds the above assumption. > If so relying on that seems gross. We don't rely on it, which is why there's a test: if overloadedIndex comes first, skip over it. We do rely on there being only one special case (overloadedIndex), and on overloadedIndex not being the only key (it's added to an object with other attributes, so it's never the only key), but we don't make any assumptions about ordering. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:79: if ($classAndKeyFirst eq "overloadedIndex") { On 2013/06/11 22:56:30, dominicc wrote: > Why special-case overloadedIndex *only* if it is the first one? We're just looking for the class name here, so we just need to skip over to the next key, which will have the class name (if any). We also check for overloaded within the loop, since it's not an object member. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:85: if ($class ne $classAndKeyFirst) { # object On 2013/06/11 22:56:30, dominicc wrote: > I'm having trouble understanding this. It looks like you're comparing the wrong > kinds of things here since "class" and "classAndKey..." look like different > kinds of things. This tests if the splitting achieved anything (i.e., is it actually A::B?). It's clearer as a regex match (=~ /::/) so I've rewritten as such. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... File Source/bindings/scripts/generate-bindings.pl (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:125: my $targetDocument; For now removed --dump-json/--compile-json from generate-bindings, so these chunks no longer present. Will bear comments in mind on next CL. On 2013/06/11 22:56:30, dominicc wrote: > Can we instead wrap up the thing that sources the abstract syntax--either the > parser or the JSON reader--into a couple of functions. Yup, though we'd have to move the middle of generate-bindings.pl into a function (notably the "foreach my $idlFile (@supplementedIdlFiles)" loop). > It might be nice to deport them to separate files. Then when Python is done you > can just delete one of said files. It will be very clear what is going on. Yup. Alternatively, just move to functions at the bottom of this file (current IDLAttributes functions) and delete them. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:203: # If dumping JSON, generate JSON output and exit On 2013/06/11 22:56:30, dominicc wrote: > I will say this again, but I think you should do the actual file reading writing > part in a separate change when they will be actually used by a new step in gyp. Removed.
https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... File Source/bindings/scripts/IRToFromJSON.pm (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:35: package IRToFromJSON; I'd call this JSONSerializer.pm. IR sounds unclear. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:43: @EXPORT_OK = qw(IRToJSON JSONToIR); I'd remove the code for controlling exports. You can just expose everything just like preprocessor.pm. This script is anyway a temporal one. Let's keep it simple. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:47: use Data::Dumper; Remove this. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:49: use IDLParser; Remove this. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:52: sub IRToJSON SerializeJSON might be a better name. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:110: sub JSONToIR deserializeJSON might be a better name. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... File Source/bindings/scripts/generate-bindings.pl (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:69: 'compile-json' => \$compileJson); Are these options needed? https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:132: goto COMPILE; You can make this change in the next CL. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:203: # If dumping JSON, generate JSON output and exit Yes, this should be in the next CL. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:212: # Roundtrip to/from JSON to verify code Instead you can say: # FIXME: This code will be soon removed once IDLParser.pm and CodeGeneratorV8.pm are connected via JSON files. See crbug/xxxx.
Revised again! How does it look now? https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... File Source/bindings/scripts/IRToFromJSON.pm (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:35: package IRToFromJSON; On 2013/06/12 03:20:21, haraken wrote: > > I'd call this JSONSerializer.pm. IR sounds unclear. Done. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:43: @EXPORT_OK = qw(IRToJSON JSONToIR); On 2013/06/12 03:20:21, haraken wrote: > > I'd remove the code for controlling exports. You can just expose everything just > like preprocessor.pm. This script is anyway a temporal one. Let's keep it > simple. Done. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:47: use Data::Dumper; On 2013/06/12 03:20:21, haraken wrote: > Remove this. Oops (>.<) Done. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:49: use IDLParser; On 2013/06/12 03:20:21, haraken wrote: > Remove this. Ok. Needed so we have classes for object construction. Ok to remove for now b/c this is only called from generate-bindings.pl, which already uses IDLParser, but if we split generate-bindings.pl into Parser + CodeGenerator pieces we'll need it again for the CodeGenerator piece. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/I... Source/bindings/scripts/IRToFromJSON.pm:52: sub IRToJSON On 2013/06/12 03:20:21, haraken wrote: > > SerializeJSON might be a better name. Done. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... File Source/bindings/scripts/generate-bindings.pl (right): https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:69: 'compile-json' => \$compileJson); On 2013/06/12 03:20:21, haraken wrote: > Are these options needed? Already removed in Patch Set 5. We can make the build 2-step in 2 ways: (A) Add options to generate-bindings.pl to either --dump-json, or --compile-json. (B) Split up generate-bindings.pl into two scripts, call them idl-to-json.pl and compile-json.pl. (A) is a shorter but messier patch and doesn't break the build, (B) is cleaner but requires changing the build. We can decide when we do step (2) Change the build. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:132: goto COMPILE; On 2013/06/12 03:20:21, haraken wrote: > You can make this change in the next CL. Yes, that's what we're doing. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:203: # If dumping JSON, generate JSON output and exit On 2013/06/12 03:20:21, haraken wrote: > Yes, this should be in the next CL. ACK. https://codereview.chromium.org/16296004/diff/18001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:212: # Roundtrip to/from JSON to verify code On 2013/06/12 03:20:21, haraken wrote: > > Instead you can say: > > # FIXME: This code will be soon removed once IDLParser.pm and CodeGeneratorV8.pm > are connected via JSON files. See crbug/xxxx. Done.
More comments inline. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... File Source/bindings/scripts/JSONSerializer.pm (right): https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:30: # Library to convert the Perl Intermediate Representation (IR) of IDLs "Perl Intermediate Representation (IR)" could be better. Abstractly, there is one intermediate representation of IDLs. And that is being realized in Perl objects or JSON. The Perl objects and the JSON should be isomorphic; there isn't a "Perl IR" and a "JSON IR." The other thing I don't like about using "intermediate representation" so much is that the JSON data are themselves intermediate between the Perl and Python. It is vaguely confusing. I would write this: # Converts the intermediate representation of IDLs between Perl objects # and JSON. Your two points about why are nice context--work them in. Once you have a high-level comment like this I think the file can use "IR" less. For that matter, this isn't really a generic JSON serializer. Maybe this package should be called "IDLSerializer." That way you can have something with a parallel name on the Python side to give readers a clue how the pieces fit together. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:53: # JSON.pm defaults to barfing on objects (blessed references) and returning keys in indeterminate order; set options to change this: Be consistent--you don't wrap here but do on line 57 https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:67: return jsonIRToPerlIR($jsonIR); Maybe $json should be $jsonDecoder or something? Ditto for line 52--jsonEncoder. This object is not JSON in any real sense. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:88: $firstKey = (keys %{$jsonIR})[1] if $firstKey eq "overloadedIndex"; Suggestion: Make a method to get the class name for a hash. Return false for non-objects. This will make the intent clearer, the surrounding code clearer, and may let you write a shorter comment. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:89: my $isObject = $firstKey =~ /::/; Nice use of explaining variable. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/g... File Source/bindings/scripts/generate-bindings.pl (right): https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:188: # FIXME: This code will be removed once IDLParser.pm and CodeGeneratorV8.pm Good. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:192: # Generate desired output for the target IDL file. Is it possible to chop this up with functions or modules so that we know exactly what data the following steps depends on? The above code sets up a bunch of state and that is intertwined with parsing, which is not what we want. I'm fine if that is in a follow-up changelist. But if so it would be nice to have a separate refactoring patch that focuses specifically on that.
https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... File Source/bindings/scripts/JSONSerializer.pm (right): https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:43: @EXPORT = qw(serializeJSON deserializeJSON); Do you need these exports? https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:57: # so can compare output between runs or between Perl and Python Regarding comments here and there, please use more formal English for readability. e.g. By default JSON.pm outputs keys of blessed references in an indetermininate order. We set options to change the default behavior. (I don't know detailed syntax issues. You must be familiar with correct English:-) https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:81: if (!scalar %{$jsonIR}) { my @keys = keys %$jsonIR; return {} if @keys == 0; https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:87: my $firstKey = (keys %{$jsonIR})[0]; $keys[0] https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:88: $firstKey = (keys %{$jsonIR})[1] if $firstKey eq "overloadedIndex"; $keys[1] https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:96: foreach my $key (keys %{$jsonIR}) { foreach my $key (@keys)
Re-revised! Haraken, thoughts on Dominic's suggestion to refactor generate-bindings.pl? I could do it in a single refactoring CL, which would be more incremental but wouldn't have any visible effects, or we could do it as part of breaking up the build into 2 steps, which would make that CL a bit bigger. Preference? https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... File Source/bindings/scripts/JSONSerializer.pm (right): https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:30: # Library to convert the Perl Intermediate Representation (IR) of IDLs On 2013/06/12 04:39:33, dominicc wrote: > "Perl Intermediate Representation (IR)" could be better. Rewritten, and renamed to remove redundant "IR" throughout. Yeah, "IR" got a bit tedious; it's really just changing representation (serialization/deserialization of a single data structure). The "decoded JSON" (back to Perl hashes, before rebuilding the Perl objects) is in a messy half-way state, so hard to give a good name (hence earlier "raw IR"). > For that matter, this isn't really a generic JSON serializer. Maybe this package > should be called "IDLSerializer." Renamed. Yeah, it's properly "Serialize/Deserialize Perl IR of IDLs to/from JSON"; IDLSerializer seems clear enough. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:43: @EXPORT = qw(serializeJSON deserializeJSON); On 2013/06/12 04:46:31, haraken wrote: > Do you need these exports? If it's a package, yes. I've removed "package" and thus exports for brevity. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:53: # JSON.pm defaults to barfing on objects (blessed references) and returning keys in indeterminate order; set options to change this: On 2013/06/12 04:39:33, dominicc wrote: > Be consistent--you don't wrap here but do on line 57 Done. (Consistently wrap multi-line.) https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:57: # so can compare output between runs or between Perl and Python On 2013/06/12 04:46:31, haraken wrote: > Regarding comments here and there, please use more formal English for > readability. (Reworded to change "barf" to "die", usual Perl term.) ? I don't see anything especially casual here, other than "barf", which is following the docs: http://search.cpan.org/~makamaka/JSON-2.59/lib/JSON.pm#allow_blessed Please always feel free to request clearer comments! (I don't intentionally use slang or idioms, but your take is v. helpful in finding out what's unclear.) https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:67: return jsonIRToPerlIR($jsonIR); On 2013/06/12 04:39:33, dominicc wrote: > Maybe $json should be $jsonDecoder or something? Ditto for line 52--jsonEncoder. > This object is not JSON in any real sense. I'm following usage in docs, which calls this object $json. We could call it $jsonEncoderDecoder, though that's a bit long. http://search.cpan.org/~makamaka/JSON-2.59/lib/JSON.pm#SYNOPSIS https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:81: if (!scalar %{$jsonIR}) { On 2013/06/12 04:46:31, haraken wrote: > my @keys = keys %$jsonIR; (>.<) Good point (changed here and below). https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:87: my $firstKey = (keys %{$jsonIR})[0]; On 2013/06/12 04:46:31, haraken wrote: > $keys[0] Done. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:88: $firstKey = (keys %{$jsonIR})[1] if $firstKey eq "overloadedIndex"; On 2013/06/12 04:46:31, haraken wrote: > $keys[1] Done. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:89: my $isObject = $firstKey =~ /::/; On 2013/06/12 04:39:33, dominicc wrote: > Nice use of explaining variable. Thanks! https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/J... Source/bindings/scripts/JSONSerializer.pm:96: foreach my $key (keys %{$jsonIR}) { On 2013/06/12 04:46:31, haraken wrote: > > foreach my $key (@keys) Done. https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/g... File Source/bindings/scripts/generate-bindings.pl (right): https://codereview.chromium.org/16296004/diff/29001/Source/bindings/scripts/g... Source/bindings/scripts/generate-bindings.pl:192: # Generate desired output for the target IDL file. On 2013/06/12 04:39:33, dominicc wrote: > Is it possible to chop this up with functions or modules so that we know exactly > what data the following steps depends on? The above code sets up a bunch of > state and that is intertwined with parsing, which is not what we want. Agreed. > I'm fine if that is in a follow-up changelist. But if so it would be nice to > have a separate refactoring patch that focuses specifically on that. I'd be happy to do a follow-up refactoring CL. Alternatively, we could do it when we change the build, as we'll be changing the flow then, and possibly breaking this script up into two pieces (which would disentangle these pretty clearly). Haraken?
LGTM from my side. Wait for comments from dominicc. > Haraken, thoughts on Dominic's suggestion to refactor generate-bindings.pl? > > I could do it in a single refactoring CL, which would be more incremental but > wouldn't have any visible effects, or we could do it as part of breaking up the > build into 2 steps, which would make that CL a bit bigger. > Preference? You don't need to refactor generate-bindings.pl since (1) generate-bindings.pl is going to be deprecated soon and (2) I'm planning to change the build flow completely (I'm just writing a document).
On 2013/06/12 09:23:56, haraken wrote: > LGTM from my side. Wait for comments from dominicc. Got it, will do. > > Haraken, thoughts on Dominic's suggestion to refactor generate-bindings.pl? > > You don't need to refactor generate-bindings.pl since > (1) generate-bindings.pl is going to be deprecated soon and Got it. > (2) I'm planning to change the build flow completely (I'm just writing a document). I'll be very interested to see what you come up with!
Informally LGTM if you address these nits. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... File Source/bindings/scripts/IDLSerializer.pm (right): https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:30: # Convert the intermediate representation of IDLs between Perl and JSON, for: Convert => Converts since this is just a library; "Convert" sounds very imperative. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:31: # 1. Modularity between frontend parser and backend code generator; and frontend parser => parser backend code generator => code generator I don't think there will be any confusion, and this matches the names of the modules that implement those parts more closely (IDLParser/CodeGeneratorV8). https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:32: # 2. Porting to Python, so can connect Perl scripts and Python scripts. "so can connect" is ungrammatical; maybe Piecemeal porting to Python, by letting us connect Perl and Python scripts. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:45: # JSON.pm defaults to dying on objects (blessed references) and returning Nice comment. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:67: # JSON.pm serializes Perl objects as a type of hash, so need to rebuild type of hash => hash is simpler and no less precise, right? https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:72: my $perlData = []; Ich nicht bin ein Perliner, but could this just be return map(jsonToPerl, @$jsonData); ?
Final tweaks. Passed final checks (same C++/headers code, some comment order changes), committing. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... File Source/bindings/scripts/IDLSerializer.pm (right): https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:30: # Convert the intermediate representation of IDLs between Perl and JSON, for: On 2013/06/12 13:39:53, dominicc wrote: > Convert => Converts > > since this is just a library; "Convert" sounds very imperative. Done. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:31: # 1. Modularity between frontend parser and backend code generator; and On 2013/06/12 13:39:53, dominicc wrote: > frontend parser => parser > backend code generator => code generator > > I don't think there will be any confusion, and this matches the names of the > modules that implement those parts more closely (IDLParser/CodeGeneratorV8). Done. (I've been using "Frontend" b/c in Python, we have a separate lexer, parser, and IDL Attribute Checker + partial interface merger, but these aren't distinguished in Perl, so no need for extra words.) https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:32: # 2. Porting to Python, so can connect Perl scripts and Python scripts. On 2013/06/12 13:39:53, dominicc wrote: > "so can connect" is ungrammatical; maybe > > Piecemeal porting to Python, by letting us connect Perl and Python scripts. Done. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:67: # JSON.pm serializes Perl objects as a type of hash, so need to rebuild On 2013/06/12 13:39:53, dominicc wrote: > type of hash => hash > > is simpler and no less precise, right? I've reworded; point is that it's a hash of a certain form (keys CLASS::KEY), so we recognize these and rebuild objects from them. https://codereview.chromium.org/16296004/diff/36001/Source/bindings/scripts/I... Source/bindings/scripts/IDLSerializer.pm:72: my $perlData = []; On 2013/06/12 13:39:53, dominicc wrote: > Ich nicht bin ein Perliner, but could this just be > > return map(jsonToPerl, @$jsonData); > > ? Good point, done! Perl being Perl, syntax is actually: return [map(jsonToPerl($_), @$jsonData)]; (return reference, specify argument) ...and there's no built-in equivalent for hashes, but makes this block shorter at least.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/16296004/44001
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/16296004/44001
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/16296004/44001
Message was sent while issue was closed.
Change committed as 152399 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
