Define EnabledComponentUpdates group policy for the component updater.

Define EnabledComponentUpdates group policy for the component updater.

BUG=624128
Committed: https://crrev.com/0b99b8d3c1cc918f43923e5566d7fcb4fae66d13
Cr-Commit-Position: refs/heads/master@{#413526}

[Sorin Jianu, 2016-08-19 05:15:14]

sorin@chromium.org changed reviewers:
+ pastarmovj@chromium.org, waffles@chromium.org

[Sorin Jianu, 2016-08-19 05:15:15]

Julian, could you please do an owner's review for the group policy changes?

The change is dependent on a patch, which includes mechanical changes in the
component updater. This change will be rebased when the dependent change lands
before this one.

I am just trying to buy some time before the weekend in USTZ and hopefully land
Friday, USTZ.

Thank you!

[Thiemo Nagel, 2016-08-19 10:38:29]

tnagel@chromium.org changed reviewers:
+ tnagel@chromium.org

[Thiemo Nagel, 2016-08-19 10:38:30]

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8856: 'name':
'EnabledComponentUpdates',
Please move "Enabled" to the end of the name to be consistent with the dominant
style.  Also it seems that the name doesn't properly capture the functionality
of the policy.  I'd suggest instead: BinaryComponentUpdatesEnabled

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8871: The updates for some
components can't be disabled. These components are considered non-binary and
they usually contain only data and no code.''',
"some components" sounds as if we were trying to hide something.  Could you
please qualify these more precisely or include at least a couple of examples? 
Also "usually" seems suspicious and imho requires more explanation.

[pastarmovj, 2016-08-19 14:06:13]

+1 to Thiemo's comments. Otherwise I am ok with the change and will stamp it
once the wording has been fixed.

[waffles, 2016-08-19 16:42:03]

lgtm

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8871: The updates for some
components can't be disabled. These components are considered non-binary and
they usually contain only data and no code.''',
On 2016/08/19 10:38:29, Thiemo Nagel (ooo) wrote:
> "some components" sounds as if we were trying to hide something.  Could you
> please qualify these more precisely or include at least a couple of examples? 
> Also "usually" seems suspicious and imho requires more explanation.

I recommend that we instead drop every occurrence of "binary" here - whether a
component is binary or text is simply an artifact of its encoding, and isn't
really meaningful about what the component is/does and therefore whether we can
disable it or not. For example, we intend to continue shipping safe-browsing
data as a binary protocol buffer, even if this policy is true.

I don't think we have a future-proof one-word description of what components can
and can't be disabled, so I would rather leave all qualifiers out of the policy
name (just "ComponentUpdatesEnabled"), and write the description along the lines
of the following:

"If set to False, updates to components are disabled. However, some components
are exempt from this policy: updates to any component that does not contain
executable code or does not significantly alter the behavior of the browser will
not be disabled. Examples of such components include the certificate revocation
lists and safe browsing data."

There's still a weasel-word in here ("significantly" - what is significant?)
which gives us some flexibility in using our judgment about what components
should be affected by this policy (even if they contain executable code), but it
removes the false binary/text distinction.

[Sorin Jianu, 2016-08-19 20:05:45]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:06:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sorin Jianu, 2016-08-19 20:08:47]

Uploaded a new patch after the rebase and responding to comments.

Thiemo and Julian, PTAL for the policy changes.

Thank you!

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8856: 'name':
'EnabledComponentUpdates',
On 2016/08/19 10:38:29, Thiemo Nagel (ooo) wrote:
> Please move "Enabled" to the end of the name to be consistent with the
dominant
> style.  Also it seems that the name doesn't properly capture the functionality
> of the policy.  I'd suggest instead: BinaryComponentUpdatesEnabled

Changed the name to match the style. 
We struggled with naming. The idea is that only a set of components are affected
by this policy and we could not find a qualifier that unambiguously describes
the scope of policy. Therefore, we prefer to leave the qualifier out and comment
more on the intention below. Would this work?

https://codereview.chromium.org/2257363002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8871: The updates for some
components can't be disabled. These components are considered non-binary and
they usually contain only data and no code.''',
On 2016/08/19 10:38:29, Thiemo Nagel (ooo) wrote:
> "some components" sounds as if we were trying to hide something.  Could you
> please qualify these more precisely or include at least a couple of examples? 
> Also "usually" seems suspicious and imho requires more explanation.

Done.

[commit-bot: I haz the power, 2016-08-19 21:23:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 21:23:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sorin Jianu, 2016-08-19 22:18:38]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 22:19:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 23:28:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 23:28:03]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2016-08-22 11:30:46]

policy LGTM with two nits.

https://codereview.chromium.org/2257363002/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2257363002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:8850: 'name':
'ComponentUpdates',
nit: Remove the group for now. I think we don't need a group for one policy
only. Shall we add more we can always add the group as it has no representation
in the GPO but only in the policy editor UI so we can always reconsider this
decision.

https://codereview.chromium.org/2257363002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:8870: If set to False, updates
to components are disabled. However, some components are exempt from this
policy: updates to any component that does not contain executable code or does
not significantly alter the behavior of the browser will not be disabled.
nit: if you wish to further justify this decision you can also add "does not
significantly alter the behavior of the browser...and are critical for its
security" (or some such) which is true for both examples below.

[Sorin Jianu, 2016-08-22 17:32:32]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 17:33:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sorin Jianu, 2016-08-22 17:35:55]

Thank you Julian, all done.

https://codereview.chromium.org/2257363002/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2257363002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:8850: 'name':
'ComponentUpdates',
On 2016/08/22 11:30:45, pastarmovj wrote:
> nit: Remove the group for now. I think we don't need a group for one policy
> only. Shall we add more we can always add the group as it has no
representation
> in the GPO but only in the policy editor UI so we can always reconsider this
> decision.

Done.

https://codereview.chromium.org/2257363002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:8870: If set to False, updates
to components are disabled. However, some components are exempt from this
policy: updates to any component that does not contain executable code or does
not significantly alter the behavior of the browser will not be disabled.
On 2016/08/22 11:30:45, pastarmovj wrote:
> nit: if you wish to further justify this decision you can also add "does not
> significantly alter the behavior of the browser...and are critical for its
> security" (or some such) which is true for both examples below.

Done.

[Sorin Jianu, 2016-08-22 17:41:39]

The CQ bit was unchecked by sorin@chromium.org

[Sorin Jianu, 2016-08-22 17:47:46]

The CQ bit was checked by sorin@chromium.org

[Sorin Jianu, 2016-08-22 17:47:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2257363002/#ps60001
(title: "Removed policy group. Reworded comment.")

[commit-bot: I haz the power, 2016-08-22 17:48:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 17:56:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 17:56:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sorin Jianu, 2016-08-22 18:05:11]

sorin@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Sorin Jianu, 2016-08-22 18:05:11]

Robert, I need an owner's approval for for the histogram changes. Thank you!

[rkaplow, 2016-08-22 18:09:48]

lgtm

[Sorin Jianu, 2016-08-22 18:10:51]

sorin@chromium.org changed reviewers:
+ asvitkine@chromium.org
- rkaplow@chromium.org

[Sorin Jianu, 2016-08-22 18:10:51]

Alexei, can you please review the histograms.xml change? Robert may be slow to
respond. I am sorry for the noise.

[Sorin Jianu, 2016-08-22 18:11:42]

sorin@chromium.org changed reviewers:
+ rkaplow@chromium.org
- asvitkine@chromium.org

[Sorin Jianu, 2016-08-22 18:11:58]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2016-08-22 18:12:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 20:41:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-22 20:44:29]

Description was changed from

==========
Define EnabledComponentUpdates group policy for the component updater.

BUG=624128
==========

to

==========
Define EnabledComponentUpdates group policy for the component updater.

BUG=624128

Committed: https://crrev.com/0b99b8d3c1cc918f43923e5566d7fcb4fae66d13
Cr-Commit-Position: refs/heads/master@{#413526}
==========

[commit-bot: I haz the power, 2016-08-22 20:44:31]

Patchset 4 (id:??) landed as
https://crrev.com/0b99b8d3c1cc918f43923e5566d7fcb4fae66d13
Cr-Commit-Position: refs/heads/master@{#413526}