Modify the base::JSONReader interface to take a set of options rather than a boolean flag.

Modify the base::JSONReader interface to take a set of options rather than a boolean flag.

This is just prep work for a large CL that rewrites the JSON parser. Splitting
out this interface change will make it easier to review the other CL.

BUG=121469
TEST=No functional change.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131680

[Robert Sesek, 2012-04-10 21:37:32]

Please enjoy this monotonous CL.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
File base/json/json_reader.cc (right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.cc:112: Value* root = reader.JsonToValue(json, false,
This (true -> false) is deliberate. The JSON parser shouldn't care about whether
the root is a structure (list, dictionary) or something else. That's also why I
removed the corresponding tests.

Everywhere this feature of the parser is used, the result is always
double-checked to the specific type (list, dict) they want.

[Mark Mentovai, 2012-04-10 21:51:02]

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
File base/json/json_reader.cc (right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.cc:112: Value* root = reader.JsonToValue(json, false,
rsesek wrote:
> This (true -> false) is deliberate. The JSON parser shouldn't care about
whether
> the root is a structure (list, dictionary) or something else. That's also why
I
> removed the corresponding tests.
> 
> Everywhere this feature of the parser is used, the result is always
> double-checked to the specific type (list, dict) they want.

Thanks for the clarification, I was going to call this out.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reader.h
File base/json/json_reader.h (right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.h:54: JSON_PARSE_RFC = 0,
…the idea being 0 is strict, and every other weird feature you might want to
support will be represented as a bit?

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.h:55: JSON_ALLOW_TRAILING_COMMAS = 1 << 1,
Because if so, this should be 1 << 0.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.h:132: static Value* Read(const std::string& json,
JSONParserOptions options);
More importantly, if so, you can’t use JSONParserOptions as a named type in this
way. If you had another bit, let’s call it JSON_MULTIFACH = 1 << 2, then the
combination (JSON_ALLOW_TRAILING_COMMAS | JSON_MULTIFACH) would be some value
that isn’t a member of the JSONParserOptions enum at all. “or”-ing these values
together to stick back into a JSONParserOptions (such as when passing to this
function) wouldn’t compile at all, for good reason.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
File base/json/json_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader_unittest.cc:236: JSON_ALLOW_TRAILING_COMMAS));
Prefer lining up under the " like you did on line 255, but NOT like what you did
on lines 410, 428, and 574.

https://chromiumcodereview.appspot.com/9960077/diff/4006/chrome/browser/conte...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/chrome/browser/conte...
chrome/browser/content_settings/content_settings_policy_provider.cc:307:
base::JSON_ALLOW_TRAILING_COMMAS));
I’m bored.

[Robert Sesek, 2012-04-10 22:06:14]

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reader.h
File base/json/json_reader.h (right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.h:54: JSON_PARSE_RFC = 0,
On 2012/04/10 21:51:02, Mark Mentovai wrote:
> …the idea being 0 is strict, and every other weird feature you might want to
> support will be represented as a bit?

Correct. Other weird features may include single-quoted strings.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.h:55: JSON_ALLOW_TRAILING_COMMAS = 1 << 1,
On 2012/04/10 21:51:02, Mark Mentovai wrote:
> Because if so, this should be 1 << 0.

Oops. Forgot to cherry-pick that fix from the rewrite branch.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader.h:132: static Value* Read(const std::string& json,
JSONParserOptions options);
On 2012/04/10 21:51:02, Mark Mentovai wrote:
> More importantly, if so, you can’t use JSONParserOptions as a named type in
this
> way. If you had another bit, let’s call it JSON_MULTIFACH = 1 << 2, then the
> combination (JSON_ALLOW_TRAILING_COMMAS | JSON_MULTIFACH) would be some value
> that isn’t a member of the JSONParserOptions enum at all. “or”-ing these
values
> together to stick back into a JSONParserOptions (such as when passing to this
> function) wouldn’t compile at all, for good reason.

I want C++11 typesafe enums.

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
File base/json/json_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/9960077/diff/4006/base/json/json_reade...
base/json/json_reader_unittest.cc:236: JSON_ALLOW_TRAILING_COMMAS));
On 2012/04/10 21:51:02, Mark Mentovai wrote:
> Prefer lining up under the " like you did on line 255, but NOT like what you
did
> on lines 410, 428, and 574.

Done.

[Mark Mentovai, 2012-04-10 22:08:28]

LGTMultifach

https://chromiumcodereview.appspot.com/9960077/diff/1068/base/json/json_reader.h
File base/json/json_reader.h (right):

https://chromiumcodereview.appspot.com/9960077/diff/1068/base/json/json_reade...
base/json/json_reader.h:57: // Allows commas to exist after the last element in
structures.
Blank line before.

[Wez, 2012-04-10 22:58:15]

remoting/ LGTM