Policy: Ignore ForceSafeSearch if ForceGoogleSafeSearch or ForceYoutubeSafetyMode are enabled

Enterprise policy: Ignore the deprecated ForceSafeSearch if ForceGoogleSafeSearch or ForceYoutubeSafetyMode are enabled

While we're here, remove the deprecated prefs::kForceSafeSearch and instead map the old policy to the new prefs

TBR=cbentzel@chromium.org for trivial io_thread.cc change

BUG=476908
Committed: https://crrev.com/2b18805daa18a3bc4d9892f50e44d5cfacdad451
Cr-Commit-Position: refs/heads/master@{#325620}

[Marc Treib, 2015-04-14 13:27:14]

treib@chromium.org changed reviewers:
+ atwilson@chromium.org

[Marc Treib, 2015-04-14 13:27:15]

https://codereview.chromium.org/1056003003/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
I guess this part isn't *strictly* required for the migration case, but
consistency and all that... :)

[Marc Treib, 2015-04-14 14:16:43]

treib@chromium.org changed reviewers:
+ battre@chromium.org

[Marc Treib, 2015-04-14 14:16:44]

+battre for c/b/net

[Thiemo Nagel, 2015-04-14 14:39:57]

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

[Thiemo Nagel, 2015-04-14 14:39:58]

Thanks for the fast CL!

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
This yields force_safe_search=true for FSS=1, FGSS=0 and FYTSM=undef, but the
correct value in that case would be force_safe_search=false.

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:434: if (!force_safety_mode &&
See above.

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:221: bool safe_search = i
% 2;
For full test coverage, these bools most be converted to tri-states: true,
false, unset.

[Marc Treib, 2015-04-14 14:47:38]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/14 14:39:58, Thiemo Nagel wrote:
> This yields force_safe_search=true for FSS=1, FGSS=0 and FYTSM=undef, but the
> correct value in that case would be force_safe_search=false.

Note that any of these being null does *not* mean "the policy has no defined
value", it just means "this member hasn't been initialized yet".
(The three are also initialized together:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...)
At this point, we can't tell "policy is explicitly disabled" from "policy is not
set".

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:221: bool safe_search = i
% 2;
On 2015/04/14 14:39:58, Thiemo Nagel wrote:
> For full test coverage, these bools most be converted to tri-states: true,
> false, unset.

See above: There really is no tri-state logic here.

[Thiemo Nagel, 2015-04-14 14:55:37]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/14 14:47:38, Marc Treib wrote:
> At this point, we can't tell "policy is explicitly disabled" from "policy is
not
> set".

Can't we use PrefMember<>::IsManaged() for that purpose?

[Marc Treib, 2015-04-14 15:02:01]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/14 14:55:37, Thiemo Nagel wrote:
> On 2015/04/14 14:47:38, Marc Treib wrote:
> > At this point, we can't tell "policy is explicitly disabled" from "policy is
> not
> > set".
> 
> Can't we use PrefMember<>::IsManaged() for that purpose?

Right, that should work! Might be a bit awkward to test though.
I'll update the CL tomorrow, have to run now...

[Andrew T Wilson (Slow), 2015-04-14 15:02:05]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
I actually don't think we want force_safe_search to depend on youtube safety
mode, but I'll defer to Thiemo here. If we do, then we probably just want to
depend on the *presence* of it, not the actual value (so, even if they set
force_google_safe_search to false, it should override the force_safe_search
value.

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:438: force_safety_mode =
(force_safe_search_ && force_safe_search_->GetValue());
Ditto here - we want people to be able to set force_youtube_safety_mode_ = false
and always have it take effect, even if forceSafeSearch = true.

[Thiemo Nagel, 2015-04-14 15:27:31]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/14 15:02:05, Andrew T Wilson wrote:
> I actually don't think we want force_safe_search to depend on youtube safety
> mode, but I'll defer to Thiemo here.

If someone used FSS for the sake of YouTube Safety Mode and thus sets FSS=1,
FGSS=undef and FYTSM=1, we probably don't want to force Safe Search on them. 
[FSS=1, FGSS=undef, FYTSM=1] should be symmetric to [FSS=1, FGSS=1,
FYTSM=undef].

> If we do, then we probably just want to depend on the *presence* of it,

Absolutely.

[Mattias Nissler (ping if slow), 2015-04-15 07:48:30]

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

[Mattias Nissler (ping if slow), 2015-04-15 07:48:31]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/14 15:27:31, Thiemo Nagel wrote:
> On 2015/04/14 15:02:05, Andrew T Wilson wrote:
> > I actually don't think we want force_safe_search to depend on youtube safety
> > mode, but I'll defer to Thiemo here.
> 
> If someone used FSS for the sake of YouTube Safety Mode and thus sets FSS=1,
> FGSS=undef and FYTSM=1, we probably don't want to force Safe Search on them. 
> [FSS=1, FGSS=undef, FYTSM=1] should be symmetric to [FSS=1, FGSS=1,
> FYTSM=undef].
> 
> > If we do, then we probably just want to depend on the *presence* of it,
> 
> Absolutely.

Stupid question: Why do we actually still have a prefs::kForceSafeSearch? From a
quick look at the code, it seems like we can just get rid of the legacy pref,
map the legacy policy to prefs in the correct way in the policy handler, and
have all consuming code just look at either prefs::kForceGoogleSafeSearch or
prefs::kForceYouTubeSafetyMode.

[Thiemo Nagel, 2015-04-15 08:03:56]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
> Stupid question: Why do we actually still have a prefs::kForceSafeSearch? From
a
> quick look at the code, it seems like we can just get rid of the legacy pref,
> map the legacy policy to prefs in the correct way in the policy handler, and
> have all consuming code just look at either prefs::kForceGoogleSafeSearch or
> prefs::kForceYouTubeSafetyMode.

Sounds good!

[Marc Treib, 2015-04-15 12:12:13]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/15 08:03:56, Thiemo Nagel wrote:
> > Stupid question: Why do we actually still have a prefs::kForceSafeSearch?
From
> a
> > quick look at the code, it seems like we can just get rid of the legacy
pref,
> > map the legacy policy to prefs in the correct way in the policy handler, and
> > have all consuming code just look at either prefs::kForceGoogleSafeSearch or
> > prefs::kForceYouTubeSafetyMode.
> 
> Sounds good!

That would mean that we basically have to re-evaluate all three policies when
any of them change. (E.g. legacy SafeSearch is already set to 1. Now
GoogleSafeSearch is set; we'd have to remove the YouTube pref.)
Not sure if the policy system easily supports this - Thiemo?

[Mattias Nissler (ping if slow), 2015-04-15 12:14:13]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
On 2015/04/15 12:12:12, Marc Treib wrote:
> On 2015/04/15 08:03:56, Thiemo Nagel wrote:
> > > Stupid question: Why do we actually still have a prefs::kForceSafeSearch?
> From
> > a
> > > quick look at the code, it seems like we can just get rid of the legacy
> pref,
> > > map the legacy policy to prefs in the correct way in the policy handler,
and
> > > have all consuming code just look at either prefs::kForceGoogleSafeSearch
or
> > > prefs::kForceYouTubeSafetyMode.
> > 
> > Sounds good!
> 
> That would mean that we basically have to re-evaluate all three policies when
> any of them change. (E.g. legacy SafeSearch is already set to 1. Now
> GoogleSafeSearch is set; we'd have to remove the YouTube pref.)
> Not sure if the policy system easily supports this - Thiemo?

If I'm not mistaken, we do a full policy->pref mapping whenever policy changes,
so that shouldn't be a problem.

[Thiemo Nagel, 2015-04-15 12:32:22]

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1056003003/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:401: if (!force_safe_search &&
> That would mean that we basically have to re-evaluate all three policies when
> any of them change. (E.g. legacy SafeSearch is already set to 1. Now
> GoogleSafeSearch is set; we'd have to remove the YouTube pref.)
> Not sure if the policy system easily supports this - Thiemo?

Mattias knows the policy stack much better than I do.  :)

[Marc Treib, 2015-04-16 09:39:23]

Alright, let's try this again.
prefs::kForceSafeSearch removed, ForceSafeSearch policy updated not to do
anything if any of the Google or YouTube policies are defined. Also updated
policy_browsertests to test all combinations of the policies.

[Andrew T Wilson (Slow), 2015-04-16 11:54:38]

lgtm, with one comment and one wording change.

https://codereview.chromium.org/1056003003/diff/60001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1056003003/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1131: for (int i = 0; i < 3 * 3 * 3;
i++) {
This is all fine, so you don't have to change it. But if you ever want to do
something like this again, consider just having a lookup table of all the cases
you want to deal with. It's easier to understand than a bunch of math/bit
twiddling as we have below.

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

https://codereview.chromium.org/1056003003/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:896: 'desc': '''This policy is
deprecated, please use ForceGoogleSafeSearch and ForceYouTubeSafetyMode instead.
If any of ForceGoogleSafeSearch or ForceYouTubeSafetyMode is enabled, then the
value of this policy is ignored.
nit: change this to "The value of this policy will be ignored if either the
ForceGoogleSafeSearch or ForceYouTubeSafeSearch policies are set".

The current wording is ambiguous about what happens if I set a
ForceYouTubeSafetyMode=false policy.

[Marc Treib, 2015-04-16 12:00:40]

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

https://codereview.chromium.org/1056003003/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:896: 'desc': '''This policy is
deprecated, please use ForceGoogleSafeSearch and ForceYouTubeSafetyMode instead.
If any of ForceGoogleSafeSearch or ForceYouTubeSafetyMode is enabled, then the
value of this policy is ignored.
On 2015/04/16 11:54:37, Andrew T Wilson wrote:
> nit: change this to "The value of this policy will be ignored if either the
> ForceGoogleSafeSearch or ForceYouTubeSafeSearch policies are set".
> 
> The current wording is ambiguous about what happens if I set a
> ForceYouTubeSafetyMode=false policy.

Right, forgot to update this. Good catch, thanks!

[Marc Treib, 2015-04-16 12:01:10]

treib@chromium.org changed reviewers:
+ mlerman@chromium.org

[Marc Treib, 2015-04-16 12:01:11]

+mlerman for c/b/profiles. PTAL!

[Mike Lerman, 2015-04-16 12:50:39]

profiles LGTM!

[Mattias Nissler (ping if slow), 2015-04-16 13:10:48]

/me likes all the code deletion :)

One further comment to improve clarity.

https://codereview.chromium.org/1056003003/diff/80001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/1056003003/diff/80001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:504: class
ForceSafeSearchPolicyHandler : public TypeCheckingPolicyHandler {
What you have works. However, I think an easier-to-follow solution would be to
drop the simple mappings declared above and move the responsibility for mapping
prefs::kFocreGoogleSafeSearch and prefs::kForceYouToubeSafetyMode into a single
place, namely this handler. That way, the reader has all the logic deciding the
pref values in one place and doesn't need to be aware of the simple mapping
declared in the table above.

[Marc Treib, 2015-04-16 13:18:23]

https://codereview.chromium.org/1056003003/diff/80001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/1056003003/diff/80001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:504: class
ForceSafeSearchPolicyHandler : public TypeCheckingPolicyHandler {
On 2015/04/16 13:10:47, Mattias Nissler wrote:
> What you have works. However, I think an easier-to-follow solution would be to
> drop the simple mappings declared above and move the responsibility for
mapping
> prefs::kFocreGoogleSafeSearch and prefs::kForceYouToubeSafetyMode into a
single
> place, namely this handler. That way, the reader has all the logic deciding
the
> pref values in one place and doesn't need to be aware of the simple mapping
> declared in the table above.

I considered that, and I actually prefer the way it is now: If you're only
interested in the non-legacy policies, you see the simple mapping above and know
all you need to know. Only the weirdly-behaving legacy policy has special code.
And if we can ever actually remove the legacy policy, it's as simple as removing
this class.
I've added a comment that refers to kSimplePolicyMap though :)

[Mattias Nissler (ping if slow), 2015-04-16 13:27:56]

LGTM

https://codereview.chromium.org/1056003003/diff/80001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/1056003003/diff/80001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:504: class
ForceSafeSearchPolicyHandler : public TypeCheckingPolicyHandler {
On 2015/04/16 13:18:23, Marc Treib wrote:
> On 2015/04/16 13:10:47, Mattias Nissler wrote:
> > What you have works. However, I think an easier-to-follow solution would be
to
> > drop the simple mappings declared above and move the responsibility for
> mapping
> > prefs::kFocreGoogleSafeSearch and prefs::kForceYouToubeSafetyMode into a
> single
> > place, namely this handler. That way, the reader has all the logic deciding
> the
> > pref values in one place and doesn't need to be aware of the simple mapping
> > declared in the table above.
> 
> I considered that, and I actually prefer the way it is now: If you're only
> interested in the non-legacy policies, you see the simple mapping above and
know
> all you need to know. Only the weirdly-behaving legacy policy has special
code.
> And if we can ever actually remove the legacy policy, it's as simple as
removing
> this class.
> I've added a comment that refers to kSimplePolicyMap though :)

I suppose somebody trying to understand how these policies work would prefer to
have the full story told in a single place. Your comment achieves that same goal
though, so fine with me.

[battre, 2015-04-17 09:55:19]

LGTM to c/b/net

[Marc Treib, 2015-04-17 10:11:06]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-04-17 10:11:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, mlerman@chromium.org
Link to the patchset: https://codereview.chromium.org/1056003003/#ps100001
(title: "comment")

[commit-bot: I haz the power, 2015-04-17 10:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056003003/100001

[commit-bot: I haz the power, 2015-04-17 10:58:28]

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

[commit-bot: I haz the power, 2015-04-17 10:58:29]

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

[Marc Treib, 2015-04-17 11:29:55]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-04-17 11:29:56]

+bauerb for Android. PTAL!

[Bernhard Bauer, 2015-04-17 11:34:31]

Android LGTM

[Marc Treib, 2015-04-17 11:34:55]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-04-17 11:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, atwilson@chromium.org, mnissler@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1056003003/#ps120001
(title: "fix Android")

[commit-bot: I haz the power, 2015-04-17 11:35:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056003003/120001

[commit-bot: I haz the power, 2015-04-17 12:33:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-17 12:33:56]

Patchset 7 (id:??) landed as
https://crrev.com/2b18805daa18a3bc4d9892f50e44d5cfacdad451
Cr-Commit-Position: refs/heads/master@{#325620}