Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
134153005:
Add strictness to Schema::Validate() (Closed)
|
Created:
6 years, 11 months ago by binjin Modified:
6 years, 11 months ago Reviewers:
Joao da Silva CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@expand-policy-schema-2 Visibility:
Public. |
DescriptionAdd strictness to Schema::Validate()
WIP
BUG=258339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246770
Patch Set 1 : initial version of Schema::Validate() with strictness #Patch Set 2 : minor fixes #Patch Set 3 : pull from previous CL #
Total comments: 34
Patch Set 4 : fixes #Patch Set 5 : fix a bug and enhance tests #
Total comments: 37
Patch Set 6 : more fixes, add integer-to-double promotion #Patch Set 7 : fix style #
Total comments: 10
Patch Set 8 : minor fixes #
Messages
Total messages: 12 (0 generated)
Hello Joao, Here is the initial version for 3rd CL. PTAL Bin
https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:638: *error = "Fatal error: The schema itself is invalid."; Just "The schema is invalid." https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:643: *error = "The schema type and value type is not matched."; The value type doesn't match the schema type. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:661: } else { The else part should just be: std::string sub_error; if (!subschema.Validate(it.value(), StrategyForNextLevel(strategy), &sub_error) { if (!StrategyAllowInvalidOnTopLevel(strategy)) { *error = sub_error; return false; } } Is there a reason why you look at the subschema.node_->type below? I may be missing something. Note that the subtype may be a list with dictionaries inside, so you shouldn't pass STRICT directly, because we may allow invalid or unknown in those inner dictionaries. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:681: if (!*it || !GetItems().Validate(**it, strategy, error)) Replace "strategy" with StrategyForNextLevel(strategy). The list may contain dictionaries. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:705: *error = "Fatal error: The schema itself is invalid."; Just "The schema is invalid." https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:710: *error = "The schema type and value type is not matched."; The value type doesn't match the schema type. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:754: } This needs to handle lists of dictionaries; the inner dictionaries must be normalized too. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:30: SCHEMA_VERY_STRICT = 0, just SCHEMA_STRICT. Add a documenting comment: // No errors will be allowed. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:31: SCHEMA_ALLOW_UNKNOWN_TOPLEVEL, Add a comment: // Unknown properties in the top-level dictionary will be ignored. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:32: SCHEMA_ALLOW_UNKNOWN, Add a comment: // Unknown properties in any dictionary will be ignored. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:33: SCHEMA_ALLOW_INVALID_TOPLEVEL, Add a comment: // Mismatched values will be ignored at the toplevel. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:34: SCHEMA_ALLOW_INVALID_TOPLEVEL_AND_ALLOW_UNKNOWN, Add a comment: // Mismatched values will be ignored at the top-level dictionary. // Unknown properties in any dictionary will be ignored. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:35: SCHEMA_ALLOW_INVALID, Add a comment: // Mismatched values will be ignored. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:76: // |error| will contain a message state the detailed reason. |error| will contain the detailed reason. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:84: // returned and |error| will contain a message state the detailed message. returned and |error| will contain the detailed reason. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema_map.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema_map.cc:69: map->Erase(policy_name); You can log the error here, to assist extension developers: LOG(ERROR) << "Dropping policy " << policy_name << " for " << it->first.component_id << " because it's not valid: " << error; https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema_unittest.cc:62: " }," Test with a list of dictionaries too. More types to tests: - Object with lists - Lists with integers - Lists with dictionaries, with lists of integers
https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:661: } else { On 2014/01/22 10:49:53, Joao da Silva wrote: > The else part should just be: > > std::string sub_error; > if (!subschema.Validate(it.value(), StrategyForNextLevel(strategy), > &sub_error) { > if (!StrategyAllowInvalidOnTopLevel(strategy)) { > *error = sub_error; > return false; > } > } > > Is there a reason why you look at the subschema.node_->type below? I may be > missing something. Note that the subtype may be a list with dictionaries inside, > so you shouldn't pass STRICT directly, because we may allow invalid or unknown > in those inner dictionaries. The current approach is result of trade-off. In short words, I dropped support for dict-list-dict-nested schema. The current approach support dict-dict-nested, dict-list-nested, list-dict-nested schema well, but for dict-list-dict-nested schema, it will use user defined strategy for outer dict and strict strategy for inner one. For the first question, I need separate routines to handle this since I think StrategyAllowInvalidOnTopLevel(strategy) is only supposed to take effect on properties. It's obvious, otherwise SCHEME_ALLOW_INVALID_TOPLEVEL will always return true even if it's supposed to be strict for deeper level invalids. For the second question, the current approach is based on an assumption that every properties in a dict which is not a dict type are supposed to strictly follow the schema. This is a result of trade-off. Take the dict-list-dict-nested schema as example: 1) There is no cheap way to verify if "some-propery-name-list-dict" is dict type instead of list type, but it's necessary as stated before for StrategyAllowInvalidOnTopLevel(strategy) check. 2) I think strategy is meaningful for outer dict(or dict-dict-nested) only. It looks odd no matter we reset it to root status or just keep it unchanged on the inner dict node, and for the former case it complicated the code a lot. 3) Basically we are combining several policies into "A.B.C" paths in a dict, it's a corner case to have dict-list-dict nested schema, and for inner dict there are no proper path names referring it as well. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:681: if (!*it || !GetItems().Validate(**it, strategy, error)) On 2014/01/22 10:49:53, Joao da Silva wrote: > Replace "strategy" with StrategyForNextLevel(strategy). The list may contain > dictionaries. For list-dict-nested schema(with list/list-list-nested in the root), I think it's better to keep strategy unchanged since it just means nothing for list. For dict-list-nested, following the assumption stated above I passed the strict strategy to lower levels. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:754: } On 2014/01/22 10:49:53, Joao da Silva wrote: > This needs to handle lists of dictionaries; the inner dictionaries must be > normalized too. Yes, it did break normalizing by list-dict-nested schema, I will fix it.
Hello Joao, As talked with you offline, I have wrong understanding of SCHEMA_ALLOW_INVALID_TOPLEVEL on the previous patch. I simplified the code as you suggest and added a few new tests for complicated schema. PTAL Bin https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:638: *error = "Fatal error: The schema itself is invalid."; On 2014/01/22 10:49:53, Joao da Silva wrote: > Just "The schema is invalid." Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:643: *error = "The schema type and value type is not matched."; On 2014/01/22 10:49:53, Joao da Silva wrote: > The value type doesn't match the schema type. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:705: *error = "Fatal error: The schema itself is invalid."; On 2014/01/22 10:49:53, Joao da Silva wrote: > Just "The schema is invalid." Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.cc:710: *error = "The schema type and value type is not matched."; On 2014/01/22 10:49:53, Joao da Silva wrote: > The value type doesn't match the schema type. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:30: SCHEMA_VERY_STRICT = 0, On 2014/01/22 10:49:53, Joao da Silva wrote: > just SCHEMA_STRICT. Add a documenting comment: > > // No errors will be allowed. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:31: SCHEMA_ALLOW_UNKNOWN_TOPLEVEL, On 2014/01/22 10:49:53, Joao da Silva wrote: > Add a comment: > > // Unknown properties in the top-level dictionary will be ignored. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:32: SCHEMA_ALLOW_UNKNOWN, On 2014/01/22 10:49:53, Joao da Silva wrote: > Add a comment: > > // Unknown properties in any dictionary will be ignored. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:33: SCHEMA_ALLOW_INVALID_TOPLEVEL, On 2014/01/22 10:49:53, Joao da Silva wrote: > Add a comment: > > // Mismatched values will be ignored at the toplevel. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:34: SCHEMA_ALLOW_INVALID_TOPLEVEL_AND_ALLOW_UNKNOWN, On 2014/01/22 10:49:53, Joao da Silva wrote: > Add a comment: > > // Mismatched values will be ignored at the top-level dictionary. > // Unknown properties in any dictionary will be ignored. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:35: SCHEMA_ALLOW_INVALID, On 2014/01/22 10:49:53, Joao da Silva wrote: > Add a comment: > > // Mismatched values will be ignored. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:76: // |error| will contain a message state the detailed reason. On 2014/01/22 10:49:53, Joao da Silva wrote: > |error| will contain the detailed reason. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema.h:84: // returned and |error| will contain a message state the detailed message. On 2014/01/22 10:49:53, Joao da Silva wrote: > returned and |error| will contain the detailed reason. Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema_map.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema_map.cc:69: map->Erase(policy_name); On 2014/01/22 10:49:53, Joao da Silva wrote: > You can log the error here, to assist extension developers: > > LOG(ERROR) << "Dropping policy " << policy_name << " for " << > it->first.component_id << " because it's not valid: " << error; Done. https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/134153005/diff/90002/components/policy/core/c... components/policy/core/common/schema_unittest.cc:62: " }," On 2014/01/22 10:49:53, Joao da Silva wrote: > Test with a list of dictionaries too. More types to tests: > > - Object with lists > - Lists with integers > - Lists with dictionaries, with lists of integers Done. Added tests for ArrayOfObjects, ObjectOfArray and ArrayOfObjectOfArray.
This looks much better! It's ready to go after fixing a couple of little details, see inline. Nice test coverage too. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:636: std::string *error) const { std::string* error The style guide says that the "*" goes next to the type, not the variable https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:664: StrategyForNextLevel(strategy), &sub_error)) { The style guide says that arguments should be aligned. Try using clang-format on this line. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:677: StrategyForNextLevel(strategy), error)) { Run clang-format on this line too https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:708: if (!value->IsType(type())) { We could handle INTEGER to DOUBLE promotion here, what do you think? https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:733: StrategyForNextLevel(strategy), &sub_error)) { Run clang-format on this line https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:746: dict->Remove(*it, NULL); RemoveWithoutPathExpansion https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:756: StrategyForNextLevel(strategy), &sub_error)) { Run clang-format on this line https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:768: list->Remove(*it, NULL); This is expensive, because it will search for the value. Use list->Erase(it, NULL); https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:26: // the strategy to handle unknown or invalid properties for dict type. Note This is not only for the dict type. How about: "the strategy to handle unknown properties or invalid values." https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:38: // Mismatched values will be ignored at the top-level dictionary. "at the top-level value." It could be a list, for example. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:81: // handle unknown or invalid properties in dict type. Allowed errors will be "handle unknown properties or invalid values." https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:89: // handle unknown or invalid properties in dict type. Allowed errors will be "handle unknown properties or invalid values." https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema_map.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_map.cc:69: SCHEMA_STRICT, &error)) { Try formatting this line with clang-format: if (!policy_value || !policy_schema.Validate(*policy_value, SCHEMA_STRICT, &error)) { ... } https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_map.cc:71: it->first.component_id << " because it's not valid: " << error; Run clang-format on this one too; I didn't format it in my previous comment. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:120: static const char* no_error_returned = "No error returned."; static const char kNoErrorReturned[] = "No error returned."; https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:140: SCHEMA_STRICT, &error)); Run clang-format on these 2 lines https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:725: root.Set("List", list_value); // Pass ownership to root; // Pass ownership to root. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:753: root.Append(dict_value); // Pass ownership to root // Pass ownership to root. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:755: // No errors // No errors.
Hello Joao, PTAL at the newest patch. Bin https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:636: std::string *error) const { On 2014/01/23 15:52:36, Joao da Silva wrote: > std::string* error > > The style guide says that the "*" goes next to the type, not the variable Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:664: StrategyForNextLevel(strategy), &sub_error)) { On 2014/01/23 15:52:36, Joao da Silva wrote: > The style guide says that arguments should be aligned. Try using clang-format on > this line. Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:677: StrategyForNextLevel(strategy), error)) { On 2014/01/23 15:52:36, Joao da Silva wrote: > Run clang-format on this line too Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:708: if (!value->IsType(type())) { On 2014/01/23 15:52:36, Joao da Silva wrote: > We could handle INTEGER to DOUBLE promotion here, what do you think? I added special check to allow Integer to Double promotion, but I prefer not to do the in-place promotion here, since it need dirty hack to replace the type value of FundamentalValue and also FundamentalValue::GetAsDouble can take care of this later. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:733: StrategyForNextLevel(strategy), &sub_error)) { On 2014/01/23 15:52:36, Joao da Silva wrote: > Run clang-format on this line Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:746: dict->Remove(*it, NULL); On 2014/01/23 15:52:36, Joao da Silva wrote: > RemoveWithoutPathExpansion Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:756: StrategyForNextLevel(strategy), &sub_error)) { On 2014/01/23 15:52:36, Joao da Silva wrote: > Run clang-format on this line Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:768: list->Remove(*it, NULL); On 2014/01/23 15:52:36, Joao da Silva wrote: > This is expensive, because it will search for the value. Use list->Erase(it, > NULL); The type of *it is size_t, and I actually called ListValue::Remove(size_t, ...). https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema.h (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:26: // the strategy to handle unknown or invalid properties for dict type. Note On 2014/01/23 15:52:36, Joao da Silva wrote: > This is not only for the dict type. How about: > > "the strategy to handle unknown properties or invalid values." Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:38: // Mismatched values will be ignored at the top-level dictionary. On 2014/01/23 15:52:36, Joao da Silva wrote: > "at the top-level value." > > It could be a list, for example. Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:81: // handle unknown or invalid properties in dict type. Allowed errors will be On 2014/01/23 15:52:36, Joao da Silva wrote: > "handle unknown properties or invalid values." Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.h:89: // handle unknown or invalid properties in dict type. Allowed errors will be On 2014/01/23 15:52:36, Joao da Silva wrote: > "handle unknown properties or invalid values." Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema_map.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_map.cc:69: SCHEMA_STRICT, &error)) { On 2014/01/23 15:52:36, Joao da Silva wrote: > Try formatting this line with clang-format: > > if (!policy_value || > !policy_schema.Validate(*policy_value, SCHEMA_STRICT, &error)) { > ... > } Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_map.cc:71: it->first.component_id << " because it's not valid: " << error; On 2014/01/23 15:52:36, Joao da Silva wrote: > Run clang-format on this one too; I didn't format it in my previous comment. Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:120: static const char* no_error_returned = "No error returned."; On 2014/01/23 15:52:36, Joao da Silva wrote: > static const char kNoErrorReturned[] = "No error returned."; Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:140: SCHEMA_STRICT, &error)); On 2014/01/23 15:52:36, Joao da Silva wrote: > Run clang-format on these 2 lines Done. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema_unittest.cc:755: // No errors On 2014/01/23 15:52:36, Joao da Silva wrote: > // No errors. Done.
The remaining comments are all trivial, so lgtm after fixing them. https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/300001/components/policy/core/... components/policy/core/common/schema.cc:768: list->Remove(*it, NULL); On 2014/01/23 17:20:13, bjin wrote: > On 2014/01/23 15:52:36, Joao da Silva wrote: > > This is expensive, because it will search for the value. Use list->Erase(it, > > NULL); > > The type of *it is size_t, and I actually called ListValue::Remove(size_t, ...). I was confused by the drop_list of the dictionary, which is a vector<string>. Nevermind, this is fine. https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:643: // Allow the integer to double promition. Note that range restriction on "promotion" https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:647: return true; Use {} around the "if" body when the "if" test or the body spans more than 1 line https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:714: remove this newline https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:716: // Allow the integer to double promition. Note that range restriction on "promotion" https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema_unittest.cc:120: static const char* kNoErrorReturned = "No error returned."; Don't use a pointer, use an array: static const char kNoErrorReturned[] = "No error returned.";
https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... File components/policy/core/common/schema.cc (right): https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:643: // Allow the integer to double promition. Note that range restriction on On 2014/01/23 20:18:47, Joao da Silva wrote: > "promotion" Done. https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:647: return true; On 2014/01/23 20:18:47, Joao da Silva wrote: > Use {} around the "if" body when the "if" test or the body spans more than 1 > line Done. https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:714: On 2014/01/23 20:18:47, Joao da Silva wrote: > remove this newline Done. https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema.cc:716: // Allow the integer to double promition. Note that range restriction on On 2014/01/23 20:18:47, Joao da Silva wrote: > "promotion" Done. https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... File components/policy/core/common/schema_unittest.cc (right): https://codereview.chromium.org/134153005/diff/400001/components/policy/core/... components/policy/core/common/schema_unittest.cc:120: static const char* kNoErrorReturned = "No error returned."; On 2014/01/23 20:18:47, Joao da Silva wrote: > Don't use a pointer, use an array: > > static const char kNoErrorReturned[] = "No error returned."; Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/binjin@chromium.org/134153005/450001
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/134153005/450001
Message was sent while issue was closed.
Change committed as 246770 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
