Added new policy Schema classes.

Added new policy Schema classes.

This implementation obsoletes PolicySchema and its existing users will be refactored.

This implementation has the advantage of allowing compile-time static data to be generated and quickly loaded at runtime. That will allow building the Chrome policies schema without impact on startup.

BUG=270667
TBR=joi@chromium.org

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

[Joao da Silva, 2013-09-12 17:59:20]

This is now polished and ready for a review. The next steps are to refactor
existing code and kill policy_schema.cc, and then to generate the chrome schema
& build it.

[Mattias Nissler (ping if slow), 2013-09-13 12:56:48]

https://chromiumcodereview-hr.appspot.com/23851022/diff/1/components/policy/c...
File components/policy/core/common/schema.h (right):

https://chromiumcodereview-hr.appspot.com/23851022/diff/1/components/policy/c...
components/policy/core/common/schema.h:25: const void* extra;
Use a union here?

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
File components/policy/core/common/schema.cc (right):

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.cc:25: class PolicySchemaImpl : public
PolicySchema {
If PolicySchema has only one implementation, why make it pure-virtual?

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.cc:99: // Validate the schema version.
That means we can never migrate to newer schema versions if we want to keep old
Chrome builds working. Is that intentional?

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.cc:115: // Checks for invalid attributes at
the top-level.
Any good reason? They'd just be ignored.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.cc:141: *error = "The \"any\" type can't be
used.";
I think you should just say "Type not supported: " << type_string

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.cc:304: static_cast<const
internal::PropertiesNode*>(schema_->extra)->additional);
Hm, casts. Maybe use a union? That's still easier to read.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.cc:309: return schema.valid() ? schema :
GetAdditionalProperties();
Shouldn't this return Schema(NULL) for the error case?

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
File components/policy/core/common/schema.h (right):

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.h:16: 
I think we usually don't put blank lines between namespace declarations, but I
may be wrong.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.h:17: namespace schema {
This namespace is not really useful (cf. current discussion on chromium-dev).

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.h:70: 
remove extra whitespace

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.h:72: // These methods can be called only
if type() == TYPE_DICTIONARY.
s/can/should/, and ideally describe what happens if I still call them.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema.h:89: 
remove extra whitespace.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
File components/policy/core/common/schema_internal.h (right):

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema_internal.h:23: // If type is TYPE_LIST then
this is a SchemaNode*.
... describing the element type.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema_internal.h:24: // If type is
TYPE_DICTIONARY then this is a PropertiesNode*.
Sounds like a union. I think that's justified here given that we're working with
POD here the static casts you have in the CL are essentially a hand-implemented
union.

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
File components/policy/core/common/schema_unittest.cc (right):

https://chromiumcodereview-hr.appspot.com/23851022/diff/14001/components/poli...
components/policy/core/common/schema_unittest.cc:36: TEST(SchemaTest,
MinimalSchema) {
Why not include this test in InvalidSchemas?

[Joao da Silva, 2013-09-13 14:02:54]

Thanks for the comments. Updates:

- removed schema namespace
- renamed PolicySchema to SchemaOwner, and made it non-virtual
- other smaller fixes

It'd be nice to use a union but it's not possible to statically initialize
members other than the first. This is a limitation of C++03 (details at
http://stackoverflow.com/a/3031653)

PTAL, thanks!

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:25: class PolicySchemaImpl : public
PolicySchema {
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> If PolicySchema has only one implementation, why make it pure-virtual?

Right. There were 2 implementations but in the end one was enough, so this can
be simplified. Done.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:99: // Validate the schema version.
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> That means we can never migrate to newer schema versions if we want to keep
old
> Chrome builds working. Is that intentional?

What can we possibly do for old builds?

We'll have to support both versions for a while, wait until extensions also
update to the new version (this may be 6 months to even a year), and then the
old version is removed when the metrics show that there are no more users.

Is there something you'd like to add at this stage?

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:115: // Checks for invalid attributes at
the top-level.
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> Any good reason? They'd just be ignored.

So that extension authors see the error.

These could be warnings for now so that we could lift the restriction in the
future, but I don't see how we can have top-level policies with any name. If you
feel strongly about being open to this future change then I'll turn these into
LOG(WARNING)s.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:141: *error = "The \"any\" type can't be
used.";
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> I think you should just say "Type not supported: " << type_string

Done. The "any" type was assumed because the JSONSchemaValidator rejects unknown
types, and only "any" is missing from SchemaTypeToValueType.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:304: static_cast<const
internal::PropertiesNode*>(schema_->extra)->additional);
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> Hm, casts. Maybe use a union? That's still easier to read.

I don't mind these casts *at all* :-)

I did want to use a union at first, but only the first element of a union can be
statically initialized.

I find this better than having 2 pointers. Do you have other ideas, or is that
cast fine for now?

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:309: return schema.valid() ? schema :
GetAdditionalProperties();
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> Shouldn't this return Schema(NULL) for the error case?

GetAdditionalProperties returns Schema(NULL) if the PropertiesNode->additional
field is NULL.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.h:16: 
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> I think we usually don't put blank lines between namespace declarations, but I
> may be wrong.

The style guide has an example with nested namespaces at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...
without the extra line; removed these lines.

I've made an exception for the anonymous namespace in the .cc files because of
habit. /base/files/file_util.h and file_util_posix.cc have examples of both of
these cases :-)

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.h:17: namespace schema {
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> This namespace is not really useful (cf. current discussion on chromium-dev).

Alright, removed. Renamed PolicySchema to SchemaOwner to avoid collusion with
policy_schema.cc.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.h:70: 
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> remove extra whitespace

Done.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.h:72: // These methods can be called only
if type() == TYPE_DICTIONARY.
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> s/can/should/, and ideally describe what happens if I still call them.

Done.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.h:89: 
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> remove extra whitespace.

Done.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema_internal.h (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema_internal.h:23: // If type is TYPE_LIST then
this is a SchemaNode*.
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> ... describing the element type.

Done.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema_internal.h:24: // If type is
TYPE_DICTIONARY then this is a PropertiesNode*.
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> Sounds like a union. I think that's justified here given that we're working
with
> POD here the static casts you have in the CL are essentially a
hand-implemented
> union.

Exactly. This was a union initially, but unfortunately only the first union
member can be statically initialized in C++03. We'll need both for the
statically generated Chrome stuff.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema_unittest.cc:36: TEST(SchemaTest,
MinimalSchema) {
On 2013/09/13 12:56:48, Mattias Nissler wrote:
> Why not include this test in InvalidSchemas?

This schema is valid, and this is just testing that the minimal schema is
accepted.

[Joao da Silva, 2013-09-13 14:21:55]

+Jói for components_tests.gypi.

@Jói: could we have more reviewers for this file? Every new unittest for
components must go through you for now :-)

[Mattias Nissler (ping if slow), 2013-09-13 14:55:16]

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:99: // Validate the schema version.
On 2013/09/13 14:02:54, Joao da Silva wrote:
> On 2013/09/13 12:56:48, Mattias Nissler wrote:
> > That means we can never migrate to newer schema versions if we want to keep
> old
> > Chrome builds working. Is that intentional?
> 
> What can we possibly do for old builds?
> 
> We'll have to support both versions for a while, wait until extensions also
> update to the new version (this may be 6 months to even a year), and then the
> old version is removed when the metrics show that there are no more users.
> 
> Is there something you'd like to add at this stage?

I'm worried about the fact that extension authors can't switch to the new
version as long as the old browser version is still around. It might be simpler
to just drop the check here if the assumption is that  new schema versions are
backward-compatible (so the browser is forward-compatible with new browser
versions).

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:115: // Checks for invalid attributes at
the top-level.
On 2013/09/13 14:02:54, Joao da Silva wrote:
> On 2013/09/13 12:56:48, Mattias Nissler wrote:
> > Any good reason? They'd just be ignored.
> 
> So that extension authors see the error.
> 
> These could be warnings for now so that we could lift the restriction in the
> future, but I don't see how we can have top-level policies with any name. If
you
> feel strongly about being open to this future change then I'll turn these into
> LOG(WARNING)s.

Right, makes sense. Let's leave the check in then.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:304: static_cast<const
internal::PropertiesNode*>(schema_->extra)->additional);
On 2013/09/13 14:02:54, Joao da Silva wrote:
> On 2013/09/13 12:56:48, Mattias Nissler wrote:
> > Hm, casts. Maybe use a union? That's still easier to read.
> 
> I don't mind these casts *at all* :-)
> 
> I did want to use a union at first, but only the first element of a union can
be
> statically initialized.
> 
> I find this better than having 2 pointers. Do you have other ideas, or is that
> cast fine for now?

Fine to go with the cast then.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:309: return schema.valid() ? schema :
GetAdditionalProperties();
On 2013/09/13 14:02:54, Joao da Silva wrote:
> On 2013/09/13 12:56:48, Mattias Nissler wrote:
> > Shouldn't this return Schema(NULL) for the error case?
> 
> GetAdditionalProperties returns Schema(NULL) if the PropertiesNode->additional
> field is NULL.

Right, sorry, I was confused.

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema_unittest.cc (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema_unittest.cc:36: TEST(SchemaTest,
MinimalSchema) {
On 2013/09/13 14:02:54, Joao da Silva wrote:
> On 2013/09/13 12:56:48, Mattias Nissler wrote:
> > Why not include this test in InvalidSchemas?
> 
> This schema is valid, and this is just testing that the minimal schema is
> accepted.

Right, sorry. I failed to parse the double negation.

https://codereview.chromium.org/23851022/diff/28001/components/policy/core/co...
File components/policy/core/common/schema_internal.h (right):

https://codereview.chromium.org/23851022/diff/28001/components/policy/core/co...
components/policy/core/common/schema_internal.h:20: // If "type" is TYPE_LIST
then this is a SchemaNode* describing the
nit: |type| (also below).

[Joao da Silva, 2013-09-13 15:15:47]

Mattias, Jói, PTAL

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/23851022/diff/14001/components/policy/core/co...
components/policy/core/common/schema.cc:99: // Validate the schema version.
On 2013/09/13 14:55:17, Mattias Nissler wrote:
> On 2013/09/13 14:02:54, Joao da Silva wrote:
> > On 2013/09/13 12:56:48, Mattias Nissler wrote:
> > > That means we can never migrate to newer schema versions if we want to
keep
> > old
> > > Chrome builds working. Is that intentional?
> > 
> > What can we possibly do for old builds?
> > 
> > We'll have to support both versions for a while, wait until extensions also
> > update to the new version (this may be 6 months to even a year), and then
the
> > old version is removed when the metrics show that there are no more users.
> > 
> > Is there something you'd like to add at this stage?
> 
> I'm worried about the fact that extension authors can't switch to the new
> version as long as the old browser version is still around. It might be
simpler
> to just drop the check here if the assumption is that  new schema versions are
> backward-compatible (so the browser is forward-compatible with new browser
> versions).

I see where you're coming from. The latest draft is indeed quite similar to v3,
so it makes sense to accept later versions.

That means that we also have to modify the JSONSchemaValidator to ignore unknown
attributes. I'll do that change later, and update these unittests.

https://codereview.chromium.org/23851022/diff/28001/components/policy/core/co...
File components/policy/core/common/schema_internal.h (right):

https://codereview.chromium.org/23851022/diff/28001/components/policy/core/co...
components/policy/core/common/schema_internal.h:20: // If "type" is TYPE_LIST
then this is a SchemaNode* describing the
On 2013/09/13 14:55:17, Mattias Nissler wrote:
> nit: |type| (also below).

Done.

[Mattias Nissler (ping if slow), 2013-09-13 15:22:10]

LGTM

[commit-bot: I haz the power, 2013-09-16 07:36:21]

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

[commit-bot: I haz the power, 2013-09-16 07:40:51]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-16 07:42:02]

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

[commit-bot: I haz the power, 2013-09-16 10:51:27]

Change committed as 223326

[Jói, 2013-09-16 11:06:39]

LGTM, sorry for the delay.