Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
138003003:
Add additional restriction to policy schema internal (part1) (Closed)
|
Created:
6 years, 11 months ago by binjin Modified:
6 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd additional restriction to policy schema internal, includes modification to python generator and C++ parser with correspond unit tests.
BUG=258339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246266
Patch Set 1 : proposal for updates to schema internal structure #
Total comments: 6
Patch Set 2 : fix python generator and C++ parser #
Total comments: 54
Patch Set 3 : fixes #
Total comments: 22
Patch Set 4 : more fixes #
Messages
Total messages: 14 (0 generated)
Hello Joao, The fist patch is merely a modification proposal for updates to schema internal structure, while I'm still hacking on the python generator and C++ parser/verifier, can you please first review this? Bin
This looks good, go ahead with the generator and parser! I think it's easier to extend the generator first. Then look at SchemaTest.Wrap (in schema_unittest.cc) and write a similar unittest to verify its correctness. Writing the parser should be easier after this is done, IMO. https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... File components/policy/core/common/schema_internal.h (right): https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:74: // Represent the limitation on TYPE_INTEGER or TYPE_STRING instance of *Represents https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:88: // For integer type only, represents that all values between |min_value| nit: add a newline after }; https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:89: // and |max_value| can be choosed. Note that integer type in base::Value *chosen
Hello Joao, Here's my current implementation to add support for new entries in schema internals in python generator and C++ parser. PTAL Bin https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... File components/policy/core/common/schema_internal.h (right): https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:74: // Represent the limitation on TYPE_INTEGER or TYPE_STRING instance of On 2014/01/14 10:50:41, Joao da Silva wrote: > *Represents Done. https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:88: // For integer type only, represents that all values between |min_value| On 2014/01/14 10:50:41, Joao da Silva wrote: > nit: add a newline after }; Done. https://codereview.chromium.org/138003003/diff/1/components/policy/core/commo... components/policy/core/common/schema_internal.h:89: // and |max_value| can be choosed. Note that integer type in base::Value On 2014/01/14 10:50:41, Joao da Silva wrote: > *chosen Done.
Nice work! Comments for improvements inline; with a couple more tests this should be good to go. Please send the next patch set to the try bots: # git cl try https://codereview.chromium.org/138003003/diff/60001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/138003003/diff/60001/components/json_schema/j... components/json_schema/json_schema_validator.cc:168: entry->type == base::Value::TYPE_DOUBLE)) { This is correct, but add an extra pair of parenthesis to make it more obvious: if (!(it.value().IsType(entry->type) || (it.value().IsType(base::Value::TYPE_INTEGER) && entry->type == base::Value::TYPE_DOUBLE))) { ... } I'm surprised this compiled for you, because the compiler gives a warning and warnings are treated as failures. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:321: const base::ListValue* possible_values; = NULL https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:384: return false; This is ignoring the 'minimum' and 'maximum' fields for non-integer types; it should produce an error instead. Try this: } else if (schema.HasKey(kMinimum) || schema.HasKey(kMaximum) { if (type != TYPE_INTEGER) { *error = "Only integers can have minimum and maximum"; return false; } ... } https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:465: const base::ListValue *possible_values; = NULL https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:470: if (possible_values->GetSize() == 0) { possible_values->empty() https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:474: int offset_begin, offset_end; One declaration per line https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:479: it != possible_values->end(); ++it) { One more space of indentation https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:486: offset_begin = static_cast<int>(int_enums_.size()); offset_end https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:491: it != possible_values->end(); ++it) { One more space of indentation https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:502: return false; 2 spaces less in indentation https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:515: int min_value = INT_MIN, max_value = INT_MAX; One declaration per line https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:522: *error = "Invalid Range restriction for int type."; range (lower R) https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:326: TEST(SchemaTest, Wrap) { Add enums and ranges to this test too https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:318: offset_begin = len(self.int_enums) Add a check at the beginning: assert all(type(x) == int for x in schema['enum']) https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:318: offset_begin = len(self.int_enums) You had a nice idea about converting enums into ranges when possible. How about this: enums = sorted(schema['enums']) if max(enums) - min(enums) + 1 == len(enums): index = self.AppendRestriction(min(enums), max(enums)) return self.AppendSchema('TYPE_INTEGER', index, name) (see below about the "name" variable) https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:323: 'integer with enumeration restriction') Use "name" (from Generate()) for the 3rd argument (which is the comment in the output). You'll have to pass it as an argument to this function https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:326: offset_begin = len(self.string_enums) Add a check at the beginning: assert all(type(x) == str for x in schema['enum'] https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:331: 'string with enumeration restriction') Use "name" from Generate() for the 3rd argument https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:335: raise RuntiemError('Empty enumeration in %s' % schema['name']) RuntimeError schema['name'] does not exist; you have to pass "name" from Generate into this function. You can also use assertions for this: assert len(schema['enum']) > 0, 'Empty enumeration in %s' % name https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:341: raise RuntimeError('Unknown enumeration type in %s' % schema['name']) schema['name'] does not exist https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:345: raise RuntimeError('Unknown ranged type in %s' % schema['name']) schema['name' does not exist; pass "name" from Generate to this function https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:354: raise RuntimeError('Invalid ranged type in %s' % schema['name']) schema['name'] does not exist https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:360: 'integer with ranged restriction') Use "name" for the 3rd argument https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:445: f.write('{{%s,%s}},\n' % (first, second)) Use spacing and a header comment like the arrays above, so that the output is more readable https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:450: f.write('%d,\n' % possible_values) Indent with 2 spaces https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:455: f.write('%s\n' % self.GetString(possible_values)) Indent with 2 spaces https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:464: ' kStringEnumerations,\n' Compilation fails when an array is empty. So if any of these is empty this must be set to NULL instead. You should also test the size of the arrays before outputting them above. That happens when compiling on platforms that don't include any of the enum/range policies (e.g. on iOS). All platforms use the other structures, so that's why they don't have similar tests.
Hello Joao, I fixed the issues according to your comments, PTAL Bin https://codereview.chromium.org/138003003/diff/60001/components/json_schema/j... File components/json_schema/json_schema_validator.cc (right): https://codereview.chromium.org/138003003/diff/60001/components/json_schema/j... components/json_schema/json_schema_validator.cc:168: entry->type == base::Value::TYPE_DOUBLE)) { On 2014/01/16 10:56:34, Joao da Silva wrote: > This is correct, but add an extra pair of parenthesis to make it more obvious: > > if (!(it.value().IsType(entry->type) || > (it.value().IsType(base::Value::TYPE_INTEGER) && > entry->type == base::Value::TYPE_DOUBLE))) { > ... > } > > I'm surprised this compiled for you, because the compiler gives a warning and > warnings are treated as failures. Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:321: const base::ListValue* possible_values; On 2014/01/16 10:56:34, Joao da Silva wrote: > = NULL Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:384: return false; On 2014/01/16 10:56:34, Joao da Silva wrote: > This is ignoring the 'minimum' and 'maximum' fields for non-integer types; it > should produce an error instead. Try this: > > } else if (schema.HasKey(kMinimum) || schema.HasKey(kMaximum) { > if (type != TYPE_INTEGER) { > *error = "Only integers can have minimum and maximum"; > return false; > } > ... > } Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:465: const base::ListValue *possible_values; On 2014/01/16 10:56:34, Joao da Silva wrote: > = NULL Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:470: if (possible_values->GetSize() == 0) { On 2014/01/16 10:56:34, Joao da Silva wrote: > possible_values->empty() Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:474: int offset_begin, offset_end; On 2014/01/16 10:56:34, Joao da Silva wrote: > One declaration per line Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:479: it != possible_values->end(); ++it) { On 2014/01/16 10:56:34, Joao da Silva wrote: > One more space of indentation Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:486: offset_begin = static_cast<int>(int_enums_.size()); On 2014/01/16 10:56:34, Joao da Silva wrote: > offset_end Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:491: it != possible_values->end(); ++it) { On 2014/01/16 10:56:34, Joao da Silva wrote: > One more space of indentation Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:502: return false; On 2014/01/16 10:56:34, Joao da Silva wrote: > 2 spaces less in indentation Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:515: int min_value = INT_MIN, max_value = INT_MAX; On 2014/01/16 10:56:34, Joao da Silva wrote: > One declaration per line Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema.cc:522: *error = "Invalid Range restriction for int type."; On 2014/01/16 10:56:34, Joao da Silva wrote: > range (lower R) Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/138003003/diff/60001/components/policy/core/c... components/policy/core/common/schema_unittest.cc:326: TEST(SchemaTest, Wrap) { On 2014/01/16 10:56:34, Joao da Silva wrote: > Add enums and ranges to this test too Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:318: offset_begin = len(self.int_enums) On 2014/01/16 10:56:34, Joao da Silva wrote: > You had a nice idea about converting enums into ranges when possible. How about > this: > > enums = sorted(schema['enums']) > if max(enums) - min(enums) + 1 == len(enums): > index = self.AppendRestriction(min(enums), max(enums)) > return self.AppendSchema('TYPE_INTEGER', index, name) > > (see below about the "name" variable) Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:318: offset_begin = len(self.int_enums) On 2014/01/16 10:56:34, Joao da Silva wrote: > Add a check at the beginning: > > assert all(type(x) == int for x in schema['enum']) Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:323: 'integer with enumeration restriction') On 2014/01/16 10:56:34, Joao da Silva wrote: > Use "name" (from Generate()) for the 3rd argument (which is the comment in the > output). You'll have to pass it as an argument to this function Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:326: offset_begin = len(self.string_enums) On 2014/01/16 10:56:34, Joao da Silva wrote: > Add a check at the beginning: > > assert all(type(x) == str for x in schema['enum'] Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:331: 'string with enumeration restriction') On 2014/01/16 10:56:34, Joao da Silva wrote: > Use "name" from Generate() for the 3rd argument Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:335: raise RuntiemError('Empty enumeration in %s' % schema['name']) On 2014/01/16 10:56:34, Joao da Silva wrote: > RuntimeError > > schema['name'] does not exist; you have to pass "name" from Generate into this > function. > > You can also use assertions for this: > > assert len(schema['enum']) > 0, 'Empty enumeration in %s' % name Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:341: raise RuntimeError('Unknown enumeration type in %s' % schema['name']) On 2014/01/16 10:56:34, Joao da Silva wrote: > schema['name'] does not exist Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:345: raise RuntimeError('Unknown ranged type in %s' % schema['name']) On 2014/01/16 10:56:34, Joao da Silva wrote: > schema['name' does not exist; pass "name" from Generate to this function Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:354: raise RuntimeError('Invalid ranged type in %s' % schema['name']) On 2014/01/16 10:56:34, Joao da Silva wrote: > schema['name'] does not exist Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:360: 'integer with ranged restriction') On 2014/01/16 10:56:34, Joao da Silva wrote: > Use "name" for the 3rd argument Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:445: f.write('{{%s,%s}},\n' % (first, second)) On 2014/01/16 10:56:34, Joao da Silva wrote: > Use spacing and a header comment like the arrays above, so that the output is > more readable Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:450: f.write('%d,\n' % possible_values) On 2014/01/16 10:56:34, Joao da Silva wrote: > Indent with 2 spaces Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:455: f.write('%s\n' % self.GetString(possible_values)) On 2014/01/16 10:56:34, Joao da Silva wrote: > Indent with 2 spaces Done. https://codereview.chromium.org/138003003/diff/60001/components/policy/tools/... components/policy/tools/generate_policy_source.py:464: ' kStringEnumerations,\n' On 2014/01/16 10:56:34, Joao da Silva wrote: > Compilation fails when an array is empty. So if any of these is empty this must > be set to NULL instead. You should also test the size of the arrays before > outputting them above. > > That happens when compiling on platforms that don't include any of the > enum/range policies (e.g. on iOS). All platforms use the other structures, so > that's why they don't have similar tests. Done.
One suggestion to share identical ranges, then extend the test schema a bit and fix some little nits and should be ready to go. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:51: " }" Add integer with enum, integer with range and string with enums to this test schema. Then the SchemaTest.Validate() test below will not enforce it but will verify that enums and ranges don't break the validation of existing schemas. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:64: std::string schemaObjectWrapper(const std::string &subschema) { SchemaObjectWrapper Also, put the & next to the type: const std::string& subschema https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:348: { "StrEnum", 9 } // 7 Leave a comma after the last } https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:354: { 0, 8, 6 }, Update the comment: 0 to 8 (exclusive) are the known properties in kPropertyNodes, and 6 is the addionalProperties node. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:360: {{0, 3}} // ["one", "two", "three"] Comma after last element https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:368: "three" Comma after last element https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:377: kStringEnums Comma after last element https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:296: return index If you look at the output you'll see that there are duplicated range restrictions, because many enums are [0, 1, 2] or many policies have only "minimum: 0". For those cases we can reuse the same RestrictionNode. Suggestion: - add a list of ranges that already exist to self in __init__ above: self.ranges = {} - if (first, second) already exists in self.ranges then reuse that index: r = (str(first), str(second)) if not r in self.ranges: self.ranges[r] = len(self.restriction_nodes) self.restriction_nodes.append(r) return self.ranges[r] https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:317: def isConsecutiveInterval(self, seq): Start the function name with a capital letter: def IsConsecutiveInterval https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:319: return True This special case is not needed; xrange(0) will be empty, and all([]) is True. https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:330: 'integer with enumeration restriction(use ranged instead): %s' % name) nit: space before (, "used range instead":
Hello Joao, Here is the fixes, for one of them I left comments instead. PTAL Bin https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:51: " }" On 2014/01/20 10:49:10, Joao da Silva wrote: > Add integer with enum, integer with range and string with enums to this test > schema. > > Then the SchemaTest.Validate() test below will not enforce it but will verify > that enums and ranges don't break the validation of existing schemas. I prefer to leave this to the coming CL as work on Validate() is part of that CL. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:64: std::string schemaObjectWrapper(const std::string &subschema) { On 2014/01/20 10:49:10, Joao da Silva wrote: > SchemaObjectWrapper > > Also, put the & next to the type: > > const std::string& subschema Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:348: { "StrEnum", 9 } // 7 On 2014/01/20 10:49:10, Joao da Silva wrote: > Leave a comma after the last } Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:354: { 0, 8, 6 }, On 2014/01/20 10:49:10, Joao da Silva wrote: > Update the comment: 0 to 8 (exclusive) are the known properties in > kPropertyNodes, and 6 is the addionalProperties node. Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:360: {{0, 3}} // ["one", "two", "three"] On 2014/01/20 10:49:10, Joao da Silva wrote: > Comma after last element Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:368: "three" On 2014/01/20 10:49:10, Joao da Silva wrote: > Comma after last element Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/core/... components/policy/core/common/schema_unittest.cc:377: kStringEnums On 2014/01/20 10:49:10, Joao da Silva wrote: > Comma after last element Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:296: return index On 2014/01/20 10:49:10, Joao da Silva wrote: > If you look at the output you'll see that there are duplicated range > restrictions, because many enums are [0, 1, 2] or many policies have only > "minimum: 0". For those cases we can reuse the same RestrictionNode. Suggestion: > > - add a list of ranges that already exist to self in __init__ above: > > self.ranges = {} > > - if (first, second) already exists in self.ranges then reuse that index: > > r = (str(first), str(second)) > if not r in self.ranges: > self.ranges[r] = len(self.restriction_nodes) > self.restriction_nodes.append(r) > return self.ranges[r] Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:317: def isConsecutiveInterval(self, seq): On 2014/01/20 10:49:10, Joao da Silva wrote: > Start the function name with a capital letter: > > def IsConsecutiveInterval Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:319: return True On 2014/01/20 10:49:10, Joao da Silva wrote: > This special case is not needed; xrange(0) will be empty, and all([]) is True. Done. https://codereview.chromium.org/138003003/diff/160001/components/policy/tools... components/policy/tools/generate_policy_source.py:330: 'integer with enumeration restriction(use ranged instead): %s' % name) On 2014/01/20 10:49:10, Joao da Silva wrote: > nit: space before (, "used range instead": Done.
lgtm!
+Benjamin for the json_schema change
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/138003003/300001
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/binjin@chromium.org/138003003/300001
Message was sent while issue was closed.
Change committed as 246266 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
