Move chrome/app/policy into components/policy.

Move chrome/app/policy into components/policy.

This move enables using the files derived from policy_templates.json in iOS
builds. List of changes:

- cloud_policy_codegen.gyp and policy_templates.gypi have been merged into
  components/policy.gypi

- policy_templates.json moved into components/policy/resources/

- policy_templates.grd and *.xtb files moved into components/policy/resources/

- syntax_check_policy_templates_json.py and
  chrome/tools/build/generate_policy_source.py moved into
  components/policy/tools

BUG=271392
R=jochen@chromium.org, pastarmovj@chromium.org

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

[Joao da Silva, 2013-12-11 22:49:39]

Ready for a first look. The CL became a mess because of the XTB files bloating
the patch size :-( If you'd like then I can move then temporarily out of the CL
for ease of reviewing.

PTAL, thanks!

[Joao da Silva, 2013-12-12 08:09:28]

Patch set 4 just moves the XTB files; patch set 3 is the one that should be
reviewed.

[Joao da Silva, 2013-12-12 08:27:00]

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi
File components/policy.gypi (right):

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:113: # Maybe use 'export_dependent_settings' to make it
easier
Meh, forgot to remove these comments to self; I'll remove before landing.

Anyway, this CL is only about moving things; I'll see if merging
policy+policy_component makes sense later (note that 'policy' is used by many
targets just to use the policy names, so maybe they can't get merged in a simple
way; I'm thinking of relying on configuration_policy==(0|1) to conditionally
include things)

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:365: # TODO: consider merging with
policy_component_test_support
Same logic here

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:554: 'msvs_cygwin_shell': 1,
This breaks the presubmit scripts but can be safely ignored.

[Mattias Nissler (ping if slow), 2013-12-12 14:11:36]

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi
File components/policy.gypi (right):

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:113: # Maybe use 'export_dependent_settings' to make it
easier
On 2013/12/12 08:27:02, Joao da Silva wrote:
> Meh, forgot to remove these comments to self; I'll remove before landing.
> 
> Anyway, this CL is only about moving things; I'll see if merging
> policy+policy_component makes sense later (note that 'policy' is used by many
> targets just to use the policy names, so maybe they can't get merged in a
simple
> way; I'm thinking of relying on configuration_policy==(0|1) to conditionally
> include things)

FWIW, if this gets built even if configuratin_policy==0, then we should at least
remove the protobuf_decoder_path line below and move that into policy_component.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:114: 'target_name': 'policy',
It'd be great if we could rename this to policy_constants for clarity - not
blocking for this CL though.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:365: # TODO: consider merging with
policy_component_test_support
I think in this case, it might be simpler to just add
chrome_settings_proto_generated_compile as a dependency to component_tests and
get rid of this target? There's really only one test that needs the file, no
need to make it available to other test binaries.

For now: Remove the comment or add period.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:385: 'target_name': 'policy_win64',
If you rename the other one, this should be policy_constants_win64 as well.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:504: '../chrome/VERSION',
is this OK to have as dependency in a component?

[Joao da Silva, 2013-12-12 14:53:20]

Requesting input from the owners of components/:

A zip file with documentation and Windows ADM files is being assembled from data
in components/policy/resources/. That zip also includes the chrome version, from
chrome/VERSION.

The question is, is it alright to include chrome/ data like this from the
component?

Some options:

1. yes, it's fine (is it?)

2. don't mention chrome/ files by name. Introduce a variable in
build/common.gypi or build/chrome_settings.gypi that maps to the VERSION file
path, and use that variable instead of the explicit path in
components/policy.gypi

3. don't reach out to chrome/ data and move this target out of
components/policy.gypi into chrome/somewhere.gypi.

Jói, Jochen, WDYT?

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi
File components/policy.gypi (right):

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:113: # Maybe use 'export_dependent_settings' to make it
easier
On 2013/12/12 14:11:38, Mattias Nissler wrote:
> On 2013/12/12 08:27:02, Joao da Silva wrote:
> > Meh, forgot to remove these comments to self; I'll remove before landing.
> > 
> > Anyway, this CL is only about moving things; I'll see if merging
> > policy+policy_component makes sense later (note that 'policy' is used by
many
> > targets just to use the policy names, so maybe they can't get merged in a
> simple
> > way; I'm thinking of relying on configuration_policy==(0|1) to conditionally
> > include things)
> 
> FWIW, if this gets built even if configuratin_policy==0, then we should at
least
> remove the protobuf_decoder_path line below and move that into
policy_component.

Agreed. Let's leave it for another CL to reduce risk on this one.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:114: 'target_name': 'policy',
On 2013/12/12 14:11:38, Mattias Nissler wrote:
> It'd be great if we could rename this to policy_constants for clarity - not
> blocking for this CL though.

SGTM. If this gets merged into policy_component then we could have a single
'policy' target that includes the right stuff for both configuration_policy==1
or ==0.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:365: # TODO: consider merging with
policy_component_test_support
On 2013/12/12 14:11:38, Mattias Nissler wrote:
> I think in this case, it might be simpler to just add
> chrome_settings_proto_generated_compile as a dependency to component_tests and
> get rid of this target? There's really only one test that needs the file, no
> need to make it available to other test binaries.
> 
> For now: Remove the comment or add period.

Removed the comment.

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:385: 'target_name': 'policy_win64',
On 2013/12/12 14:11:38, Mattias Nissler wrote:
> If you rename the other one, this should be policy_constants_win64 as well.

ack

https://codereview.chromium.org/108513011/diff/20001/components/policy.gypi#n...
components/policy.gypi:504: '../chrome/VERSION',
On 2013/12/12 14:11:38, Mattias Nissler wrote:
> is this OK to have as dependency in a component?

Good catch.

This certainly works, and we can add an indirection to the filename through
build/common.gypi or build/chrome_settings.gypi.

[jochen (gone - plz use gerrit), 2013-12-12 15:14:05]

On 2013/12/12 14:53:20, Joao da Silva wrote:
> Requesting input from the owners of components/:
> 
> A zip file with documentation and Windows ADM files is being assembled from
data
> in components/policy/resources/. That zip also includes the chrome version,
from
> chrome/VERSION.
> 
> The question is, is it alright to include chrome/ data like this from the
> component?
> 
> Some options:
> 
> 1. yes, it's fine (is it?)
> 
> 2. don't mention chrome/ files by name. Introduce a variable in
> build/common.gypi or build/chrome_settings.gypi that maps to the VERSION file
> path, and use that variable instead of the explicit path in
> components/policy.gypi
> 
> 3. don't reach out to chrome/ data and move this target out of
> components/policy.gypi into chrome/somewhere.gypi.
> 
> Jói, Jochen, WDYT?

1) is not ok

an alternative would be to move chrome/VERSION to content/

[Joao da Silva, 2013-12-12 15:17:22]

> an alternative would be to move chrome/VERSION to content/

That would work. Since a lot of places and scripts (and possibly external
scripts) refer to that file I'd rather not move it.

I'd go for (2) if you agree, otherwise I'll do (3) then.

[jochen (gone - plz use gerrit), 2013-12-12 15:22:20]

On 2013/12/12 15:17:22, Joao da Silva wrote:
> > an alternative would be to move chrome/VERSION to content/
> 
> That would work. Since a lot of places and scripts (and possibly external
> scripts) refer to that file I'd rather not move it.
> 
> I'd go for (2) if you agree, otherwise I'll do (3) then.

I think (3) is better than (2). (2) is just hiding the layering violation.

[Jói, 2013-12-12 16:07:25]

I agree (3) is better.

On Thu, Dec 12, 2013 at 3:22 PM,  <jochen@chromium.org> wrote:
> On 2013/12/12 15:17:22, Joao da Silva wrote:
>>
>> > an alternative would be to move chrome/VERSION to content/
>
>
>> That would work. Since a lot of places and scripts (and possibly external
>> scripts) refer to that file I'd rather not move it.
>
>
>> I'd go for (2) if you agree, otherwise I'll do (3) then.
>
>
> I think (3) is better than (2). (2) is just hiding the layering violation.
>
> https://codereview.chromium.org/108513011/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Joao da Silva, 2013-12-13 10:51:19]

This is ready for another look:

- components/policy.gypi does not reach out to ../chrome/VERSION anymore

- the 'policy_templates' target only generates the templates

- the new 'pack_policy_templates' target from chrome/policy_templates.gypi
creates the zip with the VERSION file now

Please review patch set 7. Patch set 8 just moved the .xtb files without
changes.

@Julian: please review the policy changes (Mattias is OOO)

@Jochen: please review the chrome/ gyp changes

Thanks!

[Joao da Silva, 2013-12-13 10:51:50]

+Julian, please see the previous comment :-)

[jochen (gone - plz use gerrit), 2013-12-13 11:46:32]

lgtm

[pastarmovj, 2013-12-13 11:50:57]

lgtm

[commit-bot: I haz the power, 2013-12-13 15:18:17]

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

[commit-bot: I haz the power, 2013-12-13 15:51:48]

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

[Joao da Silva, 2013-12-16 13:49:31]

Committed patchset #15 manually as r240875.