Fix path_parser dependency on policy component

Fix path_parser dependency on policy component

BUG=

Committed: https://crrev.com/151f782cd1b1772430ef3bb97bc5414cd1fd5647
Cr-Commit-Position: refs/heads/master@{#362684}

[Tomasz Moniuszko, 2015-05-11 08:39:35]

tmoniuszko@opera.com changed reviewers:
+ brettw@chromium.org, mnissler@chromium.org

[Mattias Nissler (ping if slow), 2015-05-11 08:49:30]

Arguably, chrome/browser/policy/BUILD.gn shouldn't build anything if
enable_configuration_policy=0.

I have a related question: We've recently looked into refactoring the build
files for the policy component, and realized that enable_configuration_policy=0
adds a lot of complexity and is in fact broken right now. As a result, I was
checking with people on whether it's safe to remove the
enable_configuration_policy build flag, as Android (which was the  motivation to
introduce it) no longer uses it. Is this CL an indication that you actually rely
on it?

[Tomasz Moniuszko, 2015-05-11 14:37:32]

On 2015/05/11 08:49:30, Mattias Nissler wrote:
> Arguably, chrome/browser/policy/BUILD.gn shouldn't build anything if
> enable_configuration_policy=0.

Done.

> 
> I have a related question: We've recently looked into refactoring the build
> files for the policy component, and realized that
enable_configuration_policy=0
> adds a lot of complexity and is in fact broken right now. As a result, I was
> checking with people on whether it's safe to remove the
> enable_configuration_policy build flag, as Android (which was the  motivation
to
> introduce it) no longer uses it. Is this CL an indication that you actually
rely
> on it?

Yes, we set enable_configuration_policy to false in some configurations.

[Mattias Nissler (ping if slow), 2015-05-11 15:35:07]

On 2015/05/11 14:37:32, Tomasz Moniuszko wrote:
> On 2015/05/11 08:49:30, Mattias Nissler wrote:
> > Arguably, chrome/browser/policy/BUILD.gn shouldn't build anything if
> > enable_configuration_policy=0.
> 
> Done.
> 
> > 
> > I have a related question: We've recently looked into refactoring the build
> > files for the policy component, and realized that
> enable_configuration_policy=0
> > adds a lot of complexity and is in fact broken right now. As a result, I was
> > checking with people on whether it's safe to remove the
> > enable_configuration_policy build flag, as Android (which was the 
motivation
> to
> > introduce it) no longer uses it. Is this CL an indication that you actually
> rely
> > on it?
> 
> Yes, we set enable_configuration_policy to false in some configurations.

Can you elaborate on what configurations are important to you?

Given our experience with the configuration breaking on the time, I feel that we
need a better strategy to keep the configuration alive if we want to keep it.

[Tomasz Moniuszko, 2015-05-12 07:30:39]

On 2015/05/11 15:35:07, Mattias Nissler wrote:
> On 2015/05/11 14:37:32, Tomasz Moniuszko wrote:
> > On 2015/05/11 08:49:30, Mattias Nissler wrote:
> > > Arguably, chrome/browser/policy/BUILD.gn shouldn't build anything if
> > > enable_configuration_policy=0.
> > 
> > Done.
> > 
> > > 
> > > I have a related question: We've recently looked into refactoring the
build
> > > files for the policy component, and realized that
> > enable_configuration_policy=0
> > > adds a lot of complexity and is in fact broken right now. As a result, I
was
> > > checking with people on whether it's safe to remove the
> > > enable_configuration_policy build flag, as Android (which was the 
> motivation
> > to
> > > introduce it) no longer uses it. Is this CL an indication that you
actually
> > rely
> > > on it?
> > 
> > Yes, we set enable_configuration_policy to false in some configurations.
> 
> Can you elaborate on what configurations are important to you?
> 
> Given our experience with the configuration breaking on the time, I feel that
we
> need a better strategy to keep the configuration alive if we want to keep it.

I had some discussion about it with dpranke. I will forward it to you via
e-mail.

[Tomasz Moniuszko, 2015-11-23 11:33:08]

I rebased the patch to current master. This review is hanging for few months
already. Several similar patches were accepted meantime. Is there a chance that
this patch can be accepted as well?

[Mattias Nissler (ping if slow), 2015-11-23 12:07:17]

I'm fine with this going in, but I'm no longer a policy OWNER. Sorry for not
following up on this CL earlier, it had simply fallen off my radar. Feel free to
ping more aggressively if reviewers are not responsive.

[Tomasz Moniuszko, 2015-11-23 12:51:54]

tmoniuszko@opera.com changed reviewers:
+ bartfab@chromium.org

[Tomasz Moniuszko, 2015-11-23 12:53:47]

On 2015/11/23 12:07:17, Mattias Nissler wrote:
> I'm fine with this going in, but I'm no longer a policy OWNER. Sorry for not
> following up on this CL earlier, it had simply fallen off my radar. Feel free
to
> ping more aggressively if reviewers are not responsive.

OK. Thanks for notifying.

@bartfab: Could you please take a look at this patch?

[bartfab (slow), 2015-11-25 16:05:46]

How about the corresponding gypi files?

[brettw, 2015-11-25 21:05:16]

On 2015/11/25 16:05:46, bartfab (slow) wrote:
> How about the corresponding gypi files?

Agreed, it looks like the old structure matches GYP, we should keep them in
sync.

[Tomasz Moniuszko, 2015-12-01 13:34:06]

On 2015/11/25 16:05:46, bartfab (slow) wrote:
> How about the corresponding gypi files?

Done (Patch Set 4).

[brettw, 2015-12-01 17:49:06]

lgtm

https://codereview.chromium.org/1133853005/diff/60001/chrome/browser/policy/B...
File chrome/browser/policy/BUILD.gn (right):

https://codereview.chromium.org/1133853005/diff/60001/chrome/browser/policy/B...
chrome/browser/policy/BUILD.gn:7: if (enable_configuration_policy) {
This if statement doesn't do anything. The only way GN will load a BUILD file is
if somebody references a target in it, and if configuration policy is disabled,
there will be no targets.

If you're trying to prevent accidents, the best thing to do is add at the top:

assert(enable_configuration_policy)

[Tomasz Moniuszko, 2015-12-02 10:23:05]

https://codereview.chromium.org/1133853005/diff/60001/chrome/browser/policy/B...
File chrome/browser/policy/BUILD.gn (right):

https://codereview.chromium.org/1133853005/diff/60001/chrome/browser/policy/B...
chrome/browser/policy/BUILD.gn:7: if (enable_configuration_policy) {
On 2015/12/01 17:49:06, brettw wrote:
> This if statement doesn't do anything. The only way GN will load a BUILD file
is
> if somebody references a target in it, and if configuration policy is
disabled,
> there will be no targets.
> 
> If you're trying to prevent accidents, the best thing to do is add at the top:
> 
> assert(enable_configuration_policy)

Done.

[Tomasz Moniuszko, 2015-12-02 10:26:38]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2015-12-02 10:26:38]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1133853005/#ps80001
(title: "Use assert instead of condition")

[commit-bot: I haz the power, 2015-12-02 10:27:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133853005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133853005/80001

[commit-bot: I haz the power, 2015-12-02 11:35:06]

Description was changed from

==========
Fix path_parser dependency on policy component

BUG=
==========

to

==========
Fix path_parser dependency on policy component

BUG=
==========

[commit-bot: I haz the power, 2015-12-02 11:35:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-02 11:35:43]

Description was changed from

==========
Fix path_parser dependency on policy component

BUG=
==========

to

==========
Fix path_parser dependency on policy component

BUG=

Committed: https://crrev.com/151f782cd1b1772430ef3bb97bc5414cd1fd5647
Cr-Commit-Position: refs/heads/master@{#362684}
==========

[commit-bot: I haz the power, 2015-12-02 11:35:44]

Patchset 5 (id:??) landed as
https://crrev.com/151f782cd1b1772430ef3bb97bc5414cd1fd5647
Cr-Commit-Position: refs/heads/master@{#362684}