Policy: generate boilerplate policy type and constant code.

Policy: generate boilerplate policy type and constant code. 

This is the first step to being able to re-constitute policy from a non-generic protobuf in a policy request in a fully automated way (i.e. the code for the process can be generated). The non-generic protobuf is required for automatic merging of policy on the server-side. 

BUG=68309
TEST=existing policy/provider tests

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

[danno, 2011-01-03 16:58:41]

not done yet, but I want to give you a flavor of what I'm up to in preparation
for handling the generated protobufs from the device management server. comments
of course welcome.

[gfeher, 2011-01-04 12:58:05]

At first sight, the overall approach looks right.

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.gypi:101: {
This target is only built for OS==win || OS==linux || OS==mac. I guess you need
to build it for all the OSes.

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.gypi:108:
'tools/build/generate_policy_source.py',
Can you move this variable out into the target?

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:512: }
Why are you adding this here?

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:8: -h                generate
header file of policy constants
Nit: turn this into --something?

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:70: ''')
These multi-line string constants make it difficult to navigate in this source
file. I have three suggestions to make this better:

f.write('\n'.join([
   '#ifndef CHROME_COMMON_POLICY_CONSTANTS_H_',
   '#define CHROME_COMMON_POLICY_CONSTANTS_H_',
   '#pragma once',
   ...])

or with implicit string joins:
f.write(
   '#ifndef CHROME_COMMON_POLICY_CONSTANTS_H_\n'
   '#define CHROME_COMMON_POLICY_CONSTANTS_H_\n'
   '#pragma once\n'
)
or define them at the beginning of the module like
POLICY_CONSTANS_H_BEGINNING = '''....'''

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:80: f.closed
f.close() ?

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:103: f.write('[] = "' + policy_name
+ '";\n')
I suggest the following for the above two lines:
f.write('const char k%s[] = "%s";\n' %
        (policy_name, policy_name))

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:147: f.closed
Didn't you mean f.close() ?

[gfeher, 2011-01-04 13:13:17]

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:147: f.closed
Okay, sorry, now I've looked up the with statement, so I understand why you
don't need f.close().

But still, why do you need f.closed?

[danno, 2011-01-11 13:03:48]

review feedback addressed

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.gypi:101: {
On 2011/01/04 12:58:06, gfeher wrote:
> This target is only built for OS==win || OS==linux || OS==mac. I guess you
need
> to build it for all the OSes.

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.gypi:108:
'tools/build/generate_policy_source.py',
On 2011/01/04 12:58:06, gfeher wrote:
> Can you move this variable out into the target?

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:512: }
On 2011/01/04 12:58:06, gfeher wrote:
> Why are you adding this here?
This was missing in the JSON file but the constants were used in the code (the
policy is implemented). Now the policy constants are generated, so the policy
actually have to be in the JSON file for things to compile.

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:8: -h                generate
header file of policy constants
On 2011/01/04 12:58:06, gfeher wrote:
> Nit: turn this into --something?

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:70: ''')
On 2011/01/04 12:58:06, gfeher wrote:
> These multi-line string constants make it difficult to navigate in this source
> file. I have three suggestions to make this better:
> 
> f.write('\n'.join([
>    '#ifndef CHROME_COMMON_POLICY_CONSTANTS_H_',
>    '#define CHROME_COMMON_POLICY_CONSTANTS_H_',
>    '#pragma once',
>    ...])
> 
> or with implicit string joins:
> f.write(
>    '#ifndef CHROME_COMMON_POLICY_CONSTANTS_H_\n'
>    '#define CHROME_COMMON_POLICY_CONSTANTS_H_\n'
>    '#pragma once\n'
> )
> or define them at the beginning of the module like
> POLICY_CONSTANS_H_BEGINNING = '''....'''

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:80: f.closed
On 2011/01/04 12:58:06, gfeher wrote:
> f.close() ?

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:103: f.write('[] = "' + policy_name
+ '";\n')
On 2011/01/04 12:58:06, gfeher wrote:
> I suggest the following for the above two lines:
> f.write('const char k%s[] = "%s";\n' %
>         (policy_name, policy_name))

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:147: f.closed
On 2011/01/04 12:58:06, gfeher wrote:
> Didn't you mean f.close() ?

Done.

http://codereview.chromium.org/6002015/diff/5001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:147: f.closed
On 2011/01/04 13:13:17, gfeher wrote:
> Okay, sorry, now I've looked up the with statement, so I understand why you
> don't need f.close().
> 
> But still, why do you need f.closed?

Done.

[gfeher, 2011-01-11 15:43:58]

LG

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:783: Disabling this setting allows
cookies to be set by web page elements that are not from the domain that is in
thebrowser's address bar and prevents users from changing this settings.
nit: the browser's

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.gypi:13:
'tools/build/generate_policy_source.py',
Nit suggestion: make this ['python', 'tools/build/generate_policy_source.py']

And refer to it as <@(generate_policy_source_script) in actions.

http://codereview.chromium.org/6002015/diff/25001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_store_interface.h:10: #include
"chrome/browser/policy/configuration_policy_type.h"
I am not sure, but would it make sense to mark it here that this header is
generated from policy_templates.json? (I see coders desperately searching for
the source of policy constants.)

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:55: def
OutputGeneratedWarningForC(f):
Please add weak "internal use" indicator to the name of this function.
(underscore at the beginning)

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:93:
'L"SOFTWARE\\Policies\\Google\\Chrome";\n'
Please extract this and the other subkey into a global constant, e.g.:
CHROME_SUBKEY = "SOFTWARE\\Policies\\Google\\Chrome"
CHROMIUM_SUBKEY = ...

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:135: exec('data = ' + text, globs)
Nit: please replace this with return eval(text).

[Jakob Kummerow, 2011-01-14 14:59:21]

a couple o' nits

http://codereview.chromium.org/6002015/diff/59001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/59001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:1: #!/usr/bin/python2.4
python2.4 is a bit dated. Is that necessary for the buildbots? (If they run
Lucid, it's not.)

http://codereview.chromium.org/6002015/diff/59001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:2: # Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/6002015/diff/59001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:7: Usage: pyhton
generate_policy_source.py [switches] input_file
python

(On another note, I find the optparse module easier to use than getopt, but
that's a matter of personal preference.)

http://codereview.chromium.org/6002015/diff/59001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:65: '// ITS IS GENERATED BY
generate_policy_source.py\n'
s/ITS IS/IT IS/

[danno, 2011-01-20 17:18:32]

Please review again. Brad, can you please look at the .gyp magic to make the
generated code work.

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:783: Disabling this setting allows
cookies to be set by web page elements that are not from the domain that is in
thebrowser's address bar and prevents users from changing this settings.
On 2011/01/11 15:43:58, gfeher wrote:
> nit: the browser's

Done.

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.gypi:13:
'tools/build/generate_policy_source.py',
On 2011/01/11 15:43:58, gfeher wrote:
> Nit suggestion: make this ['python', 'tools/build/generate_policy_source.py']
> 
> And refer to it as <@(generate_policy_source_script) in actions.

Done.

http://codereview.chromium.org/6002015/diff/25001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_store_interface.h:10: #include
"chrome/browser/policy/configuration_policy_type.h"
On 2011/01/11 15:43:58, gfeher wrote:
> I am not sure, but would it make sense to mark it here that this header is
> generated from policy_templates.json? (I see coders desperately searching for
> the source of policy constants.)

Done.

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:55: def
OutputGeneratedWarningForC(f):
On 2011/01/11 15:43:58, gfeher wrote:
> Please add weak "internal use" indicator to the name of this function.
> (underscore at the beginning)

Done.

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:93:
'L"SOFTWARE\\Policies\\Google\\Chrome";\n'
On 2011/01/11 15:43:58, gfeher wrote:
> Please extract this and the other subkey into a global constant, e.g.:
> CHROME_SUBKEY = "SOFTWARE\\Policies\\Google\\Chrome"
> CHROMIUM_SUBKEY = ...

Done.

http://codereview.chromium.org/6002015/diff/25001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:135: exec('data = ' + text, globs)
On 2011/01/11 15:43:58, gfeher wrote:
> Nit: please replace this with return eval(text).

Done.

[bradn, 2011-01-20 17:57:01]

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:54: '<(policy_out_dir)',
I don't think you want to litter these all over.
You can use direct_dependent_settings to export this from the policy target. If
_really_ need to propagate to all transitive dependencies you could use
all_dependent_settings (but I strongly discourage this, as its a symptom of
headers that end up including the universe).

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:189: 'dependenciLles': [
?

http://codereview.chromium.org/6002015/diff/79001/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/6002015/diff/79001/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:194: '<(SHARED_INTERMEDIATE_DIR)/policy',
Have the policy targets export this rather than have users of policy need to
know to have it.

http://codereview.chromium.org/6002015/diff/79001/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:900: '<(SHARED_INTERMEDIATE_DIR)/policy',
Get this from policy.

[danno, 2011-01-24 18:14:22]

addressed feedback

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:54: '<(policy_out_dir)',
On 2011/01/20 17:57:02, bradn wrote:
> I don't think you want to litter these all over.
> You can use direct_dependent_settings to export this from the policy target.
If
> _really_ need to propagate to all transitive dependencies you could use
> all_dependent_settings (but I strongly discourage this, as its a symptom of
> headers that end up including the universe).

Done.

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:189: 'dependenciLles': [
On 2011/01/20 17:57:02, bradn wrote:
> ?

Done.

http://codereview.chromium.org/6002015/diff/79001/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:189: 'dependenciLles': [
On 2011/01/20 17:57:02, bradn wrote:
> ?

Done.

http://codereview.chromium.org/6002015/diff/79001/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/6002015/diff/79001/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:194: '<(SHARED_INTERMEDIATE_DIR)/policy',
On 2011/01/20 17:57:02, bradn wrote:
> Have the policy targets export this rather than have users of policy need to
> know to have it.

Done.

http://codereview.chromium.org/6002015/diff/79001/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:900: '<(SHARED_INTERMEDIATE_DIR)/policy',
On 2011/01/20 17:57:02, bradn wrote:
> Get this from policy.

Done.

[bradn, 2011-01-24 18:17:58]

LGTM on the gyp.

[Mattias Nissler (ping if slow), 2011-01-25 11:42:21]

Some comments.

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode41
chrome/chrome.gyp:41: 'policy_win64',
why this?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode853
chrome/chrome.gyp:853: 'policy',
syncapi depends on policy? why?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode1795
chrome/chrome.gyp:1795: 'policy',
same question.

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_common.gypi#n...
chrome/chrome_common.gypi:581: 'policy_win64',
why?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:173: 'policy',
why do we need to make the dependency explicit here? test_support_common already
pulls in policy, no?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:246: 'policy',
test_support_common already includes it?

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:7: Usage: python
generate_policy_source.py [switches] platform input_file
what is platform? mention it in the doc string?

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:38: print "one and only one
platform and input file must be specified."
I suggest s/one and only/exactly/g

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:44: if option in ("--help", "-h"):
Seems like the loop doesn't make sense here. If I put --help after other
options, we'll still generate output. That's is not what I would expect. Also,
most of our python code uses optparse, which is much easier to use IMHO.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:55: print "not implemented"
Why have the option in the first place then? Can always add it in a later CL.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:67: '// FROM
policy_templates.json\n'
Actually, the json file to read is passed as a command line argument, so you
might want to put the correct name here.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:80: f.write('// The windows
registry path where policy configuration resides.\n'
80 chars

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:145: return eval(text)
Still no proper JSON parsing for policy templates? Sigh.

[Mattias Nissler (ping if slow), 2011-01-25 12:17:00]

Oh, please disregard the ^.*\.gypi?^ comments, I forgot to remove them.

On 2011/01/25 11:42:21, Mattias Nissler wrote:
> Some comments.
> 
> http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode41
> chrome/chrome.gyp:41: 'policy_win64',
> why this?
> 
> http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode853
> chrome/chrome.gyp:853: 'policy',
> syncapi depends on policy? why?
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode1795
> chrome/chrome.gyp:1795: 'policy',
> same question.
> 
> http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_common.gypi
> File chrome/chrome_common.gypi (right):
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_common.gypi#n...
> chrome/chrome_common.gypi:581: 'policy_win64',
> why?
> 
> http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi#ne...
> chrome/chrome_tests.gypi:173: 'policy',
> why do we need to make the dependency explicit here? test_support_common
already
> pulls in policy, no?
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi#ne...
> chrome/chrome_tests.gypi:246: 'policy',
> test_support_common already includes it?
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> File chrome/tools/build/generate_policy_source.py (right):
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:7: Usage: python
> generate_policy_source.py [switches] platform input_file
> what is platform? mention it in the doc string?
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:38: print "one and only one
> platform and input file must be specified."
> I suggest s/one and only/exactly/g
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:44: if option in ("--help",
"-h"):
> Seems like the loop doesn't make sense here. If I put --help after other
> options, we'll still generate output. That's is not what I would expect. Also,
> most of our python code uses optparse, which is much easier to use IMHO.
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:55: print "not implemented"
> Why have the option in the first place then? Can always add it in a later CL.
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:67: '// FROM
> policy_templates.json\n'
> Actually, the json file to read is passed as a command line argument, so you
> might want to put the correct name here.
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:80: f.write('// The windows
> registry path where policy configuration resides.\n'
> 80 chars
> 
>
http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
> chrome/tools/build/generate_policy_source.py:145: return eval(text)
> Still no proper JSON parsing for policy templates? Sigh.

[danno, 2011-01-25 14:27:28]

address mnisslers feedback

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode41
chrome/chrome.gyp:41: 'policy_win64',
The 64-bit nacl libraries need to link against 64-bit targets. Including
"policy_constants.h" also now requires a dependency to get the include
directory.

On 2011/01/25 11:42:21, Mattias Nissler wrote:
> why this?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode853
chrome/chrome.gyp:853: 'policy',
For include directory.
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> syncapi depends on policy? why?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome.gyp#newcode1795
chrome/chrome.gyp:1795: 'policy',
For include directory.
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> same question.

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_common.gypi#n...
chrome/chrome_common.gypi:581: 'policy_win64',
For include directory.
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> why?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:173: 'policy',
policy an indirect dependency, so we don't get the include directory by default.
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> why do we need to make the dependency explicit here? test_support_common
already
> pulls in policy, no?

http://codereview.chromium.org/6002015/diff/89001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:246: 'policy',
see above.
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> test_support_common already includes it?

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:7: Usage: python
generate_policy_source.py [switches] platform input_file
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> what is platform? mention it in the doc string?

Done.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:38: print "one and only one
platform and input file must be specified."
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> I suggest s/one and only/exactly/g

Done.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:44: if option in ("--help", "-h"):
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> Seems like the loop doesn't make sense here. If I put --help after other
> options, we'll still generate output. That's is not what I would expect. Also,
> most of our python code uses optparse, which is much easier to use IMHO.

Done.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:55: print "not implemented"
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> Why have the option in the first place then? Can always add it in a later CL.

Done.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:67: '// FROM
policy_templates.json\n'
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> Actually, the json file to read is passed as a command line argument, so you
> might want to put the correct name here.

Done.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:80: f.write('// The windows
registry path where policy configuration resides.\n'
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> 80 chars

Done.

http://codereview.chromium.org/6002015/diff/89001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:145: return eval(text)
nope!
On 2011/01/25 11:42:21, Mattias Nissler wrote:
> Still no proper JSON parsing for policy templates? Sigh.

[Mattias Nissler (ping if slow), 2011-01-25 16:29:35]

LGTM with a nit.