Share JSON schema constants.

Share JSON schema constants.

BUG=None
TEST=unit_tests green


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157051

[Mattias Nissler (ping if slow), 2012-06-28 12:56:47]

LGTM, but please also run this by the maintainer of JSONSchemaValidator.

[Joao da Silva, 2012-06-28 12:58:47]

@Aaron: you seem to be the author of the JSONSchemaValidator, can you have a
look? Thanks!

[Aaron Boodman, 2012-06-28 21:24:11]

https://chromiumcodereview.appspot.com/10696034/diff/1/chrome/common/json_sch...
File chrome/common/json_schema_constants.h (right):

https://chromiumcodereview.appspot.com/10696034/diff/1/chrome/common/json_sch...
chrome/common/json_schema_constants.h:10: extern const char
kAdditionalProperties[];
Did you consider just making these static fields of JSONSchemaValidator?

[Joao da Silva, 2012-06-29 09:03:14]

https://chromiumcodereview.appspot.com/10696034/diff/1/chrome/common/json_sch...
File chrome/common/json_schema_constants.h (right):

https://chromiumcodereview.appspot.com/10696034/diff/1/chrome/common/json_sch...
chrome/common/json_schema_constants.h:10: extern const char
kAdditionalProperties[];
On 2012/06/28 21:24:11, Aaron Boodman wrote:
> Did you consider just making these static fields of JSONSchemaValidator?

Yea, that was the first approach. But it became ugly fast in the other
functions, because it required prefixing every use with JSONSchemaValidator::.
Something like "using JSONSchemaValidator::kProperties" doesn't work. wdyt?

[Aaron Boodman, 2012-06-29 21:32:10]

Actually, stepping back, I'm a bit confused as to what is even going on here.
How is JSON Schema being used for some kind of group policy?

It seems like fishing the constants out of JSONSchemaValidator and implementing
similar-looking code to interpret schemas is not a good plan.

[Joao da Silva, 2012-07-02 09:01:00]

On 2012/06/29 21:32:10, Aaron Boodman wrote:
> Actually, stepping back, I'm a bit confused as to what is even going on here.
> How is JSON Schema being used for some kind of group policy?
> 
> It seems like fishing the constants out of JSONSchemaValidator and
implementing
> similar-looking code to interpret schemas is not a good plan.

We're using (automatically generated) JSON schemas to assist in decoding values
from the registry. These schemas aren't used for validation yet, but are used to
determine the data type of values stored in the registry. The problem is that
the registry can't represent some of the JSON data types used by policies (e.g.
booleans, lists, doubles). We also represent the policies as "expanded" values
in the registry instead of as a string-encoded-JSON-blob so that management of
Chrome integrates well with the existing administrative tools.

So the code looks similar but it isn't doing the same thing :-) I don't feel
strongly about this CL (and maybe the linker turns these intro shared strings
anyway), but I think sharing the constants is a good thing. Let me know what you
think!

[Aaron Boodman, 2012-08-06 23:40:44]

I'm going through old CLs and found this one. Do you still want this reviews?

[Joao da Silva, 2012-08-07 09:34:49]

This CL isn't terribly important, I just find it nicer to share the
constants (the linker might coalesce them anyway).

Please have another look and let me know what you think of the change.
Thanks!

On Tue, Aug 7, 2012 at 1:40 AM, <aa@chromium.org> wrote:

> I'm going through old CLs and found this one. Do you still want this
> reviews?
>
>
http://codereview.chromium.**org/10696034/<http://codereview.chromium.org/106...
>

[Aaron Boodman, 2012-08-10 19:30:23]

Instead of poking into the JSON Schema format, is it possible to expose this
functionality through JSONSchemaValidator at a higher level?

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:33: using namespace
json_schema_constants;
If you end up sticking with the approach you have:

We don't typically include entire namespace this way because it makes it hard to
find where symbols come from.

Instead, how about:

namespace json_schema = json_schema_constants. Or just bite the bullet and type
it out each time :)

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:225: base::Value::Type GetType(const
base::DictionaryValue* schema,
We already have JSONSchemaValidator::GetJSONSchemaType. Why is it insufficient?
Can it be improved?

[Aaron Boodman, 2012-09-04 21:36:06]

Ping?

[Joao da Silva, 2012-09-05 14:03:29]

@aa: PTAL. Bit the bullet and went for the prefix each time :-)

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:33: using namespace
json_schema_constants;
On 2012/08/10 19:30:23, Aaron Boodman wrote:
> If you end up sticking with the approach you have:
> 
> We don't typically include entire namespace this way because it makes it hard
to
> find where symbols come from.
> 
> Instead, how about:
> 
> namespace json_schema = json_schema_constants. Or just bite the bullet and
type
> it out each time :)

Just went for this one. It's a bit extra typing but still better than typing the
full JSONSchemaValidator:: or json_schema_constants::. wdyt?

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:225: base::Value::Type GetType(const
base::DictionaryValue* schema,
On 2012/08/10 19:30:23, Aaron Boodman wrote:
> We already have JSONSchemaValidator::GetJSONSchemaType. Why is it
insufficient?
> Can it be improved?

JSONSchemaValidator::GetJSONSchemaType does Value->Schema and this does the
inverse, Schema->Value.

[Aaron Boodman, 2012-09-11 22:32:39]

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:225: base::Value::Type GetType(const
base::DictionaryValue* schema,
On 2012/09/05 14:03:29, Joao da Silva wrote:
> On 2012/08/10 19:30:23, Aaron Boodman wrote:
> > We already have JSONSchemaValidator::GetJSONSchemaType. Why is it
> insufficient?
> > Can it be improved?
> 
> JSONSchemaValidator::GetJSONSchemaType does Value->Schema and this does the
> inverse, Schema->Value.

Just add it to JSONSchemaValidator then. I feel like it's better to encapsulate
this knowledge there rather than put it in this leaf.

[Joao da Silva, 2012-09-12 16:09:39]

PTYAL :-)

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10696034/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:225: base::Value::Type GetType(const
base::DictionaryValue* schema,
On 2012/09/11 22:32:39, Aaron Boodman wrote:
> On 2012/09/05 14:03:29, Joao da Silva wrote:
> > On 2012/08/10 19:30:23, Aaron Boodman wrote:
> > > We already have JSONSchemaValidator::GetJSONSchemaType. Why is it
> > insufficient?
> > > Can it be improved?
> > 
> > JSONSchemaValidator::GetJSONSchemaType does Value->Schema and this does the
> > inverse, Schema->Value.
> 
> Just add it to JSONSchemaValidator then. I feel like it's better to
encapsulate
> this knowledge there rather than put it in this leaf.

Done.

[Aaron Boodman, 2012-09-16 00:58:43]

lgtm

http://codereview.chromium.org/10696034/diff/25008/chrome/common/json_schema_...
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/10696034/diff/25008/chrome/common/json_schema_...
chrome/common/json_schema_validator.cc:104: bool
JSONSchemaValidator::GetValueType(const std::string& schema_type,
Ah, this function actually does something slightly different - it operates on
the type name, not an actual value.

In that case, I think it was better in policy_loader_win.cc. Sorry!

[Joao da Silva, 2012-09-16 08:40:37]

Thanks for the review!

@thakis: please do an owner check for chrome/

http://codereview.chromium.org/10696034/diff/25008/chrome/common/json_schema_...
File chrome/common/json_schema_validator.cc (right):

http://codereview.chromium.org/10696034/diff/25008/chrome/common/json_schema_...
chrome/common/json_schema_validator.cc:104: bool
JSONSchemaValidator::GetValueType(const std::string& schema_type,
On 2012/09/16 00:58:43, Aaron Boodman wrote:
> Ah, this function actually does something slightly different - it operates on
> the type name, not an actual value.
> 
> In that case, I think it was better in policy_loader_win.cc. Sorry!

np, done!

[Nico, 2012-09-16 13:15:29]

lgtm

http://codereview.chromium.org/10696034/diff/28015/chrome/common/json_schema_...
File chrome/common/json_schema_constants.h (right):

http://codereview.chromium.org/10696034/diff/28015/chrome/common/json_schema_...
chrome/common/json_schema_constants.h:8: namespace json_schema_constants {
add overview comment

[Joao da Silva, 2012-09-16 13:36:26]

Thanks Nico!

http://codereview.chromium.org/10696034/diff/28015/chrome/common/json_schema_...
File chrome/common/json_schema_constants.h (right):

http://codereview.chromium.org/10696034/diff/28015/chrome/common/json_schema_...
chrome/common/json_schema_constants.h:8: namespace json_schema_constants {
On 2012/09/16 13:15:29, Nico wrote:
> add overview comment

Done.

[commit-bot: I haz the power, 2012-09-16 13:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10696034/26010

[commit-bot: I haz the power, 2012-09-16 15:36:25]

Change committed as 157051