Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23594008:
Initial code generation for features. (Closed)
|
Created:
7 years, 3 months ago by dhnishi (use Chromium) Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis is a preliminary patch to auto-generate the ID enum in APIPermission.
_permission_features.h: https://gist.github.com/DHNishi/2014be18e7912916ea25
_permission_features.cc: https://gist.github.com/DHNishi/b78bc9588b2d7a46331d
BUG=280286
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226826
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233214
Patch Set 1 : . #
Total comments: 54
Patch Set 2 : Addressing #2 #
Total comments: 16
Patch Set 3 : Addressing comments. #
Total comments: 20
Patch Set 4 : Addressing #6. #
Total comments: 6
Patch Set 5 : Tests and namespaces. #
Total comments: 3
Patch Set 6 : Adding tests for real. #
Total comments: 13
Patch Set 7 : Generate the class name. #
Total comments: 2
Patch Set 8 : Remove leading '_' and spacing fix. #
Total comments: 2
Patch Set 9 : Addressing comment #20. #
Total comments: 1
Patch Set 10 : Removed underscore stripping. #Patch Set 11 : Use newer SchemaLoader constructor. #Patch Set 12 : GYP/GYPI removed. #Patch Set 13 : Merged from Master. #Messages
Total messages: 41 (0 generated)
First revision is ready to be torn into.
excited https://codereview.chromium.org/23594008/diff/38001/chrome/common/extensions/... File chrome/common/extensions/api/feature.gyp (right): https://codereview.chromium.org/23594008/diff/38001/chrome/common/extensions/... chrome/common/extensions/api/feature.gyp:7: 'target_name': 'feature', it's simpler to just put this in with api.gyp with a :feature target_name. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/cpp_util.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/cpp_util.py:120: """Return a "set" of features where any feature with the same name is tossed. this method returns a list not a set. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:50: (c.Append('default: ') no default:, let compiler fail, which.. it shouldn't. have the NOTREACHED/return "" outside the switch. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:70: feature.constant_name)) prefer either: c.Append('if (id == "%s") return %s' % (feature.name, feature.constant_name)) or c.Append('if (id == "%s") return %s' % (feature.name, feature.constant_name)) https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:77: ) generate a std::map<std::string, ID> on construction and reference into that so this isn't looping through every feature. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:81: .Eblock(None, 1) # class PermissionFeatures not sure what this is https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:22: def GenerateSchema(filenames, root, destdir): Private, so _GenerateSchema. Add a '''Docstring. ''' https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:25: os.path.normpath(filenames[0]), root)), True) what is True here? make it a named parameter if anything... https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:26: schema = os.path.normpath(filenames[0]) pass schema into this function rather than filenames (and it makes sense to call it filename). also I don't know what advantage the os.path.normpath has. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:27: schema_filename, schema_extension = os.path.splitext(schema) schema_extension isn't used, so substitute _ https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:33: if type(feature_dict) is not list: a bit odd to be comparing a type of a variable called "feature_dict" to a list. anyhow, typically you would use isinstance(feature_dict, dict): here. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:38: feature_list.append(Feature(feature_def, single_def, schema)) here: feature_list += [Feature(feature_def, single_def, schema) for single_def in feature_dict] https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:42: full_path = relpath + "/%s" % schema os.path.join(relpath, schema) https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:74: sys.exit(0) # This is OK as a no-op I don't think this make sense here https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:79: "Only one file is allowed (for now).") single line https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:20: self._feature_defs = cpp_util.RemoveFeatureDuplicates(permission_defs) why are there duplicates? |features| is a simpler name https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:43: .Sblock(None, 1) rather than making changes to code.py and doing, this, do just (c.Append('class PermissionFeatures {') .Append(' public:') .Cblock(self._GenerateBody())) and about the self._GenerateBody() - it's nicer to read these Code blocks when composed together. So write a GenerateBody method which returns a Code. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:47: (c.Append('// Replaces APIPermission::ID') these comments were to you, I didn't mean to generate them as well https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:57: .Append() likewise, pull the enum list generation into another method and do like (c.Append('enum ID {') .Cblock(self._GenerateEnumConstants(self._feature_defs)) .Eblock('}') ) dunno, something like that. Can't remember which of the Block things you need. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:61: c.Append('static std::string ToString(ID id);') return const char* from these methods. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:67: .Eblock(None, 1) # class PermissionFeatures don't think you need these to be so complicated https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/model.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:38: """ A Feature """A feature, as specified in files such as chrome/common/extensions/api/_permission_features.json. """ https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:52: self.constant_name = self._constant_name(self.name) this is a property of the C++ code not of the abstract model https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:55: self.source_file_dir, self.source_file_filename = os.path.split(source_file) would be nice not to have source_file nor source_file_dir in this model either, but I know that unfortunately other Model objects do have that. Ideally... not, though. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:60: def _constant_name(self, target): (move this into cpp_util.py if something like that isn't already there) https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:521: no blank line?
https://codereview.chromium.org/23594008/diff/38001/chrome/common/extensions/... File chrome/common/extensions/api/feature.gyp (right): https://codereview.chromium.org/23594008/diff/38001/chrome/common/extensions/... chrome/common/extensions/api/feature.gyp:7: 'target_name': 'feature', On 2013/09/12 16:20:43, kalman wrote: > it's simpler to just put this in with api.gyp with a :feature target_name. Done. File removed and modified the api.gyp file instead. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/cpp_util.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/cpp_util.py:120: """Return a "set" of features where any feature with the same name is tossed. On 2013/09/12 16:20:43, kalman wrote: > this method returns a list not a set. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:50: (c.Append('default: ') On 2013/09/12 16:20:43, kalman wrote: > no default:, let compiler fail, which.. it shouldn't. have the NOTREACHED/return > "" outside the switch. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:70: feature.constant_name)) On 2013/09/12 16:20:43, kalman wrote: > prefer either: > > c.Append('if (id == "%s") return %s' % > (feature.name, feature.constant_name)) > > or > > c.Append('if (id == "%s") return %s' % (feature.name, > feature.constant_name)) Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:77: ) On 2013/09/12 16:20:43, kalman wrote: > generate a std::map<std::string, ID> on construction and reference into that so > this isn't looping through every feature. Done. I worried that I'm breaking design patterns with my implementation, though. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:81: .Eblock(None, 1) # class PermissionFeatures On 2013/09/12 16:20:43, kalman wrote: > not sure what this is Whoops. That was a remnant from an older revision. Removed. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:22: def GenerateSchema(filenames, root, destdir): On 2013/09/12 16:20:43, kalman wrote: > Private, so _GenerateSchema. > > Add a > > '''Docstring. > ''' Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:25: os.path.normpath(filenames[0]), root)), True) On 2013/09/12 16:20:43, kalman wrote: > what is True here? make it a named parameter if anything... The SchemaLoader by default doesn't allow multiple schemas to be loaded. We need this for the features gen, so I added a flag to allow it to happen. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:26: schema = os.path.normpath(filenames[0]) On 2013/09/12 16:20:43, kalman wrote: > pass schema into this function rather than filenames (and it makes sense to call > it filename). also I don't know what advantage the os.path.normpath has. If the path is coming in an oddball way with unnecessary ".."s and such, normpath strips them. Also, done. |schema| is now passed in, rather than normpathing twice. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:27: schema_filename, schema_extension = os.path.splitext(schema) On 2013/09/12 16:20:43, kalman wrote: > schema_extension isn't used, so substitute _ Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:33: if type(feature_dict) is not list: I'm having difficulty thinking of a good name for it, since it's sometimes a list and sometimes a dict. I switched it over to the generic name |feature| for now, but I'll keep thinking about it. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:38: feature_list.append(Feature(feature_def, single_def, schema)) On 2013/09/12 16:20:43, kalman wrote: > here: > > feature_list += [Feature(feature_def, single_def, schema) > for single_def in feature_dict] That looks cleaner. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:42: full_path = relpath + "/%s" % schema On 2013/09/12 16:20:43, kalman wrote: > os.path.join(relpath, schema) Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:74: sys.exit(0) # This is OK as a no-op On 2013/09/12 16:20:43, kalman wrote: > I don't think this make sense here I replaced it with a raise Exception, since it doesn't make much sense for the feature compiler to be called w/o any files to compile from. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:79: "Only one file is allowed (for now).") On 2013/09/12 16:20:43, kalman wrote: > single line Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:20: self._feature_defs = cpp_util.RemoveFeatureDuplicates(permission_defs) On 2013/09/12 16:20:43, kalman wrote: > why are there duplicates? > > |features| is a simpler name The same feature may be defined multiple times for different channels. When the permission features are read in, there are two "Audio" definitions, for example. |permission_defs| renamed to |features|. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:43: .Sblock(None, 1) On 2013/09/12 16:20:43, kalman wrote: > rather than making changes to code.py and doing, this, do just > > (c.Append('class PermissionFeatures {') > .Append(' public:') > .Cblock(self._GenerateBody())) > > and about the self._GenerateBody() - it's nicer to read these Code blocks when > composed together. So write a GenerateBody method which returns a Code. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:47: (c.Append('// Replaces APIPermission::ID') On 2013/09/12 16:20:43, kalman wrote: > these comments were to you, I didn't mean to generate them as well Whoops, haha. Removed. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:57: .Append() On 2013/09/12 16:20:43, kalman wrote: > likewise, pull the enum list generation into another method and do like > > (c.Append('enum ID {') > .Cblock(self._GenerateEnumConstants(self._feature_defs)) > .Eblock('}') > ) > > dunno, something like that. Can't remember which of the Block things you need. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:61: c.Append('static std::string ToString(ID id);') On 2013/09/12 16:20:43, kalman wrote: > return const char* from these methods. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:67: .Eblock(None, 1) # class PermissionFeatures On 2013/09/12 16:20:43, kalman wrote: > don't think you need these to be so complicated Removed the EBlock change. Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/model.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:38: """ A Feature On 2013/09/12 16:20:43, kalman wrote: > """A feature, as specified in files such as > chrome/common/extensions/api/_permission_features.json. > """ Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:52: self.constant_name = self._constant_name(self.name) On 2013/09/12 16:20:43, kalman wrote: > this is a property of the C++ code not of the abstract model Removed from the abstract model. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:55: self.source_file_dir, self.source_file_filename = os.path.split(source_file) On 2013/09/12 16:20:43, kalman wrote: > would be nice not to have source_file nor source_file_dir in this model either, > but I know that unfortunately other Model objects do have that. Ideally... not, > though. I don't think I had anything actually using those variables, so I'll remove it. I just had them there to mirror the other objects. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:60: def _constant_name(self, target): On 2013/09/12 16:20:43, kalman wrote: > (move this into cpp_util.py if something like that isn't already there) Done. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/model.py:521: On 2013/09/12 16:20:43, kalman wrote: > no blank line? Done.
https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:26: schema = os.path.normpath(filenames[0]) On 2013/09/13 17:36:42, Daniel Nishi wrote: > On 2013/09/12 16:20:43, kalman wrote: > > pass schema into this function rather than filenames (and it makes sense to > call > > it filename). also I don't know what advantage the os.path.normpath has. > > If the path is coming in an oddball way with unnecessary ".."s and such, > normpath strips them. Yeah - though I mean more like do things actually break if this happens. But, not a big point. Cleaner sounds nice. > > Also, done. |schema| is now passed in, rather than normpathing twice. https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/38001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:20: self._feature_defs = cpp_util.RemoveFeatureDuplicates(permission_defs) On 2013/09/13 17:36:42, Daniel Nishi wrote: > On 2013/09/12 16:20:43, kalman wrote: > > why are there duplicates? > > > > |features| is a simpler name > > The same feature may be defined multiple times for different channels. When the > permission features are read in, there are two "Audio" definitions, for example. > > |permission_defs| renamed to |features|. Ah right. So that's actually something that we need to support. Those are the "ComplexFeatures". A list of dicts isn't actually a set of different Features, it's a list of different configurations that single feature can have. So what you really need to do I think is update the Feature model to allow this. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/cpp_util.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/cpp_util.py:131: feature_set and not feature_set.add(feature.name)] this isn't c++ related, it's model related. however, it should also go away given my other comment. sorry for the miscommunication there. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:12: def Generate(self, permission_defs, source_file): you need to remove references to "permissions", they're generic feature files. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:45: 'singleton = new std::map<std::string, PermissionFeatures::ID>;') You can't do this - it's a memory leak. Usually in Chrome code if you need a singleton you use base::LazyInstance. However, you don't need that here. Make the map just a member variable of *Features. Let users of these files create base::LazyInstance<*Feature>s themselves. That involves making all of those methods non-static too, I suppose. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:36: feature_list.append(Feature(feature_def, feature, schema)) yeah - let Feature figure this out, since a list is actually just a single feature, just a more complex representation of one. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:38: ## The dict may contain list of features, not a single feature. # not ## https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:75: raise Exception("No files were specified for the generator.") ValueError https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:79: raise Exception("Only one file is allowed (for now).") ValueError. though that said you can combine both this check and the above with "len(filenames) == 1" https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/schema_loader.py:52: 'a single schema.' % schema) meh, just kill this check and don't pass in that boolean.
Gists updated as well. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/cpp_util.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/cpp_util.py:131: feature_set and not feature_set.add(feature.name)] On 2013/09/13 21:37:52, kalman wrote: > this isn't c++ related, it's model related. however, it should also go away > given my other comment. sorry for the miscommunication there. Done. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:12: def Generate(self, permission_defs, source_file): On 2013/09/13 21:37:52, kalman wrote: > you need to remove references to "permissions", they're generic feature files. Began phasing out references. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:45: 'singleton = new std::map<std::string, PermissionFeatures::ID>;') On 2013/09/13 21:37:52, kalman wrote: > You can't do this - it's a memory leak. Usually in Chrome code if you need a > singleton you use base::LazyInstance. > > However, you don't need that here. Make the map just a member variable of > *Features. Let users of these files create base::LazyInstance<*Feature>s > themselves. > > That involves making all of those methods non-static too, I suppose. Made all of the methods non-static in preparation. I don't follow the other part, though. You want each Feature (PermissionFeature, APIFeature, etc) would have its own map member variable? I wasn't quite sure what was meant, so I made a private map instead of the singleton pattern that was used before. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:36: feature_list.append(Feature(feature_def, feature, schema)) On 2013/09/13 21:37:52, kalman wrote: > yeah - let Feature figure this out, since a list is actually just a single > feature, just a more complex representation of one. The model now handles it. Not sure if this is preferred implementation, since I did notice it was done differently on the C++ side of things, but right now I just have ComplexFeatures as having a list of features. For simplification, I've just had the SimpleFeatures just be a ComplexFeature with a single item in its list. Would you like a more hard distinction between the two? https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:38: ## The dict may contain list of features, not a single feature. On 2013/09/13 21:37:52, kalman wrote: > # not ## Done. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:75: raise Exception("No files were specified for the generator.") On 2013/09/13 21:37:52, kalman wrote: > ValueError Merged the two error checks as per the other comment. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:79: raise Exception("Only one file is allowed (for now).") On 2013/09/13 21:37:52, kalman wrote: > ValueError. > > though that said you can combine both this check and the above with > "len(filenames) == 1" Done. https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23594008/diff/46001/tools/json_schema_compile... tools/json_schema_compiler/schema_loader.py:52: 'a single schema.' % schema) On 2013/09/13 21:37:52, kalman wrote: > meh, just kill this check and don't pass in that boolean. Done.
https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:45: c.Append('_featureMap.insert(std::pair<std::string, ' feature_map_[%s] = %s not feature_map_.insert(..) https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:47: .Concat(self._GeneratePublicBody()) IIRC, Sblock() Concat(...) Eblock() can be written as Cblock(...) https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:65: on need to break the chain here? https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:73: c.Append('const char* ToString(ID id);') this method can be const https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:76: (c.Append('ID FromString(const std::string& id);') so can this method https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:78: ) nor here and above. the comments are self-explanatory. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:84: c.Append('std::map<std::string, PermissionFeatures::ID> _featureMap;') features_ or feature_map_ not _featureMap. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:85: return c also this can be return Code().Append('...') https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... File tools/json_schema_compiler/model.py (right): https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/model.py:48: def __init__(self, feature_name, features, source_file): I thought you weren't using the source_file here anymore, same below? https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/model.py:52: if isinstance(features, dict): instead of this, have a factory method defined in here, like def CreateFeature(name, model): if isinstance(model, dict): return SimpleFeature(name, model) return ComplexFeature( name, [SimpleFeature(name, child) for child in model]) something like that. and call that from feature_compiler.
Gist updated as well. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:45: c.Append('_featureMap.insert(std::pair<std::string, ' On 2013/09/17 01:28:07, kalman wrote: > feature_map_[%s] = %s > > not > > feature_map_.insert(..) Done. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:47: .Concat(self._GeneratePublicBody()) On 2013/09/17 01:28:07, kalman wrote: > IIRC, > > Sblock() > Concat(...) > Eblock() > > can be written as > > Cblock(...) Cblock(...) concats another code object. I could do it that way, but I'd have to define another code and then pass it into Cblock -- I don't see any indentation levels in it, either, so I think I'd still need the S and E blocks. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:65: On 2013/09/17 01:28:07, kalman wrote: > on need to break the chain here? Break removed. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:73: c.Append('const char* ToString(ID id);') On 2013/09/17 01:28:07, kalman wrote: > this method can be const Changed the parameter to be const. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:76: (c.Append('ID FromString(const std::string& id);') On 2013/09/17 01:28:07, kalman wrote: > so can this method Method is now const. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:78: ) On 2013/09/17 01:28:07, kalman wrote: > nor here and above. the comments are self-explanatory. Removed the comments. Code seemed simple enough. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:84: c.Append('std::map<std::string, PermissionFeatures::ID> _featureMap;') On 2013/09/17 01:28:07, kalman wrote: > features_ or feature_map_ not _featureMap. Opted for features_ https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:85: return c On 2013/09/17 01:28:07, kalman wrote: > also this can be > > return Code().Append('...') Done. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... File tools/json_schema_compiler/model.py (right): https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/model.py:48: def __init__(self, feature_name, features, source_file): On 2013/09/17 01:28:07, kalman wrote: > I thought you weren't using the source_file here anymore, same below? That is correct. Extraneous parameter removed. https://codereview.chromium.org/23594008/diff/14011/tools/json_schema_compile... tools/json_schema_compiler/model.py:52: if isinstance(features, dict): On 2013/09/17 01:28:07, kalman wrote: > instead of this, have a factory method defined in here, like > > def CreateFeature(name, model): > if isinstance(model, dict): > return SimpleFeature(name, model) > return ComplexFeature( > name, [SimpleFeature(name, child) for child in model]) > > something like that. > > and call that from feature_compiler. Done.
This looks great, but you still need a test (sorry to have not mentioned this earlier). It can be really simple; use the same mechanism that the JSON schema compiler tests, namely add a test feature file, add a simple and a complex feature, add it to the test.gyp file (I think it's that?), do some simple FromString and ToString tests on it. https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:65: help='root directory to output generated files.') this also need a C++ namespace option similar to how the JSON schema compiler works, so that we can put these in the extensions:: namespace. https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:69: .Append('const char* ToString(const ID id);') const char* ToString(ID id) const; https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:70: .Append('const ID FromString(const std::string& id);') ID FromString(const std::string& id) const;
Tests have been added. The namespace feature has been ported from the JSON Schema Compiler. https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... File tools/json_schema_compiler/features_compiler.py (right): https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... tools/json_schema_compiler/features_compiler.py:65: help='root directory to output generated files.') On 2013/09/17 18:26:19, kalman wrote: > this also need a C++ namespace option similar to how the JSON schema compiler > works, so that we can put these in the extensions:: namespace. Done. https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... File tools/json_schema_compiler/features_h_generator.py (right): https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:69: .Append('const char* ToString(const ID id);') On 2013/09/17 18:26:19, kalman wrote: > const char* ToString(ID id) const; Done. https://codereview.chromium.org/23594008/diff/63001/tools/json_schema_compile... tools/json_schema_compiler/features_h_generator.py:70: .Append('const ID FromString(const std::string& id);') On 2013/09/17 18:26:19, kalman wrote: > ID FromString(const std::string& id) const; Done. https://codereview.chromium.org/23594008/diff/67001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/23594008/diff/67001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:190: 'root_namespace': 'extensions::feature', Is this okay as the namespace?
Did you forget to upload the tests? https://codereview.chromium.org/23594008/diff/67001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/23594008/diff/67001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:190: 'root_namespace': 'extensions::feature', On 2013/09/18 18:32:45, Daniel Nishi wrote: > Is this okay as the namespace? Yep - though maybe "features" not "feature".
could you call the gyp target name "features" as well?
Oops! I added the test for real now. https://codereview.chromium.org/23594008/diff/67001/chrome/common/extensions/... File chrome/common/extensions/api/api.gyp (right): https://codereview.chromium.org/23594008/diff/67001/chrome/common/extensions/... chrome/common/extensions/api/api.gyp:190: 'root_namespace': 'extensions::feature', On 2013/09/18 18:35:58, kalman wrote: > On 2013/09/18 18:32:45, Daniel Nishi wrote: > > Is this okay as the namespace? > > Yep - though maybe "features" not "feature". Done.
lgtm with a bit of test cleanup + remove the "permissions" references in the test files. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/permission_features_unittest.cc (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:5: #include <string> just features_unittest.cc https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:13: PermissionFeatures test_features = PermissionFeatures(); just PermissionFeatures test_features; same below. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:15: ASSERT_EQ(PermissionFeatures::kComplex, test_features.FromString("complex")); for all, prefer EXPECT over ASSERT. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:23: std::string(test_features.ToString(PermissionFeatures::kComplex))); EXPECT_STREQ, no std::string-ifying. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/test_permission_features.json (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/test_permission_features.json:5: { just test_features.json. No permissions.
As of right now, "permissions" is in the name of the generated file because it is based on the file name of the JSON that goes in. What would be a preferred file name for the generated files?
https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:17: """A .cc generator for PermissionFeatures. sorry I totally missed that there was still PermissionFeatures here. This compiler is going to be used for all 3 features files + who knows what in the future. You need to pull in the name of the class from the file name; turn _permission_features into PermissionFeatures, _api_features into ApiFeatures, etc.
https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:17: """A .cc generator for PermissionFeatures. On 2013/09/18 20:04:47, kalman wrote: > sorry I totally missed that there was still PermissionFeatures here. > > This compiler is going to be used for all 3 features files + who knows what in > the future. You need to pull in the name of the class from the file name; turn > _permission_features into PermissionFeatures, _api_features into ApiFeatures, > etc. For now, I've got it taking the path and turning it into a singular class name using a new function in the cpp_utils. Is it okay to have the leading underscore in the filename of the permission features files e.g. _permission_features.cc/_permission_features.h or should leading underscores be stripped? It's currently generating like that because it is modeling based on the JSON file's name. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/permission_features_unittest.cc (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:5: #include <string> On 2013/09/18 19:48:31, kalman wrote: > just features_unittest.cc Done. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:13: PermissionFeatures test_features = PermissionFeatures(); On 2013/09/18 19:48:31, kalman wrote: > just > > PermissionFeatures test_features; > > same below. Done. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:15: ASSERT_EQ(PermissionFeatures::kComplex, test_features.FromString("complex")); On 2013/09/18 19:48:31, kalman wrote: > for all, prefer EXPECT over ASSERT. Done. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/permission_features_unittest.cc:23: std::string(test_features.ToString(PermissionFeatures::kComplex))); On 2013/09/18 19:48:31, kalman wrote: > EXPECT_STREQ, no std::string-ifying. Done. https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/test/test_permission_features.json (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/test/test_permission_features.json:5: { On 2013/09/18 19:48:31, kalman wrote: > just test_features.json. No permissions. Done.
https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... File tools/json_schema_compiler/features_cc_generator.py (right): https://codereview.chromium.org/23594008/diff/70001/tools/json_schema_compile... tools/json_schema_compiler/features_cc_generator.py:17: """A .cc generator for PermissionFeatures. On 2013/09/18 22:00:23, Daniel Nishi wrote: > On 2013/09/18 20:04:47, kalman wrote: > > sorry I totally missed that there was still PermissionFeatures here. > > > > This compiler is going to be used for all 3 features files + who knows what in > > the future. You need to pull in the name of the class from the file name; turn > > _permission_features into PermissionFeatures, _api_features into ApiFeatures, > > etc. > > For now, I've got it taking the path and turning it into a singular class name > using a new function in the cpp_utils. > > Is it okay to have the leading underscore in the filename of the permission > features files e.g. _permission_features.cc/_permission_features.h or should > leading underscores be stripped? It's currently generating like that because it > is modeling based on the JSON file's name. yeah, strip the leading _.
https://codereview.chromium.org/23594008/diff/91001/tools/json_schema_compile... File tools/json_schema_compiler/cpp_util.py (right): https://codereview.chromium.org/23594008/diff/91001/tools/json_schema_compile... tools/json_schema_compiler/cpp_util.py:138: no \n at end of file; x2 \n between each function.
Now lstripping the '_' off of the filename. https://codereview.chromium.org/23594008/diff/91001/tools/json_schema_compile... File tools/json_schema_compiler/cpp_util.py (right): https://codereview.chromium.org/23594008/diff/91001/tools/json_schema_compile... tools/json_schema_compiler/cpp_util.py:138: On 2013/09/18 22:01:55, kalman wrote: > no \n at end of file; x2 \n between each function. Done.
lgtm https://codereview.chromium.org/23594008/diff/83002/tools/json_schema_compile... File tools/json_schema_compiler/test/features_unittest.cc (right): https://codereview.chromium.org/23594008/diff/83002/tools/json_schema_compile... tools/json_schema_compiler/test/features_unittest.cc:23: test_features.ToString(TestFeatures::kComplex)); these should be able to fit on 1 line
https://codereview.chromium.org/23594008/diff/83002/tools/json_schema_compile... File tools/json_schema_compiler/test/features_unittest.cc (right): https://codereview.chromium.org/23594008/diff/83002/tools/json_schema_compile... tools/json_schema_compiler/test/features_unittest.cc:23: test_features.ToString(TestFeatures::kComplex)); On 2013/09/18 22:19:33, kalman wrote: > these should be able to fit on 1 line It is done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/83003
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/83003
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... 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/23594008/diff/83003/build/features_compile.gypi File build/features_compile.gypi (right): https://codereview.chromium.org/23594008/diff/83003/build/features_compile.gy... build/features_compile.gypi:33: '<(SHARED_INTERMEDIATE_DIR)/<(cc_dir)/<(RULE_INPUT_ROOT).cc', The schema file used right now is the "_permission_features.json". The issue is that the Python strips the leading underscore, but I can't seem to find a way to coax GYP into stripping it, so it looks for a corresponding generated _permission_features.cc/h, doesn't find it, then fails the compile. My build compile was passing because it still had the "_permission_features" generated files from previous runs. I can think of a couple of solutions, but I'm not fond of any of them. 1. Fake the generated file -- if an underscore is stripped, create a dummy file with the underscore leading the file name in the generated folder. 2. Try to manually add in the outputs and not have it in there using the RULE_INPUT_*. 3. Do not strip the _ in the generated file.
> 3. Do not strip the _ in the generated file. Yeah, sounds the easiest.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/117001
Retried try job too often on win_rel for step(s) browser_tests, chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests, content_browsertests, mini_installer_test, nacl_integration, sync_integration_tests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/117001
Retried try job too often on linux_rel for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/168001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... 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/DHNishi@gmail.com/23594008/168001
Message was sent while issue was closed.
Change committed as 226826
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/201001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/490001
Retried try job too often on win_rel for step(s) browser_tests, content_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/DHNishi@gmail.com/23594008/490001
Message was sent while issue was closed.
Change committed as 233214 |
