Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11377049:
Moving prepopulated search engines to a JSON file. (Closed)
|
Created:
8 years, 1 month ago by beaudoin Modified:
8 years, 1 month ago CC:
chromium-reviews, pam+watch_chromium.org, M-A Ruel Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMoving prepopulated search engines to a JSON file.
This CL also includes the python script required to convert the JSON file to a .cc/.h pair. The generated .cc/.h are not generated by the build process and must be committed to the repository.
BUG=159990
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167793
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168430
Patch Set 1 #
Total comments: 9
Patch Set 2 : Answered review comments. #
Total comments: 8
Patch Set 3 : Ready for thorough review #
Total comments: 62
Patch Set 4 : Unit tests for python and C++. Added build step. #
Total comments: 26
Patch Set 5 : Answered review comments. #
Total comments: 8
Patch Set 6 : Minor clean-up and more robust unit test. #
Total comments: 1
Patch Set 7 : Fixed failing build on windows and Peter's nits. #Patch Set 8 : Fix for failure on win and cros builders. #Patch Set 9 : Getting rid of windows backslashes this time. #Patch Set 10 : Rebased to fix conflict. #Messages
Total messages: 36 (0 generated)
Hi Peter! Here is an initial version of the script, JSON and generated .cc/.h to replace the manually maintained list const PrepopulatedEngine in template_url_prepopulate_data.cc. The goal of this CR is mostly to expose my approach and get your feedback on it, that's why only 2 search engines have been converted. If you think it's OK overall, don't review too thoroughly, I'll make a major documentation/cleanup pass. For tracking purposes, TODO: - Cleanup/document python code - Document script usage (where? In template_url_prepopulate_data.cc?) - Move all the PrepopulatedEngine to JSON via some automated process. - Move PrepopulatedEngine alternate_urls to an array and drop the JSON parsing in template_url_prepopulate_data.cc.
On 2012/11/08 05:46:04, beaudoin wrote: > Hi Peter! > > Here is an initial version of the script, JSON and generated .cc/.h to replace > the manually maintained list const PrepopulatedEngine in > template_url_prepopulate_data.cc. > > The goal of this CR is mostly to expose my approach and get your feedback on it, > that's why only 2 search engines have been converted. If you think it's OK > overall, don't review too thoroughly, I'll make a major documentation/cleanup > pass. > > For tracking purposes, TODO: > - Cleanup/document python code > - Document script usage (where? In template_url_prepopulate_data.cc?) > - Move all the PrepopulatedEngine to JSON via some automated process. > - Move PrepopulatedEngine alternate_urls to an array and drop the JSON parsing > in template_url_prepopulate_data.cc. Also TODO: - Write tests for the python script.
I'd like to see what Google looks like converted as well. You should probably move the version number into the JSON file alongside the engine definitions. I didn't look at the Python code since I don't speak Python. I think the readability and size savings are somehow less than I was envisioning, so I'm not as enthusiastic about this as I'd hoped, but maybe it's still a good idea. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/prepopulated_engines.json (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/prepopulated_engines.json:5: // This file is used by json_to_struct.py to generate prepopulated_engined.h/cc. Typo http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/prepopulated_engines.json:16: "encoding": "UTF-8", Let's make this field default to UTF-8 since that's what almost everyone wants. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/prepopulated_engines_schema.json:53: // 91, 92, 93, 94, 95, 96, 97, 98, 99, 102+ Seems like this list should be in the other JSON file http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url_prepopulate_data.cc:37: using namespace search_engines; This is banned by the style guide
Hi Peter, (At a conf. today, no time for code so only comments for now.) I still think it's worth going forward with the JSON conversion. I believe it will make maintaining the data and upgrading the model much easier in the future. Removing unnamed, order-dependent fields also reduces the risk of error. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url_prepopulate_data.cc:37: using namespace search_engines; On 2012/11/08 19:02:25, Peter Kasting wrote: > This is banned by the style guide I didn't like it either. Other options I can think of: - Use the namespace qualifier everywhere. Drawback: it will bloat this file. - Change the namespace to a class, make the variables static, move the content of this file to the class itself. Drawback: it will change this file significantly. - Generate more than only the PrepopulatedEngine from JSON. (ie. the engines_* arrays). Drawback: more complex python code.
The ultimate goal is long-term readability and maintainability. So "big change from what we do now" isn't important, and "Python code is complex" is important only insofar as we actually need to touch it -- which is presumably much less often than we need to touch the JSON files. I don't know which solution that argues for, but those are the principles I'd keep in mind.
On 2012/11/08 19:42:40, Peter Kasting wrote: > The ultimate goal is long-term readability and maintainability. So "big change > from what we do now" isn't important, and "Python code is complex" is important > only insofar as we actually need to touch it -- which is presumably much less > often than we need to touch the JSON files. > > I don't know which solution that argues for, but those are the principles I'd > keep in mind. I think it argue for wrapping all that in a class, as long as it's OK to have some of the class "implementation" in the generated prepopulated_engine.cc and some in tempate_url_prepopulate_data.cc. I say "implementation" because the only thing in prepopulated_engine.cc will be static variables initialization.
Here's a second version that answer your review comments. Since you seemed to agree with the direction I've taken I'll go ahead and wrap-up the patch. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/prepopulated_engines.json (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/prepopulated_engines.json:5: // This file is used by json_to_struct.py to generate prepopulated_engined.h/cc. On 2012/11/08 19:02:25, Peter Kasting wrote: > Typo Done. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/prepopulated_engines.json:16: "encoding": "UTF-8", On 2012/11/08 19:02:25, Peter Kasting wrote: > Let's make this field default to UTF-8 since that's what almost everyone wants. Done. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/prepopulated_engines_schema.json:53: // 91, 92, 93, 94, 95, 96, 97, 98, 99, 102+ On 2012/11/08 19:02:25, Peter Kasting wrote: > Seems like this list should be in the other JSON file Done. http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/11377049/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url_prepopulate_data.cc:37: using namespace search_engines; On 2012/11/08 19:39:01, beaudoin wrote: > On 2012/11/08 19:02:25, Peter Kasting wrote: > > This is banned by the style guide > > I didn't like it either. Other options I can think of: > - Use the namespace qualifier everywhere. Drawback: it will bloat this file. > - Change the namespace to a class, make the variables static, move the content > of this file to the class itself. Drawback: it will change this file > significantly. > - Generate more than only the PrepopulatedEngine from JSON. (ie. the engines_* > arrays). Drawback: more complex python code. > Found an elegant fix by using the same namespace for the generated file as for this one.
http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... File chrome/browser/search_engines/prepopulated_engines.json (right): http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines.json:23: // Increment this if you change the above data in ways that mean users with Nit: This comment shouldn't say "above data" http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines.json:50: "search_url": "{google:baseURL}search?q={searchTerms}&{google:RLZ} {google:acceptedSuggestion}{google:originalQueryForSuggestion}{google:assistedQueryStats}{google:searchFieldtrialParameter}sourceid=chrome&ie={inputEncoding}", The space here is erroneous http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines.json:53: "alternate_urls": "[\"{google:baseURL}#q={searchTerms}\", \"{google:baseURL}search#q={searchTerms}\", \"{google:baseURL}webhp#q={searchTerms}\"]", Having to escape the quotes here sucks, can we avoid that somehow? http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines_schema.json:28: // Use SEARCH_ENGINE_OTHER if there is no matching type. Nit: Weird to comment this but not comment the UTF-8 one above. I'd probably just remove this.
This latest patch answers your review comments, including moving the alternate_urls to a JSON array. It also moves all the PrepopulatedEngine to the JSON file. It is ready for thorough review. (I'll find a Python reviewer.) TODO: Check if tests fail and fix them. http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... File chrome/browser/search_engines/prepopulated_engines.json (right): http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines.json:23: // Increment this if you change the above data in ways that mean users with On 2012/11/09 22:07:23, Peter Kasting wrote: > Nit: This comment shouldn't say "above data" Done. http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines.json:50: "search_url": "{google:baseURL}search?q={searchTerms}&{google:RLZ} {google:acceptedSuggestion}{google:originalQueryForSuggestion}{google:assistedQueryStats}{google:searchFieldtrialParameter}sourceid=chrome&ie={inputEncoding}", On 2012/11/09 22:07:23, Peter Kasting wrote: > The space here is erroneous Done. http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines.json:53: "alternate_urls": "[\"{google:baseURL}#q={searchTerms}\", \"{google:baseURL}search#q={searchTerms}\", \"{google:baseURL}webhp#q={searchTerms}\"]", On 2012/11/09 22:07:23, Peter Kasting wrote: > Having to escape the quotes here sucks, can we avoid that somehow? Yes. It's actually one benefit of that approach: The "alternate_urls" is now a static C array so we don't have nested JSON and the JSON parsing is all done before run-time. Done. http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): http://codereview.chromium.org/11377049/diff/7003/chrome/browser/search_engin... chrome/browser/search_engines/prepopulated_engines_schema.json:28: // Use SEARCH_ENGINE_OTHER if there is no matching type. On 2012/11/09 22:07:23, Peter Kasting wrote: > Nit: Weird to comment this but not comment the UTF-8 one above. I'd probably > just remove this. Done.
@ben For chrome/chrome_browser.gypi Hi Benjamin, I've added you as a reviewer because I saw you contributed to the python code for the json_schema_compiler. Do you think you could review the python code in this CL? If not, can you recommend someone? Thanks!
lgtm for gyp
Sure. Here is first pass, just some basic things with overall questions. http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/prepopulated_engines.json (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.json:6: // Any time you modify this file regenerate the .h/.cc. From src/ call: Is there some reason you can't generate those files as part of the build step, rather than needing to check in generated files? We do that in extensions for the JSON Schema compiler (see build/json_schema_compile.gypi and chrome/common/extensions/api/api.gyp) http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.json:30: "int_variables": { Does order matter? In which case you'll need to do a bit more work so that Python uses an ordered dict when it reads in the JSON. http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines_schema.json:15: { "field": "name", "type": "string16" }, Have you considered using JSON schema for this? Changing this to use it would be a bit of make-work but it'd be consistent with the other place we do JSON based code generation in chromium (at least, the places I am aware of). We have a parsing library in tools/json_schema_compiler/model.py. This file basically maps onto JSON schema: type_name is namespace. ctype is compiled_type (our own thing). Etc. There is no equivalent of "headers" but you could pull it out separately, or consider making it part of the gyp generation. http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:34: #include "prepopulated_engines.h" out of curiosity: why not chrome/browser/seatch_engines/prepopulated_engines.h? http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... File tools/json_to_struct/element_generator.py (right): http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:8: def JSONToCString16(json_string_literal): Private functions prefix with _. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:10: done by converting \\u#### to \\x####. Can you use a regex? http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:23: def GenerateInt(field_info, content, lines): likewise, consider generating and returning Code objects, then composing them from the callers. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:66: var_counter = 0 If this is necessary, prefer to make the element generator a class which maintains its own state. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:78: var = 'temp_%s' % var_counter Can you use the name of the fields as a variable name rather than temp values? It would make the generated code easier to read. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:117: content = element[field_info['field']] content = elemtent.get(field_info['field'], None) rather than the KeyError stuff http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... File tools/json_to_struct/json_to_struct.py (right): http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:55: sys.path.insert(0, os.path.normpath(_script_path + "/../../")) I think it'd be polite to remove this from the path after importing the required modules. Inside try-finally preferably. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:60: HEAD = """// Copyright (c) %d The Chromium Authors. All rights reserved. I thought we weren't generating the (c) anymore? http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:71: def GenerateHeaderGuard(h_filename): Private functions should begin with an underscore, presumably that's all of the functions in this file. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:93: header_guard = GenerateHeaderGuard(h_filename) You might find the Code class in tools/json_schema_compiler/code.py useful. It's line-based, does indentation, 80 character limiting for comments (etc), and it's composable. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:181: sys.exit(0) # This is OK as a no-op why? when can this happen? http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:197: GenerateH(output_root, head, opts.namespace, schema, description) Passing head into these methods seems unnecessary, they could call a function like GenerateCopyright themselves. http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/struc... File tools/json_to_struct/struct_generator.py (right): http://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/struc... tools/json_to_struct/struct_generator.py:5: def GenerateIntField(field_info): private functions and variables prefix with _, use Code, etc.
http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/prepopulated_engines.cc (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.cc:10: #include <stdio.h> Why is this needed? http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.cc:18: const PrepopulatedEngine bing_bg_BG = { Nit: The ordering here seems to be random, neither alphabetical by engine name nor ordered by increasing engine unique ID. Can we put things here in some consistent order so it's easier for a human to scan the generated file for errors or inconsistencies? http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/prepopulated_engines.json (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.json:27: // in stats. This comment is now wrong and should refer to your variable below. http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.json:195: "id": 7 // Can't be 3 as this has to appear in the Arabian countries' lists Nit: Comments should not go > 80 chars http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.json:858: "{google:baseURL}#q={searchTerms}", Nit: This uses 4-space indent, you use 2-space elsewhere http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines_schema.json:17: // If omitted, there is no favicon. Do any of our engines not have a favicon? If not let's omit this comment so this doesn't appear to be optional. If we don't already, we probably could/should make the code generator check that every engine definition contains all the non-optional fields. http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:34: #include "prepopulated_engines.h" On 2012/11/12 18:38:13, kalman wrote: > out of curiosity: why not chrome/browser/seatch_engines/prepopulated_engines.h? Yeah, that's probably an error actually. http://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:1195: sizeof(engine.alternate_urls[0]); ++i) { Can we use arraysize() somehow instead? Maybe we need to declare the field as an array of pointers instead of a pointer to pointers?
Answered comments, added python and C++ unit tests, added a build step so prepopulated_engines.h/cc are gone. @ben Will need more thorough review of *.gyp? for the new build step. Not entirely sure I did that right. (Works though. ;)) +maruel Who helped me with some of the hairy python stuff. (Thanks!) https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines.cc (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.cc:10: #include <stdio.h> On 2012/11/12 22:46:36, Peter Kasting wrote: > Why is this needed? For NULL. :) Is there a better place to get it? Do I drop it use 0? Define it myself? https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.cc:18: const PrepopulatedEngine bing_bg_BG = { On 2012/11/12 22:46:36, Peter Kasting wrote: > Nit: The ordering here seems to be random, neither alphabetical by engine name > nor ordered by increasing engine unique ID. > > Can we put things here in some consistent order so it's easier for a human to > scan the generated file for errors or inconsistencies? Following discussion: compile-time generation removes the need for this. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.json:6: // Any time you modify this file regenerate the .h/.cc. From src/ call: On 2012/11/12 18:38:13, kalman wrote: > Is there some reason you can't generate those files as part of the build step, > rather than needing to check in generated files? We do that in extensions for > the JSON Schema compiler (see build/json_schema_compile.gypi and > chrome/common/extensions/api/api.gyp) Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.json:27: // in stats. On 2012/11/12 22:46:36, Peter Kasting wrote: > This comment is now wrong and should refer to your variable below. It's not the same variable, but I've moved the other one here too. Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.json:30: "int_variables": { On 2012/11/12 18:38:13, kalman wrote: > Does order matter? In which case you'll need to do a bit more work so that > Python uses an ordered dict when it reads in the JSON. I tried doing that but could not pull it off in Python 2.6. The OrderedDict was introduced in 2.7 as well as a key parameter to json.loads(). https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.json:195: "id": 7 // Can't be 3 as this has to appear in the Arabian countries' lists On 2012/11/12 22:46:36, Peter Kasting wrote: > Nit: Comments should not go > 80 chars Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.json:858: "{google:baseURL}#q={searchTerms}", On 2012/11/12 22:46:36, Peter Kasting wrote: > Nit: This uses 4-space indent, you use 2-space elsewhere Wow! You have bionic eyes. Nice catch. :) Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines_schema.json:15: { "field": "name", "type": "string16" }, On 2012/11/12 18:38:13, kalman wrote: > Have you considered using JSON schema for this? Changing this to use it would be > a bit of make-work but it'd be consistent with the other place we do JSON based > code generation in chromium (at least, the places I am aware of). > > We have a parsing library in tools/json_schema_compiler/model.py. This file > basically maps onto JSON schema: type_name is namespace. ctype is compiled_type > (our own thing). Etc. > > There is no equivalent of "headers" but you could pull it out separately, or > consider making it part of the gyp generation. I considered it but decided against it because of the large overhead it would have imposed in my code, and because my schemas only allow for C fundamental data types whereas the json_schema_compiler allows std::string and more. For simplicity, I would argue the proposed approach is better. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines_schema.json:17: // If omitted, there is no favicon. On 2012/11/12 22:46:36, Peter Kasting wrote: > Do any of our engines not have a favicon? If not let's omit this comment so > this doesn't appear to be optional. Most engines in the "UMA-only engines" do not define a favicon. > If we don't already, we probably could/should make the code generator check that > every engine definition contains all the non-optional fields. I've added a way to define optional fields and an explicit check. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/template_url_prepopulate_data.cc:34: #include "prepopulated_engines.h" On 2012/11/12 18:38:13, kalman wrote: > out of curiosity: why not chrome/browser/seatch_engines/prepopulated_engines.h? Because mistake! Thanks for catching it! :) Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/template_url_prepopulate_data.cc:34: #include "prepopulated_engines.h" On 2012/11/12 22:46:36, Peter Kasting wrote: > On 2012/11/12 18:38:13, kalman wrote: > > out of curiosity: why not > chrome/browser/seatch_engines/prepopulated_engines.h? > > Yeah, that's probably an error actually. Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/template_url_prepopulate_data.cc:1195: sizeof(engine.alternate_urls[0]); ++i) { On 2012/11/12 22:46:36, Peter Kasting wrote: > Can we use arraysize() somehow instead? Maybe we need to declare the field as > an array of pointers instead of a pointer to pointers? Unfortunately it wont let me declare it as an array of pointers unless it's the last element in the structure, and then it cannot be initialized with NULL. This would make the generator much less general and the code relatively complex. In fact, I've discovered (thanks unit tests!) that my way of counting the number of elements in the array always returns 1. Instead, I'm now having the Python generate an extra field containing the size of the array. (Alternate option is generating a NULL-terminated array but it would only works for pointer types. Not sure the extra comlexity is worth the marginal gain in memory footprint.) Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... File tools/json_to_struct/element_generator.py (right): https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... tools/json_to_struct/element_generator.py:8: def JSONToCString16(json_string_literal): On 2012/11/12 18:38:13, kalman wrote: > Private functions prefix with _. I've also elected to make some other function private. Not sure if I should try to lock this even more? Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... tools/json_to_struct/element_generator.py:10: done by converting \\u#### to \\x####. On 2012/11/12 18:38:13, kalman wrote: > Can you use a regex? I thought about it and could not find a way that would guarantee I catch \u but not \\u. Negative lookbehind does not work as it would fail catching \\\u. I think this is the best solution. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... tools/json_to_struct/element_generator.py:23: def GenerateInt(field_info, content, lines): On 2012/11/12 18:38:13, kalman wrote: > likewise, consider generating and returning Code objects, then composing them > from the callers. Thought about this too, but didn't want to reorganize the tools/ directory when what I needed was readily obtained via '\n'.join() https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... tools/json_to_struct/element_generator.py:66: var_counter = 0 On 2012/11/12 18:38:13, kalman wrote: > If this is necessary, prefer to make the element generator a class which > maintains its own state. No longer necessary. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... tools/json_to_struct/element_generator.py:78: var = 'temp_%s' % var_counter On 2012/11/12 18:38:13, kalman wrote: > Can you use the name of the fields as a variable name rather than temp values? > It would make the generated code easier to read. Excellent idea. It forced me to disallow nested arrays (otherwise you would get the same variable multiple times) but we don't need them so I think it's OK. There is still the risk that a duplicate variable name exists, but the mangling should reduce that. Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/elem... tools/json_to_struct/element_generator.py:117: content = element[field_info['field']] On 2012/11/12 18:38:13, kalman wrote: > content = elemtent.get(field_info['field'], None) rather than the KeyError stuff Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... File tools/json_to_struct/json_to_struct.py (right): https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... tools/json_to_struct/json_to_struct.py:55: sys.path.insert(0, os.path.normpath(_script_path + "/../../")) On 2012/11/12 18:38:13, kalman wrote: > I think it'd be polite to remove this from the path after importing the required > modules. Inside try-finally preferably. Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... tools/json_to_struct/json_to_struct.py:60: HEAD = """// Copyright (c) %d The Chromium Authors. All rights reserved. On 2012/11/12 18:38:13, kalman wrote: > I thought we weren't generating the (c) anymore? Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... tools/json_to_struct/json_to_struct.py:71: def GenerateHeaderGuard(h_filename): On 2012/11/12 18:38:13, kalman wrote: > Private functions should begin with an underscore, presumably that's all of the > functions in this file. Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... tools/json_to_struct/json_to_struct.py:93: header_guard = GenerateHeaderGuard(h_filename) On 2012/11/12 18:38:13, kalman wrote: > You might find the Code class in tools/json_schema_compiler/code.py useful. It's > line-based, does indentation, 80 character limiting for comments (etc), and it's > composable. Yeah, as noted elsewhere, I thought about using it but my use case what much simpler (no >80 comments, no >2 space indents) so I went with the simple '\n'.join() if you really thing there is much to be gained by going to Code it can be done, but will require moving the code.py class to the tools/ directory (like json_comment_eater.py). https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... tools/json_to_struct/json_to_struct.py:181: sys.exit(0) # This is OK as a no-op On 2012/11/12 18:38:13, kalman wrote: > why? when can this happen? Good question. Cut-and-pasted from the json_schema_compiler. :) Got rid of it... Done. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/json... tools/json_to_struct/json_to_struct.py:197: GenerateH(output_root, head, opts.namespace, schema, description) On 2012/11/12 18:38:13, kalman wrote: > Passing head into these methods seems unnecessary, they could call a function > like GenerateCopyright themselves. That GenerateCopyright function would need the opts.schema and description_filename which would need to be passed down GenerateH and GenerateCC. I went with the approach of passing head instead. Let me know if you really prefer the other way. https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/stru... File tools/json_to_struct/struct_generator.py (right): https://codereview.chromium.org/11377049/diff/12001/tools/json_to_struct/stru... tools/json_to_struct/struct_generator.py:5: def GenerateIntField(field_info): On 2012/11/12 18:38:13, kalman wrote: > private functions and variables prefix with _, use Code, etc. Done. (I think :))
(One more small comment/question to Peter regarding NULL + size_t.) https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines.cc (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.cc:10: #include <stdio.h> On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 22:46:36, Peter Kasting wrote: > > Why is this needed? > > For NULL. :) > > Is there a better place to get it? Do I drop it use 0? Define it myself? Also: now it's probably needed for size_t too -- so it should probably be included in the .h. What do you think?
https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines.cc (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.cc:10: #include <stdio.h> On 2012/11/13 18:51:13, beaudoin wrote: > On 2012/11/13 18:44:26, beaudoin wrote: > > On 2012/11/12 22:46:36, Peter Kasting wrote: > > > Why is this needed? > > > > For NULL. :) > > > > Is there a better place to get it? Do I drop it use 0? Define it myself? > > Also: now it's probably needed for size_t too -- so it should probably be > included in the .h. What do you think? If you need these and they aren't already included, #include <cstddef> to get them. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines_schema.json:17: // If omitted, there is no favicon. On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 22:46:36, Peter Kasting wrote: > > Do any of our engines not have a favicon? If not let's omit this comment so > > this doesn't appear to be optional. > > Most engines in the "UMA-only engines" do not define a favicon. Hmmm. I'm torn between making them define favicons too (it wouldn't be hard) so that we can require this, or leaving things as they are and adding a comment saying that if you're not declaring an UMA-only engine you need to have this. The former is more work for data we're not going to use anyway, but safer. WDYT?
Generally speaking: prefer OO style than util-method style. It's easier to see how components fit together. https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... File chrome/browser/search_engines/prepopulated_engines.cc (right): https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... chrome/browser/search_engines/prepopulated_engines.cc:10: #include <stdio.h> On 2012/11/13 19:23:10, Peter Kasting wrote: > On 2012/11/13 18:51:13, beaudoin wrote: > > On 2012/11/13 18:44:26, beaudoin wrote: > > > On 2012/11/12 22:46:36, Peter Kasting wrote: > > > > Why is this needed? > > > > > > For NULL. :) > > > > > > Is there a better place to get it? Do I drop it use 0? Define it myself? > > > > Also: now it's probably needed for size_t too -- so it should probably be > > included in the .h. What do you think? > > If you need these and they aren't already included, #include <cstddef> to get > them. Apologies for the intrusion, but I've seen people #including base/basictypes.h many times in Chromium code: > gg 'include "base/basictypes' | wc -l 3318 Is that the idiom you're looking for? https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... File chrome/browser/search_engines/prepopulated_engines.json (right): https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... chrome/browser/search_engines/prepopulated_engines.json:30: "int_variables": { On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 18:38:13, kalman wrote: > > Does order matter? In which case you'll need to do a bit more work so that > > Python uses an ordered dict when it reads in the JSON. > > I tried doing that but could not pull it off in Python 2.6. The OrderedDict was > introduced in 2.7 as well as a key parameter to json.loads(). Bummer (I only really meant it as an observation - I'm not close enough to this to know whether order does actually matter - though if it does I guess you'd need to turn it into a list rather than a dictionary). https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... chrome/browser/search_engines/prepopulated_engines_schema.json:15: { "field": "name", "type": "string16" }, On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 18:38:13, kalman wrote: > > Have you considered using JSON schema for this? Changing this to use it would > be > > a bit of make-work but it'd be consistent with the other place we do JSON > based > > code generation in chromium (at least, the places I am aware of). > > > > We have a parsing library in tools/json_schema_compiler/model.py. This file > > basically maps onto JSON schema: type_name is namespace. ctype is > compiled_type > > (our own thing). Etc. > > > > There is no equivalent of "headers" but you could pull it out separately, or > > consider making it part of the gyp generation. > > I considered it but decided against it because of the large overhead it would > have imposed in my code, and because my schemas only allow for C fundamental > data types whereas the json_schema_compiler allows std::string and more. For > simplicity, I would argue the proposed approach is better. I don't know about large overhead? It's just a matter of reusing model.py. That doesn't really have any knowledge of C++ so you're free to use string16 rather than std::string or whatever. Anyway, I guess I don't really feel strongly. I'd argue (as I did before) that it's nice to not reinvent a JSON meta-language and use JSON Schema, which we already have tools built around, but happy to go with your judgement here. https://chromiumcodereview.appspot.com/11377049/diff/12001/tools/json_to_stru... File tools/json_to_struct/element_generator.py (right): https://chromiumcodereview.appspot.com/11377049/diff/12001/tools/json_to_stru... tools/json_to_struct/element_generator.py:23: def GenerateInt(field_info, content, lines): On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 18:38:13, kalman wrote: > > likewise, consider generating and returning Code objects, then composing them > > from the callers. > > Thought about this too, but didn't want to reorganize the tools/ directory when > what I needed was readily obtained via '\n'.join() Why do you need to reorganize the tools directory? Can't you just include it the way you include the other tools, by augmenting sys.path? The Code object really is useful, mostly just because it's composable and leads the code generators toward a nice structure. And as I mentioned, it's much less error prone given it does automatic indentation. Plus has nice features like block-level % substitution. https://chromiumcodereview.appspot.com/11377049/diff/12001/tools/json_to_stru... File tools/json_to_struct/json_to_struct.py (right): https://chromiumcodereview.appspot.com/11377049/diff/12001/tools/json_to_stru... tools/json_to_struct/json_to_struct.py:93: header_guard = GenerateHeaderGuard(h_filename) On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 18:38:13, kalman wrote: > > You might find the Code class in tools/json_schema_compiler/code.py useful. > It's > > line-based, does indentation, 80 character limiting for comments (etc), and > it's > > composable. > > Yeah, as noted elsewhere, I thought about using it but my use case what much > simpler (no >80 comments, no >2 space indents) so I went with the simple > '\n'.join() if you really thing there is much to be gained by going to Code it > can be done, but will require moving the code.py class to the tools/ directory > (like json_comment_eater.py). See previous comment. Why does it require that? https://chromiumcodereview.appspot.com/11377049/diff/12001/tools/json_to_stru... tools/json_to_struct/json_to_struct.py:197: GenerateH(output_root, head, opts.namespace, schema, description) On 2012/11/13 18:44:26, beaudoin wrote: > On 2012/11/12 18:38:13, kalman wrote: > > Passing head into these methods seems unnecessary, they could call a function > > like GenerateCopyright themselves. > > That GenerateCopyright function would need the opts.schema and > description_filename which would need to be passed down GenerateH and > GenerateCC. I went with the approach of passing head instead. Let me know if you > really prefer the other way. My apologies, I didn't see that. Different comment, and I alluded to this in another file: rather than threading values through static function calls it's nice to bundle those up in a class and call simple methods on them. E.g. (_Generator(output_root, opts.namespace, schema, description) .GenerateH() .GenerateCC()) That would let you define GenerateCopyright on that object, etc. https://chromiumcodereview.appspot.com/11377049/diff/3003/build/json_to_struc... File build/json_to_struct.gypi (right): https://chromiumcodereview.appspot.com/11377049/diff/3003/build/json_to_struc... build/json_to_struct.gypi:8: # json_schema_file: a json file that comprise the structure model. Heh, well unless you're actually planning to make this JSON schema, perhaps you should change these :) https://chromiumcodereview.appspot.com/11377049/diff/3003/chrome/browser/sear... File chrome/browser/search_engines/prepopulated_engines.json (right): https://chromiumcodereview.appspot.com/11377049/diff/3003/chrome/browser/sear... chrome/browser/search_engines/prepopulated_engines.json:23: nit: blank line (sorry) https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... File tools/json_to_struct/element_generator.py (right): https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/element_generator.py:38: content = field_info['default'] content = field_info.get('default', 'NULL') rather than keyerror? https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/element_generator.py:54: except KeyError: etc It would actually be nice not to need to repeat all this code and have the callee of this functions do this "content is None" business. See comment below. https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/element_generator.py:72: lines.append(' 0,') # Size of the array. nit: return, no else. https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/element_generator.py:101: } I think that using this is obfuscating the code in _GenerateFieldContent, over that just comparing to the type. It might make the structure of those _Generate functions less contrived. https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/element_generator.py:133: except KeyError: keyerror etc https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... File tools/json_to_struct/json_to_struct.py (right): https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/json_to_struct.py:62: sys.path = _old_path nit: rather than storing _old_path use sys.path.pop(0). https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/json_to_struct.py:108: except KeyError: use schema.get('headers', []) rather than keyerror. https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/json_to_struct.py:124: except KeyError: use description.get('int_variables', []) rather than keyerror. https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/json_to_struct.py:161: f.write(element_generator.GenerateElements(schema['type_name'], This distinction between the "struct generator" and the "element generator" is confusing me. I can't parse the difference between them. Can we do what JSON schema compiler does, and partition these into a "h generator" and a "cc generator", and if they have common components then write classes for those. https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... File tools/json_to_struct/struct_generator.py (right): https://chromiumcodereview.appspot.com/11377049/diff/3003/tools/json_to_struc... tools/json_to_struct/struct_generator.py:41: } same comment as earlier.
https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): https://chromiumcodereview.appspot.com/11377049/diff/12001/chrome/browser/sea... chrome/browser/search_engines/prepopulated_engines_schema.json:15: { "field": "name", "type": "string16" }, On 2012/11/13 20:28:07, kalman wrote: > On 2012/11/13 18:44:26, beaudoin wrote: > > On 2012/11/12 18:38:13, kalman wrote: > > > Have you considered using JSON schema for this? Changing this to use it > would > > be > > > a bit of make-work but it'd be consistent with the other place we do JSON > > based > > > code generation in chromium (at least, the places I am aware of). > > > > > > We have a parsing library in tools/json_schema_compiler/model.py. This file > > > basically maps onto JSON schema: type_name is namespace. ctype is > > compiled_type > > > (our own thing). Etc. > > > > > > There is no equivalent of "headers" but you could pull it out separately, or > > > consider making it part of the gyp generation. > > > > I considered it but decided against it because of the large overhead it would > > have imposed in my code, and because my schemas only allow for C fundamental > > data types whereas the json_schema_compiler allows std::string and more. For > > simplicity, I would argue the proposed approach is better. > > I don't know about large overhead? It's just a matter of reusing model.py. That > doesn't really have any knowledge of C++ so you're free to use string16 rather > than std::string or whatever. > > Anyway, I guess I don't really feel strongly. I'd argue (as I did before) that > it's nice to not reinvent a JSON meta-language and use JSON Schema, which we > already have tools built around, but happy to go with your judgement here. Wild speculative thought: another option would be to augment JSON schema compiler (as the IDL work did) to add the generators that you need.
Getting closer to wrapping this one! ben@ : Still missing a rubberstamp for the new compile step. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines.cc (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines.cc:10: #include <stdio.h> On 2012/11/13 19:23:10, Peter Kasting wrote: > On 2012/11/13 18:51:13, beaudoin wrote: > > On 2012/11/13 18:44:26, beaudoin wrote: > > > On 2012/11/12 22:46:36, Peter Kasting wrote: > > > > Why is this needed? > > > > > > For NULL. :) > > > > > > Is there a better place to get it? Do I drop it use 0? Define it myself? > > > > Also: now it's probably needed for size_t too -- so it should probably be > > included in the .h. What do you think? > > If you need these and they aren't already included, #include <cstddef> to get > them. Switched to <cstddef> and moved to .h for size_t. (Include what you need...) Done. https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... File chrome/browser/search_engines/prepopulated_engines_schema.json (right): https://codereview.chromium.org/11377049/diff/12001/chrome/browser/search_eng... chrome/browser/search_engines/prepopulated_engines_schema.json:17: // If omitted, there is no favicon. On 2012/11/13 19:23:10, Peter Kasting wrote: > On 2012/11/13 18:44:26, beaudoin wrote: > > On 2012/11/12 22:46:36, Peter Kasting wrote: > > > Do any of our engines not have a favicon? If not let's omit this comment so > > > this doesn't appear to be optional. > > > > Most engines in the "UMA-only engines" do not define a favicon. > > Hmmm. I'm torn between making them define favicons too (it wouldn't be hard) so > that we can require this, or leaving things as they are and adding a comment > saying that if you're not declaring an UMA-only engine you need to have this. > The former is more work for data we're not going to use anyway, but safer. > WDYT? Added the missing favicons and made it mandatory. One of the UMA-only already had a favicon, so I think it makes sense, and it reduces the risk of future errors... Done. https://codereview.chromium.org/11377049/diff/3003/build/json_to_struct.gypi File build/json_to_struct.gypi (right): https://codereview.chromium.org/11377049/diff/3003/build/json_to_struct.gypi#... build/json_to_struct.gypi:8: # json_schema_file: a json file that comprise the structure model. On 2012/11/13 20:28:07, kalman wrote: > Heh, well unless you're actually planning to make this JSON schema, perhaps you > should change these :) I'm actually using a schema -- just my own format. :) You want me to change the variable name? https://codereview.chromium.org/11377049/diff/3003/chrome/browser/search_engi... File chrome/browser/search_engines/prepopulated_engines.json (right): https://codereview.chromium.org/11377049/diff/3003/chrome/browser/search_engi... chrome/browser/search_engines/prepopulated_engines.json:23: On 2012/11/13 20:28:07, kalman wrote: > nit: blank line (sorry) Don't be. :) Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/eleme... File tools/json_to_struct/element_generator.py (right): https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:38: content = field_info['default'] On 2012/11/13 20:28:07, kalman wrote: > content = field_info.get('default', 'NULL') rather than keyerror? Thanks. First Python patch so I have likely missed a couple of idioms. :) Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:54: except KeyError: On 2012/11/13 20:28:07, kalman wrote: > etc > > It would actually be nice not to need to repeat all this code and have the > callee of this functions do this "content is None" business. See comment below. Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:72: lines.append(' 0,') # Size of the array. On 2012/11/13 20:28:07, kalman wrote: > nit: return, no else. Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:101: } On 2012/11/13 20:28:07, kalman wrote: > I think that using this is obfuscating the code in _GenerateFieldContent, over > that just comparing to the type. It might make the structure of those _Generate > functions less contrived. Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:133: except KeyError: On 2012/11/13 20:28:07, kalman wrote: > keyerror etc Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/json_... File tools/json_to_struct/json_to_struct.py (right): https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:62: sys.path = _old_path On 2012/11/13 20:28:07, kalman wrote: > nit: rather than storing _old_path use sys.path.pop(0). /facepalm :) Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:108: except KeyError: On 2012/11/13 20:28:07, kalman wrote: > use schema.get('headers', []) rather than keyerror. Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:124: except KeyError: On 2012/11/13 20:28:07, kalman wrote: > use description.get('int_variables', []) rather than keyerror. Done. https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/json_... tools/json_to_struct/json_to_struct.py:161: f.write(element_generator.GenerateElements(schema['type_name'], On 2012/11/13 20:28:07, kalman wrote: > This distinction between the "struct generator" and the "element generator" is > confusing me. I can't parse the difference between them. > > Can we do what JSON schema compiler does, and partition these into a "h > generator" and a "cc generator", and if they have common components then write > classes for those. Following chat discussion: we'll keep it like this for now. Opened a bug for an eventual merge/clean-up: crbug.com/160809 https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/struc... File tools/json_to_struct/struct_generator.py (right): https://codereview.chromium.org/11377049/diff/3003/tools/json_to_struct/struc... tools/json_to_struct/struct_generator.py:41: } On 2012/11/13 20:28:07, kalman wrote: > same comment as earlier. Done.
lgtm https://codereview.chromium.org/11377049/diff/3003/build/json_to_struct.gypi File build/json_to_struct.gypi (right): https://codereview.chromium.org/11377049/diff/3003/build/json_to_struct.gypi#... build/json_to_struct.gypi:8: # json_schema_file: a json file that comprise the structure model. On 2012/11/13 21:42:04, beaudoin wrote: > On 2012/11/13 20:28:07, kalman wrote: > > Heh, well unless you're actually planning to make this JSON schema, perhaps > you > > should change these :) > > I'm actually using a schema -- just my own format. :) You want me to change the > variable name? Yes I think that would be less confusing. :) perhaps just schema_file (the fact that the schema is JSON is an implementation detail, in this case) https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... File tools/json_to_struct/element_generator.py (right): https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:13: i = 0 More descriptive name than "i"? Perhaps escape_index? Also might prefer a structure like escape_index = json_string_literal.find('\\') while escape_index >= 0: ... https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:14: while 1: (True) https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:16: if i == -1: break (break on its own line)
LGTM with caveat https://codereview.chromium.org/11377049/diff/1011/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): https://codereview.chromium.org/11377049/diff/1011/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:183: TEST(TemplateURLPrepopulateDataTest, ProvidersFromPrepopulated) { This test is really fragile and will break every time we touch the Google entry, which is frequently. I'd actually rather have no test than this test. If we have a test, it should be very general, e.g. ensure that there are more than zero engines defined, and they all have names and search URLs. Something basic like that.
https://codereview.chromium.org/11377049/diff/3003/build/json_to_struct.gypi File build/json_to_struct.gypi (right): https://codereview.chromium.org/11377049/diff/3003/build/json_to_struct.gypi#... build/json_to_struct.gypi:8: # json_schema_file: a json file that comprise the structure model. On 2012/11/13 22:12:05, kalman wrote: > On 2012/11/13 21:42:04, beaudoin wrote: > > On 2012/11/13 20:28:07, kalman wrote: > > > Heh, well unless you're actually planning to make this JSON schema, perhaps > > you > > > should change these :) > > > > I'm actually using a schema -- just my own format. :) You want me to change > the > > variable name? > > Yes I think that would be less confusing. :) perhaps just schema_file (the fact > that the schema is JSON is an implementation detail, in this case) Done. https://codereview.chromium.org/11377049/diff/1011/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): https://codereview.chromium.org/11377049/diff/1011/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:183: TEST(TemplateURLPrepopulateDataTest, ProvidersFromPrepopulated) { On 2012/11/13 22:42:45, Peter Kasting wrote: > This test is really fragile and will break every time we touch the Google entry, > which is frequently. > > I'd actually rather have no test than this test. If we have a test, it should > be very general, e.g. ensure that there are more than zero engines defined, and > they all have names and search URLs. Something basic like that. I've kept the test but made it much less brittle. This is the one that caught my bug about the alternate_urls size being wrong, and this could be a really hard one to catch/witness otherwise. I've removed tests for things that were already verified by the python unit tests. I still check the default is "Google" because it's the only engine with some alternate_urls and if we ever change the default engine for the US I'd rather have a failure for "Google" than for the number of alternate_urls. https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... File tools/json_to_struct/element_generator.py (right): https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:13: i = 0 On 2012/11/13 22:12:05, kalman wrote: > More descriptive name than "i"? Perhaps escape_index? > > Also might prefer a structure like > > escape_index = json_string_literal.find('\\') > while escape_index >= 0: > ... Done. https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:14: while 1: On 2012/11/13 22:12:05, kalman wrote: > (True) Gone. https://codereview.chromium.org/11377049/diff/1011/tools/json_to_struct/eleme... tools/json_to_struct/element_generator.py:16: if i == -1: break On 2012/11/13 22:12:05, kalman wrote: > (break on its own line) Gone.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11377049/10019
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/11377049/diff/10019/chrome/browser/search_eng... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): https://codereview.chromium.org/11377049/diff/10019/chrome/browser/search_eng... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:194: ASSERT_GT(t_urls.size(), 0u); Nit: Or ASSERT_FALSE(t_urls.empty()), and similar on all the checks below.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11377049/15020
Change committed as 167793
On 2012/11/15 00:37:35, I haz the power (commit-bot) wrote: > Change committed as 167793 Fix: The original patch created ugly #ifndef header guards that did not respect chromium style guidelines. On some platforms, these header guards contained characters that are not allowed inside an #ifndef. The fix cleans up considerably the header guard by separating the "base path" from the main "file path". Only the latter is used in creating the header guard.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11377049/19001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11377049/18007
Retried try job too often for step(s) sync_integration_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11377049/21003
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11377049/21003
Change committed as 168430 |
