Generate the Chrome policy schema at compile time.

Generate the Chrome policy schema at compile time.

The schema describes the expected types of the known Chrome policies, so that
they can be properly loaded at runtime from the platform policy services.

The Chrome schema is represented by static structures that are generated at
compile time, so that no parsing is required at runtime. This is important
because the schema is always created to load the initial policy during startup.

Also updated the policy::internal:: schema structures to avoid unnecessary
relocations at load time.

BUG=270667
R=dconnelly@chromium.org, pastarmovj@chromium.org

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

[Joao da Silva, 2013-10-20 21:43:04]

Adding 2 reviewers because this one is a bit tricky :-)

- increases the binary size by about 8600 bytes (compared to 18k with the
version at https://codereview.chromium.org/24407003)

- the ChromePolicyDefinitionList array becomes largely redundant with the
schema; I'm working on a CL to remove it. A first impression suggests that the
combined increase is only 4500 bytes; the number of relocations in the end is
almost exactly the same

- this is way easier to review if you apply it locally and look at
out/Debug/gen/policy/policy/policy_constants.[h,cc] :-)

Please have a look, thanks!

[dconnelly, 2013-10-21 10:25:08]

Preliminary Python comments. C++ review still in progress.

https://codereview.chromium.org/31273002/diff/1/chrome/tools/build/generate_p...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:308: if s in self.shared_strings:
In case you didn't know, there's a ternary expression syntax

https://codereview.chromium.org/31273002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:320: assert name in
SIMPLE_SCHEMA_NAME_MAP
These asserts aren't necessary and aren't very idiomatic.  A KeyError is thrown
when you try to subscript-access missing properties of a dictionary.

[Joao da Silva, 2013-10-21 16:40:14]

+bartfab, feel free to have a look & comment

[pastarmovj, 2013-10-22 09:30:46]

Some comments from me as well.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.cc:184: return Schema(data_,
data_->schema_nodes + 0);
I don't think this "+ 0" really adds much value here.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.cc:295: int index = base_index;
Just a warning that this redefines the variable index declared at line 268. It
seems safe but I'd rather avoid it to make the code easier to understand and
debug.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:35: // Builds a schema pointing to the
inner structure of |data|,
nitty nit: It seems like you can fit more of this comment on the previous line.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:118: // Note: PropertiesNodes must have
their PropertyNodes sorted by key.
This note is a bit of a tricky thing. There is no direct refernce to
PropertiesNodes type in this function and therefore it is hard to guess what is
meant here. Either remove the comment, extend it or move it to where it actually
is important that this property is true.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:128: class Internal;
I don't like this name very much. It doesn't tell what it really is - a data
container. I think InternalSchemaStorage or something like this might be more
useful name.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:136: static int Parse(const
base::DictionaryValue& schema,
I think this function as well as the two helpers below belong to the Internal
class as actual member functions since they require the instance to work.

[dconnelly, 2013-10-22 11:55:52]

Sorry, not much more to add. This appears to be correct based on our
conversation from yesterday.

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:300: 'integer': -1,
use None instead of -1

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:367: properties = [
(self.GetString(key), self.Generate(schema, key))
This can be a generator expression

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.cc:277: const base::DictionaryValue* dict =
NULL;
Can these be moved closer to their first use?

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:150: scoped_ptr<Internal> internal_;
Please add a comment explaining that internal_ will hold NULL when the instance
was obtained using Wrap() instead of Parse().

[Joao da Silva, 2013-10-22 12:53:26]

Thanks for the comments, PTAL.

https://codereview.chromium.org/31273002/diff/1/chrome/tools/build/generate_p...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:308: if s in self.shared_strings:
On 2013/10/21 10:25:08, dconnelly wrote:
> In case you didn't know, there's a ternary expression syntax

Good idea, done.

https://codereview.chromium.org/31273002/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:320: assert name in
SIMPLE_SCHEMA_NAME_MAP
On 2013/10/21 10:25:08, dconnelly wrote:
> These asserts aren't necessary and aren't very idiomatic.  A KeyError is
thrown
> when you try to subscript-access missing properties of a dictionary.

Done.

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:300: 'integer': -1,
On 2013/10/22 11:55:52, dconnelly wrote:
> use None instead of -1

Done.

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:367: properties = [
(self.GetString(key), self.Generate(schema, key))
On 2013/10/22 11:55:52, dconnelly wrote:
> This can be a generator expression

Done.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.cc (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.cc:184: return Schema(data_,
data_->schema_nodes + 0);
On 2013/10/22 09:30:46, pastarmovj wrote:
> I don't think this "+ 0" really adds much value here.

Done.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.cc:277: const base::DictionaryValue* dict =
NULL;
On 2013/10/22 11:55:52, dconnelly wrote:
> Can these be moved closer to their first use?

Done.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.cc:295: int index = base_index;
On 2013/10/22 09:30:46, pastarmovj wrote:
> Just a warning that this redefines the variable index declared at line 268. It
> seems safe but I'd rather avoid it to make the code easier to understand and
> debug.

Renamed the outer "index" to "schema_index".

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:35: // Builds a schema pointing to the
inner structure of |data|,
On 2013/10/22 09:30:46, pastarmovj wrote:
> nitty nit: It seems like you can fit more of this comment on the previous
line.

That's right, but the whole comment doesn't file in a single line. I find it
more readable to break on the comma than in the middle of a part of the
sentence. WDYT?

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:118: // Note: PropertiesNodes must have
their PropertyNodes sorted by key.
On 2013/10/22 09:30:46, pastarmovj wrote:
> This note is a bit of a tricky thing. There is no direct refernce to
> PropertiesNodes type in this function and therefore it is hard to guess what
is
> meant here. Either remove the comment, extend it or move it to where it
actually
> is important that this property is true.

Right, removed the comment. This is already detailed in schema_internal.h and in
the generator script.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:128: class Internal;
On 2013/10/22 09:30:46, pastarmovj wrote:
> I don't like this name very much. It doesn't tell what it really is - a data
> container. I think InternalSchemaStorage or something like this might be more
> useful name.

Renamed to InternalStorage.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:136: static int Parse(const
base::DictionaryValue& schema,
On 2013/10/22 09:30:46, pastarmovj wrote:
> I think this function as well as the two helpers below belong to the Internal
> class as actual member functions since they require the instance to work.

Good observation, done. This header is much cleaner now :-)

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:150: scoped_ptr<Internal> internal_;
On 2013/10/22 11:55:52, dconnelly wrote:
> Please add a comment explaining that internal_ will hold NULL when the
instance
> was obtained using Wrap() instead of Parse().

Done.

[dconnelly, 2013-10-22 13:37:35]

lgtm

https://codereview.chromium.org/31273002/diff/240001/chrome/tools/build/gener...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/240001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:312: if self.simple_types[name] ==
None:
It's doesn't really matter, but you can simply say "if not
self.simple_types[name]:" instead of testing against None.

[pastarmovj, 2013-10-22 13:42:53]

LGTM but keep in mind that neither of us has Python readability so I can't
guarantee you the quality of your Python code :) 
You might want to have it scanned by someone who has.

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/31273002/diff/140001/components/policy/core/c...
components/policy/core/common/schema.h:35: // Builds a schema pointing to the
inner structure of |data|,
On 2013/10/22 12:53:27, Joao da Silva wrote:
> On 2013/10/22 09:30:46, pastarmovj wrote:
> > nitty nit: It seems like you can fit more of this comment on the previous
> line.
> 
> That's right, but the whole comment doesn't file in a single line. I find it
> more readable to break on the comma than in the middle of a part of the
> sentence. WDYT?

sgtm

[Joao da Silva, 2013-10-23 12:18:16]

Note that delta from 2->3. I think it's an interesting category of errors, and
only broke (per chance) on Windows.

[Joao da Silva, 2013-11-03 20:54:59]

Daniel, see the inline comment for an interesting bug caused by the generator
expression.

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:367: properties = [
(self.GetString(key), self.Generate(schema, key))
On 2013/10/22 11:55:52, dconnelly wrote:
> This can be a generator expression

Actually it can't :-)

Turning this into a generator means that the recursive call to self.Generate()
will be performed while inserting the parent node's properties, so that child
nodes get in between; this breaks the binary search lookups.

A very subtle bug, and an indicator that this code is not very obvious. I've
added a new test that catches this problem in another CL, and I'll revert this
change in this CL.

[dconnelly, 2013-11-04 09:18:51]

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/31273002/diff/140001/chrome/tools/build/gener...
chrome/tools/build/generate_policy_source.py:367: properties = [
(self.GetString(key), self.Generate(schema, key))
yuck, good catch

On 2013/11/03 20:55:00, Joao da Silva wrote:
> On 2013/10/22 11:55:52, dconnelly wrote:
> > This can be a generator expression
> 
> Actually it can't :-)
> 
> Turning this into a generator means that the recursive call to self.Generate()
> will be performed while inserting the parent node's properties, so that child
> nodes get in between; this breaks the binary search lookups.
> 
> A very subtle bug, and an indicator that this code is not very obvious. I've
> added a new test that catches this problem in another CL, and I'll revert this
> change in this CL.

[commit-bot: I haz the power, 2013-11-06 09:54:42]

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

[commit-bot: I haz the power, 2013-11-06 14:00:06]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Joao da Silva, 2013-11-07 12:41:08]

Committed patchset #11 manually as r233595 (presubmit successful).