Add more polices to the protected list.

Add more polices to the protected list.

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

[pastarmovj, 2014-07-16 16:25:01]

Hi Joao can you please review this Change? Thanks!

[pastarmovj, 2014-07-16 16:26:40]

Adding jar for the histograms file.

[jar (doing other things), 2014-07-16 16:29:25]

Adding asvitkine for histograms.xml

[Alexei Svitkine (slow), 2014-07-16 17:42:56]

lgtm

[Joao da Silva, 2014-07-17 09:57:55]

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:76: const char*
kInsecurePolicies[] = {
Document

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:78:
policy::key::kDefaultSearchProviderEnabled,
Shouldn't we include all of the search provider policies?

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:82:
policy::key::kRestoreOnStartupURLs
policy:: isn't needed

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:172: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kInsecurePolicies); ++i) {
Doesn't arraysize() work here?

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:174:
policy->Erase(kInsecurePolicies[i]);
It would be cool to have this somehow surface in about:policy, so that admins
understand why these policies are being ignored. Unfortunately we don't have an
easy way to do that now... maybe leave a TODO here and file a bug for this?

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:181: }
This is in the wrong block: it shouldn't be conditional on the if() in line 137,
which is about the kExtensionInstallForcelist policy.

To make this code more clear, I suggest moving the special handling of
kExtensionInstallForcelist to its own separate function and call that from here.

[pastarmovj, 2014-07-17 14:12:59]

PTAL.

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:76: const char*
kInsecurePolicies[] = {
On 2014/07/17 09:57:55, Joao da Silva wrote:
> Document

Done.

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:78:
policy::key::kDefaultSearchProviderEnabled,
On 2014/07/17 09:57:55, Joao da Silva wrote:
> Shouldn't we include all of the search provider policies?

They are gated on this one can be a useful lead if we get a problem reported if
they are present but this one isn't.

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:82:
policy::key::kRestoreOnStartupURLs
On 2014/07/17 09:57:55, Joao da Silva wrote:
> policy:: isn't needed

Done.

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:172: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kInsecurePolicies); ++i) {
On 2014/07/17 09:57:55, Joao da Silva wrote:
> Doesn't arraysize() work here?

Done.

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:174:
policy->Erase(kInsecurePolicies[i]);
On 2014/07/17 09:57:55, Joao da Silva wrote:
> It would be cool to have this somehow surface in about:policy, so that admins
> understand why these policies are being ignored. Unfortunately we don't have
an
> easy way to do that now... maybe leave a TODO here and file a bug for this?

Done.

https://codereview.chromium.org/392333003/diff/1/components/policy/core/commo...
components/policy/core/common/policy_loader_win.cc:181: }
On 2014/07/17 09:57:55, Joao da Silva wrote:
> This is in the wrong block: it shouldn't be conditional on the if() in line
137,
> which is about the kExtensionInstallForcelist policy.
> 
> To make this code more clear, I suggest moving the special handling of
> kExtensionInstallForcelist to its own separate function and call that from
here.

Done.

[Joao da Silva, 2014-07-17 16:18:44]

lgtm

Can we extend the unit tests for this class to verify that one of these policies
is indeed getting skipped in non-AD machines? Not sure how easy it is to mock
that call.

[pastarmovj, 2014-07-18 11:21:58]

Thanks! I can add such test now because the call is now easily mockable (it was
not when the original code was written) but I will do so in a second CL to keep
this open for potential merges if decided so.

[pastarmovj, 2014-07-18 11:22:03]

The CQ bit was checked by pastarmovj@chromium.org

[commit-bot: I haz the power, 2014-07-18 11:22:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/392333003/20001

[commit-bot: I haz the power, 2014-07-18 13:24:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-18 14:09:03]

Change committed as 284086