Added a ForceYouTubeRestrict policy and deprecated the old ForceYouTubeSafetyMode policy

Added a ForceYouTubeRestrict policy and deprecated the old ForceYouTubeSafetyMode policy

ForceYouTubeRestrict allows 3 settings: off/moderate/strict.
ForceYouTubeSafetyMode only allowed off/on.
If ForceYouTubeRestrict is set, ForceYouTubeSafetyMode is ignored.
If ForceYouTubeRestrict is not set and ForceYouTubeSafetyMode is set, 'on' is mapped to 'moderate'.
If the deprecated ForceSafeSearch policy is set and neither ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, 'on' is mapped to 'moderate' as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no longer sent,
even if ForceYouTubeRestrict is not set. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
Committed: https://crrev.com/c42c0dddcb3c630cb30096d16a4485bd1025eccc
Cr-Commit-Position: refs/heads/master@{#423571}

[ljusten (tachyonic), 2016-08-11 16:07:46]

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

[commit-bot: I haz the power, 2016-08-11 16:08:15]

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

[ljusten (tachyonic), 2016-08-11 16:09:12]

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

[ljusten (tachyonic), 2016-08-11 16:09:12]

Hey Thiemo,

Alright, the YouTube change is up. Sorry I have to push all CLs to you. Check
them out whenever you get to it, no rush! Thanks for your support, I really
appreciate it!

- Lutz

[commit-bot: I haz the power, 2016-08-11 17:35:18]

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

[commit-bot: I haz the power, 2016-08-11 17:35:19]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-08-12 12:12:21]

Some initial comments.

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

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:151:
force_youtube_restrict_(NULL),
Could you maybe replace NULL by nullptr while you're at it?

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:318: request, headers,
(safe_search_util::YouTubeRestrictMode)
Please use static_cast instead of c-style cast.  Also it would seem prudent to
add DCHECK()s to protect against out of range values.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.cc:94: // sets the Safety Mode header.
Setting |YTRM_OFF| removes a Restrict header
I thought you're removing the safety mode header?  That comment seems out of
date.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.h:20: YTRM_OFF = 0,      // Do not enforce a
restriction on YouTube content. YouTube
Nit: Please don't use abbreviations.  I'd suggest to go for YOUTUBE_OFF, etc.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.h:32: // If |request| is a request to
YouTube, enforces a minimum YouTube's Restrict
Nit: The 's is weird here.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.h:33: // mode by setting YouTube's Restrict
header. Forcing to YTRM_OFF will remove
Nit: Present tense is better style, imho.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:118: //{
key::kForceYouTubeSafetyMode,   // Deprecated, the ForceYouTubeSafetyMode
Nit: We don't keep commented-out code.  Just drop it.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:611: if
(policies.GetValue(key::kForceGoogleSafeSearch) ||
In principle, we have the concept of a LegacyPoliciesDeprecatingPolicyHandler. 
Could you maybe use that for your purpose?

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:626:
(int)(enabled ? safe_search_util::YTRM_MODERATE
I don't think the (int) cast is necessary.  In general we avoid casts like the
plague.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:635: class
ForceYouTubeSafetyModePolicyHandler : public TypeCheckingPolicyHandler {
configuration_policy_handler_list_factory.cc doesn't seem to be the proper place
for defining handlers.  I'd suggest to move it somewhere else.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:655:
(int)(enabled ? safe_search_util::YTRM_MODERATE
The (int) cast doesn't seem necessary.

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:940: // DEPRECATED: This on/off setting has been
replaced by the more fine-grained
Same here: Just drop the constant.

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.h#...
chrome/common/pref_names.h:311: // extern const char kForceYouTubeSafetyMode[];
// Deprecated, see cc file
Please don't comment-out code.  Just drop that line.

https://codereview.chromium.org/2239753002/diff/1/components/policy/resources...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2239753002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:1056: 'desc': '''This policy
is deprecated, please use ForceGoogleSafeSearch and ForceYouTubeRestrict
instead. This policy will be ignored if either the ForceGoogleSafeSearch, the
ForceYouTubeRestrict or the (deprecated) ForceYouTubeSafetyMode policies are
set.
Please wrap policy names in <ph name...>...</ph> to signal translators that
these must not be translated.  Example: 

<ph name="FORCEYOUTUBERESTRICT_POLICY_NAME">ForceYouTubeRestrict</ph>

Nit: Prefer present tense over future tense.

https://codereview.chromium.org/2239753002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:1144: If you set this setting
to Strict, Strict Restricted Mode on YouTube is always active.
Style nit: Imho passive voice should be preferred in technical documentation. 
(As opposed to: "You should prefer passive voice in technical documentation.")

[ljusten (tachyonic), 2016-08-16 09:24:54]

I've incorporated the comments. There are two remaining things for discussion:
- Where to put the two policy handlers in
configuration_policy_handler_list_factory.cc
- Whether to change LegacyPoliciesDeprecatingPolicyHandler so that it can handle
multiple new policy handlers (new bug/cl?).

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

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:151:
force_youtube_restrict_(NULL),
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Could you maybe replace NULL by nullptr while you're at it?

Done.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:318: request, headers,
(safe_search_util::YouTubeRestrictMode)
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Please use static_cast instead of c-style cast.  Also it would seem prudent to
> add DCHECK()s to protect against out of range values.

Done.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.cc:94: // sets the Safety Mode header.
Setting |YTRM_OFF| removes a Restrict header
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> I thought you're removing the safety mode header?  That comment seems out of
> date.

Plans change, old comments stay... Reformulated the whole comment.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.h:20: YTRM_OFF = 0,      // Do not enforce a
restriction on YouTube content. YouTube
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Nit: Please don't use abbreviations.  I'd suggest to go for YOUTUBE_OFF, etc.

Ohhhkayyy :-)

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.h:32: // If |request| is a request to
YouTube, enforces a minimum YouTube's Restrict
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Nit: The 's is weird here.

Acknowledged.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/net/safe_sea...
chrome/browser/net/safe_search_util.h:33: // mode by setting YouTube's Restrict
header. Forcing to YTRM_OFF will remove
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Nit: Present tense is better style, imho.

I've reformulated that a bit.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:118: //{
key::kForceYouTubeSafetyMode,   // Deprecated, the ForceYouTubeSafetyMode
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Nit: We don't keep commented-out code.  Just drop it.

Done.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:611: if
(policies.GetValue(key::kForceGoogleSafeSearch) ||
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> In principle, we have the concept of a LegacyPoliciesDeprecatingPolicyHandler.

> Could you maybe use that for your purpose?

Yes, this could be done. However, I decided against it for now as it would make
the code slightly longer and harder to understand. The related policy handler
above (ForceSafeSearchPolicyHandler) cannot be converted since
LegacyPoliciesDeprecatingPolicyHandler supports "many legacy policies -  one new
policy", but ForceSafeSearchPolicyHandler needs "one legacy policy - many new
policies". Thus, moving one to LegacyPoliciesDeprecatingPolicyHandler and the
other not would break symmetry and make the code harder to understand.

LegacyPoliciesDeprecatingPolicyHandler could be changed to support many new
polcies. If we decided to do that, we should probably make a separate
changelist.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:626:
(int)(enabled ? safe_search_util::YTRM_MODERATE
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> I don't think the (int) cast is necessary.  In general we avoid casts like the
> plague.

Done.

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:635: class
ForceYouTubeSafetyModePolicyHandler : public TypeCheckingPolicyHandler {
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> configuration_policy_handler_list_factory.cc doesn't seem to be the proper
place
> for defining handlers.  I'd suggest to move it somewhere else.

I just copy&pasted code! Nothing wrong with that!

Suggestions? Maybe create a new
src/chrome/browser/safe_browsing/safe_browsing_policy_handler.h/cc and move
ForceSafeSearchPolicyHandler and ForceYouTubeSafetyModePolicyHandler?

Or maybe in chrome/browser/net where safe_search_util is?

https://codereview.chromium.org/2239753002/diff/1/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:655:
(int)(enabled ? safe_search_util::YTRM_MODERATE
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> The (int) cast doesn't seem necessary.

True, I've removed it. I added it because I don't like implicit conversions,
even from enum to int.

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:940: // DEPRECATED: This on/off setting has been
replaced by the more fine-grained
On 2016/08/12 12:12:20, Thiemo Nagel wrote:
> Same here: Just drop the constant.

Done.

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2239753002/diff/1/chrome/common/pref_names.h#...
chrome/common/pref_names.h:311: // extern const char kForceYouTubeSafetyMode[];
// Deprecated, see cc file
On 2016/08/12 12:12:21, Thiemo Nagel wrote:
> Please don't comment-out code.  Just drop that line.

Done.

https://codereview.chromium.org/2239753002/diff/1/components/policy/resources...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2239753002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:1056: 'desc': '''This policy
is deprecated, please use ForceGoogleSafeSearch and ForceYouTubeRestrict
instead. This policy will be ignored if either the ForceGoogleSafeSearch, the
ForceYouTubeRestrict or the (deprecated) ForceYouTubeSafetyMode policies are
set.
On 2016/08/12 12:12:21, Thiemo Nagel wrote:
> Please wrap policy names in <ph name...>...</ph> to signal translators that
> these must not be translated.  Example: 
> 
> <ph name="FORCEYOUTUBERESTRICT_POLICY_NAME">ForceYouTubeRestrict</ph>
> 
> Nit: Prefer present tense over future tense.

Acknowledged.

https://codereview.chromium.org/2239753002/diff/1/components/policy/resources...
components/policy/resources/policy_templates.json:1144: If you set this setting
to Strict, Strict Restricted Mode on YouTube is always active.
On 2016/08/12 12:12:21, Thiemo Nagel wrote:
> Style nit: Imho passive voice should be preferred in technical documentation. 
> (As opposed to: "You should prefer passive voice in technical documentation.")

Done.

[ljusten (tachyonic), 2016-09-05 09:04:16]

ljusten@chromium.org changed reviewers:
+ erg@chromium.org, mef@chromium.org, mkwst@chromium.org, treib@chromium.org

[ljusten (tachyonic), 2016-09-05 09:04:17]

mkwst@chromium.org: Please review changes in 
tools/metrics/histograms/histograms.xml

mef@chromium.org: Please review changes in 
chrome/browser/net

erg@chromium.org: Please review changes in 
chrome/browser/profiles

treib@chromium.org: Please review changes in 
chrome/browser/supervised_user and other parts that touch your youtube safety
mode code.

[Mike West, 2016-09-05 09:09:13]

I'm only in the //tools/metrics/histograms/ file for Blink-side UseCounter
changes. This change looks pretty trivial, I know, but you'll need a "real"
OWNER to stamp it. Sorry!

[ljusten (tachyonic), 2016-09-05 09:12:52]

Description was changed from

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

The new policy allows 3 settings: off/moderate/strict. The old one only allowed
off/on,
where 'on' corresponds to 'moderate'.
If the old policy is set, but the new one is not, the value is interpreted as
'moderate'.
If the new policy is set, the old one is ignored.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, the value is interpreted as moderate as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set yet. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

to

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

The new policy allows 3 settings: off/moderate/strict. The old one only allowed
off/on,
where 'on' corresponds to 'moderate'.
If the old policy is set, but the new one is not, the value is interpreted as
'moderate'.
If the new policy is set, the old one is ignored.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, the value is interpreted as moderate as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set yet. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

[ljusten (tachyonic), 2016-09-05 09:12:52]

ljusten@chromium.org changed reviewers:
- mkwst@chromium.org

[ljusten (tachyonic), 2016-09-05 09:27:53]

ljusten@chromium.org changed reviewers:
+ holte@chromium.org

[ljusten (tachyonic), 2016-09-05 09:27:54]

holte@chromium.org, please review changes in histograms.xml.

[Marc Treib, 2016-09-05 09:58:11]

Mostly LG, with a bunch of small comments below.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:123: default:
Leave out the "default" case. That way, if anyone ever adds a value to the enum,
they'll get a compile error if they don't handle it here.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.h:19: enum YouTubeRestrictMode {
optional: enum class, with values OFF, MODERATE, STRICT?
(In particular, "YouTubeRestrictMode.OFF" is IMO much clearer than
"YOUTUBE_OFF".)

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

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:634: if
(value->GetAsBoolean(&enabled))
Braces please if the body doesn't fit on one line.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:663: if
(value && value->GetAsBoolean(&enabled))
Also here, braces please.

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

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1202: youtube == 0 ? nullptr        
   // subtracting 1 gives 0,1,2, see above
misaligned

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1222: if (youtube == 0 &&
legacy_enabled)
nit: braces

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_pref_store.cc:147: bool
force_safe_search = true;
Maybe add a comment that prefs::kForceGoogleSafeSearch is set automatically?

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_pref_store_unittest.cc
(right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_pref_store_unittest.cc:117: //
kForceGoogleSafeSearch defaults to true and kForceYouTubeRestrict
So, Restrict=Moderate results in the same behavior as the old SafetyMode=true?

https://codereview.chromium.org/2239753002/diff/60001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/common/pref_name...
chrome/common/pref_names.cc:946: // YouTube.
Mention the enum that lists the possible values?

https://codereview.chromium.org/2239753002/diff/60001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:320: "os": ["win", "linux",
"mac", "chromeos"]
Isn't Android supported too? (Also applies to the three existing policies)

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

https://codereview.chromium.org/2239753002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:1189: If this setting is set
to Off or no value is set, Restricted Mode on YouTube is not enforced. The user
may pick any value''',
nit: Add a final period.

[ljusten (tachyonic), 2016-09-05 15:14:51]

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

[commit-bot: I haz the power, 2016-09-05 15:15:00]

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

[ljusten (tachyonic), 2016-09-05 15:24:32]

treib@: PTAL

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:123: default:
On 2016/09/05 09:58:10, Marc Treib wrote:
> Leave out the "default" case. That way, if anyone ever adds a value to the
enum,
> they'll get a compile error if they don't handle it here.

Done.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.h:19: enum YouTubeRestrictMode {
On 2016/09/05 09:58:10, Marc Treib wrote:
> optional: enum class, with values OFF, MODERATE, STRICT?
> (In particular, "YouTubeRestrictMode.OFF" is IMO much clearer than
> "YOUTUBE_OFF".)

I like that. It also adds type-safety as it requires explicit casts to int in a
couple of places, e.g. when passing the enum into the constructor of
FundamentalValue.

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

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:634: if
(value->GetAsBoolean(&enabled))
On 2016/09/05 09:58:10, Marc Treib wrote:
> Braces please if the body doesn't fit on one line.

Done.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:663: if
(value && value->GetAsBoolean(&enabled))
On 2016/09/05 09:58:10, Marc Treib wrote:
> Also here, braces please.

Done.

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

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1202: youtube == 0 ? nullptr        
   // subtracting 1 gives 0,1,2, see above
On 2016/09/05 09:58:10, Marc Treib wrote:
> misaligned

Done.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1222: if (youtube == 0 &&
legacy_enabled)
On 2016/09/05 09:58:10, Marc Treib wrote:
> nit: braces

Done.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_pref_store.cc:147: bool
force_safe_search = true;
On 2016/09/05 09:58:10, Marc Treib wrote:
> Maybe add a comment that prefs::kForceGoogleSafeSearch is set automatically?

Done.

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_pref_store_unittest.cc
(right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_pref_store_unittest.cc:117: //
kForceGoogleSafeSearch defaults to true and kForceYouTubeRestrict
On 2016/09/05 09:58:11, Marc Treib wrote:
> So, Restrict=Moderate results in the same behavior as the old SafetyMode=true?

Correct.

https://codereview.chromium.org/2239753002/diff/60001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/common/pref_name...
chrome/common/pref_names.cc:946: // YouTube.
On 2016/09/05 09:58:11, Marc Treib wrote:
> Mention the enum that lists the possible values?

Done.

https://codereview.chromium.org/2239753002/diff/60001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/2239753002/diff/60001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:320: "os": ["win", "linux",
"mac", "chromeos"]
On 2016/09/05 09:58:11, Marc Treib wrote:
> Isn't Android supported too? (Also applies to the three existing policies)

Done.

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

https://codereview.chromium.org/2239753002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:1189: If this setting is set
to Off or no value is set, Restricted Mode on YouTube is not enforced. The user
may pick any value''',
On 2016/09/05 09:58:11, Marc Treib wrote:
> nit: Add a final period.

Done.

[commit-bot: I haz the power, 2016-09-05 15:47:38]

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

[commit-bot: I haz the power, 2016-09-05 15:47:39]

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

[Marc Treib, 2016-09-05 16:01:55]

lgtm

Thanks! My parts LGTM, one comment for policy owners.

https://codereview.chromium.org/2239753002/diff/80001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/2239753002/diff/80001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:5: "os": ["List of operating
systems that support this policy. Valid values:", "win", "linux", "mac",
"chromeos", "android", "Defaults to empty if not specified."],
Ah, do the tests not support Android maybe? One of the policy folks will have to
judge whether it's okay to just add that here.

[ljusten (tachyonic), 2016-09-06 14:45:23]

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

[commit-bot: I haz the power, 2016-09-06 14:45:42]

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-09-06 15:25:49]

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

[commit-bot: I haz the power, 2016-09-06 15:25:50]

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

[Elliot Glaysher, 2016-09-06 18:24:06]

simple renaming in profiles lgtm

[Steven Holte, 2016-09-06 20:26:45]

histograms lgtm

[mef, 2016-09-06 21:26:12]

c/b/n/* lgtm

[ljusten (tachyonic), 2016-09-07 07:49:28]

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

[commit-bot: I haz the power, 2016-09-07 07:49:41]

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-09-07 09:11:38]

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

[commit-bot: I haz the power, 2016-09-07 09:11:39]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-10-05 17:38:15]

I think the CL description would be clearer if most occurrences of "old" and
"new" would be replaced by the actual policy names.  Since 4 different policies
are involved, the references can be confusing.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:317:
force_youtube_restrict_->GetValue() !=
In my understanding, the static cast can blow up (i.e. lead to undefined
behavior) in case a corrupted, out-of-range pref is read from disk.  It seems to
me that a range check is required.  (Sorry for overlooking this in the first
review round.)

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.cc:92: // Does nothing if |request| is not a
request to YouTube. Otherwise, if |mode|
Please don't duplicate comments (otherwise they're certain to get out of sync
sooner or later).  The .h file is the right place for the method description,
thus please remove it here.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.cc:123: // No default! That way, adding a
new enum value results in a compile
This is pretty much a standard practice.  No need to add a comment about it.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.h:20: kOff = 0,      // Do not restrict
YouTube content. YouTube might still
Sadly the Chromium style guide requires SHOUTY_CASE, overruling the Google style
guide in that regard.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:622: // Note:
Those policies are declared in kSimplePolicyMap above. except
Nit: Since you're continuing with small caps, the period was probably intended
as a comma?

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:623: //      
ForceYouTubeSafetyMode, which has been replaced by
Nit: I'd suggest to not indent inside the comment, that'll safe you one line of
vertical space.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:625: if
(policies.GetValue(key::kForceGoogleSafeSearch) ||
As I read the code, the combination of ForceSafeSearch=true,
ForceGoogleSafeSearch=true policies leads to prefs
ForceYouTubeSafetyMode=undefined and ForceYouTubeRestrict=undefined.  That
doesn't seem right.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:635: // Note
that ForceYouTubeRestrict is an int policy,
Nit: Please wrap comments at 80 characters.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1182: // (undefined, off, moderate,
strict) for the three policies.
Probably all 4 policies should be included in the test.

https://codereview.chromium.org/2239753002/diff/140001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/common/pref_nam...
chrome/common/pref_names.cc:924: // Integer controlling whether Restrict Mode
(moderate/strict) is mandatory on
Nit: I'd suggest "mandatory on" --> "applied to".

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1142: If you enable this
setting, Restricted Mode on YouTube is always enforced to be at least Moderate.
Style nit: I'd suggest to use passive voice in technical documentation (not
addressing the reader directly with "you").

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1144: If you disable this
setting or do not set a value, Restricted Mode on YouTube is not enforced.''',
I guess it would make sense to add "on the client side" or similar, since afaiu,
restricted mode can also be enforced by the server based on the user's account,
independent of the presence or lack of a header sent by Chrome.

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1189: If this setting is set
to Off or no value is set, Restricted Mode on YouTube is not enforced. The user
may pick any value.''',
I think this should come with a caveat that other policies may cause restricted
mode to be set.

Ideally, at one place in this file there should be a complete decision tree that
explains which combinations will cause which effects.  Then the individual
policies can refer to that place for full explanation.

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1190: 'arc_support': 'This
policy has no effect on the Android YouTube app. If you want to enforce Safety
Mode on YouTube, you should disallow installation of the Android YouTube app.',
Nit: Same as above, I'd prefer passive voice in techdoc.

[ljusten (tachyonic), 2016-10-05 18:53:19]

Description was changed from

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

The new policy allows 3 settings: off/moderate/strict. The old one only allowed
off/on,
where 'on' corresponds to 'moderate'.
If the old policy is set, but the new one is not, the value is interpreted as
'moderate'.
If the new policy is set, the old one is ignored.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, the value is interpreted as moderate as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set yet. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

to

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

ForceYouTubeRestrict allows 3 settings: off/moderate/strict.
ForceYouTubeSafetyMode only allowed off/on.
If ForceYouTubeRestrict is set, ForceYouTubeSafetyMode is ignored.
If ForceYouTubeRestrict is not set and ForceYouTubeSafetyMode is set, 'on' is
mapped to 'moderate'.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, 'on' is mapped to 'moderate' as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

[ljusten (tachyonic), 2016-10-06 10:14:46]

tnagel: PTAL

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:317:
force_youtube_restrict_->GetValue() !=
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> In my understanding, the static cast can blow up (i.e. lead to undefined
> behavior) in case a corrupted, out-of-range pref is read from disk.  It seems
to
> me that a range check is required.  (Sorry for overlooking this in the first
> review round.)

I believe you're right. If the compiler decides to store the enum in an unsigned
char, a cast of 257 might be aliased to 1 and cause undefined behavior. I've
added a range check.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.cc:92: // Does nothing if |request| is not a
request to YouTube. Otherwise, if |mode|
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Please don't duplicate comments (otherwise they're certain to get out of sync
> sooner or later).  The .h file is the right place for the method description,
> thus please remove it here.

Done.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.cc:123: // No default! That way, adding a
new enum value results in a compile
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> This is pretty much a standard practice.  No need to add a comment about it.

Done.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.h:20: kOff = 0,      // Do not restrict
YouTube content. YouTube might still
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Sadly the Chromium style guide requires SHOUTY_CASE, overruling the Google
style
> guide in that regard.
> 
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Sigh... I cannot simply rename kStrict to STRICT because some Windows code
#defines STRICT. I decided to go back to a plain enum and name the values
YOUTUBE_RESTRICT_*. That's clear, compiles, complies with naming conventions and
saves some typecasts to int.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:622: // Note:
Those policies are declared in kSimplePolicyMap above. except
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Nit: Since you're continuing with small caps, the period was probably intended
> as a comma?

Done.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:623: //      
ForceYouTubeSafetyMode, which has been replaced by
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Nit: I'd suggest to not indent inside the comment, that'll safe you one line
of
> vertical space.

Done.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:625: if
(policies.GetValue(key::kForceGoogleSafeSearch) ||
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> As I read the code, the combination of ForceSafeSearch=true,
> ForceGoogleSafeSearch=true policies leads to prefs
> ForceYouTubeSafetyMode=undefined and ForceYouTubeRestrict=undefined.  That
> doesn't seem right.

This is correct, see crbug.com/476908. As soon as ForceGoogleSafeSearch is set,
it is understood that ForceSafeSearch is ignored. If we did set
ForceYouTubeRestrict=Moderate in this case (there is no ForceYouTubeSafetyMode
pref anymore), then on new clients Moderate would be enforced, whereas on old
clients we would not enforce restricted resp. safety mode.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:635: // Note
that ForceYouTubeRestrict is an int policy,
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Nit: Please wrap comments at 80 characters.

Done.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1182: // (undefined, off, moderate,
strict) for the three policies.
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Probably all 4 policies should be included in the test.

Done.

https://codereview.chromium.org/2239753002/diff/140001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/common/pref_nam...
chrome/common/pref_names.cc:924: // Integer controlling whether Restrict Mode
(moderate/strict) is mandatory on
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Nit: I'd suggest "mandatory on" --> "applied to".

I'd like to keep it this way. 'Mandatory' is stronger and hints that strict
settings overrule potential less strict settings elsewhere, e.g. in YouTube
settings. It's also consistent with the lingo for safe search.

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1142: If you enable this
setting, Restricted Mode on YouTube is always enforced to be at least Moderate.
On 2016/10/05 17:38:15, Thiemo Nagel (slow) wrote:
> Style nit: I'd suggest to use passive voice in technical documentation (not
> addressing the reader directly with "you").

Done.

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1144: If you disable this
setting or do not set a value, Restricted Mode on YouTube is not enforced.''',
On 2016/10/05 17:38:15, Thiemo Nagel (slow) wrote:
> I guess it would make sense to add "on the client side" or similar, since
afaiu,
> restricted mode can also be enforced by the server based on the user's
account,
> independent of the presence or lack of a header sent by Chrome.

Done.

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1189: If this setting is set
to Off or no value is set, Restricted Mode on YouTube is not enforced. The user
may pick any value.''',
On 2016/10/05 17:38:15, Thiemo Nagel (slow) wrote:
> I think this should come with a caveat that other policies may cause
restricted
> mode to be set.

I've clarified this a bit by mentioning YouTube policies. 

> Ideally, at one place in this file there should be a complete decision tree
that
> explains which combinations will cause which effects.  Then the individual
> policies can refer to that place for full explanation.

Since SafetyMode is deprecated, I don't think we need to explain the full
decision tree. We're meeting the usual expectation "if the new policy is not
set, the deprecated one keeps working like before" and "the new policy overrides
the deprecated one".

https://codereview.chromium.org/2239753002/diff/140001/components/policy/reso...
components/policy/resources/policy_templates.json:1190: 'arc_support': 'This
policy has no effect on the Android YouTube app. If you want to enforce Safety
Mode on YouTube, you should disallow installation of the Android YouTube app.',
On 2016/10/05 17:38:14, Thiemo Nagel (slow) wrote:
> Nit: Same as above, I'd prefer passive voice in techdoc.

Done.

[ljusten (tachyonic), 2016-10-06 10:41:35]

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

[commit-bot: I haz the power, 2016-10-06 10:41:51]

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-10-06 13:05:12]

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

[commit-bot: I haz the power, 2016-10-06 13:05:13]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-10-06 14:05:58]

A few more comments, mostly small ones.

Please try to avoid mixing your changes together with a rebase as that makes it
harder to look at your changes.  For an example that shows unrelated changes:

https://codereview.chromium.org/2239753002/diff2/140001:160001/chrome/browser...

The easiest way to accomplish that is to only rebase while there are no changes
pending locally and to run "git cl upload" after each local rebase.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:317:
force_youtube_restrict_->GetValue() !=
Afaiu, it's UB whenever the int is outside the enum range.  Most of the time the
compiler will probably play nice and nothing bad will happen, but any change in
compiler version could introduce something unexpected.

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2239753002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:625: if
(policies.GetValue(key::kForceGoogleSafeSearch) ||
Alright.  Thanks for updating the CL description to reflect that.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1184: // Go over all combinations of
(undefined,true,false) resp.
Nit: Could you please insert spaces after the commas?

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1197: "Adjust 'youtube' to match
YouTubeRestrictMode enum");
Nit: I think it would be more readable to move the static_assert outside the
loop.

Nit: I was scratching my head quite a bit about the purpose of that
static_assert (especially since I hadn't read further below).  How about
replacing the assert message by something along the lines of "this test depends
on enum entries having specific values"?

Nit: 'youtube' has been renamed to 'youtube_restrict'.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1213: // The legacy ForceSafeSearch
policy should only have an effect if
Nit: Please wrap comments at 80 characters.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1244: EXPECT_EQ(youtube_restrict !=
0,
That doesn't seem to make sense when youtube_restrict == 0.  Maybe it would be
better to write this as

if (youtube_restrict != 0) {
  EXPECT_EQ(youtube_restrict - 1, prefs
>GetInteger(prefs::kForceYouTubeRestrict));
}

or probably rather add an else branch to the conditional below?

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1253:
EXPECT_EQ(prefs->GetInteger(prefs::kForceYouTubeRestrict),expected_mode);
Nit: Missing blank after comma.

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:321: if (value >
safe_search_util::YOUTUBE_RESTRICT_OFF &&
This doesn't call ForceYouTubeRestrict() and thus doesn't strip headers when
restrict is off.

Also it depends on OFF being the first enum entry.  I'd thus suggest to add a
static_assert(OFF==0).

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate_unittest.cc:439:
static_assert(safe_search_util::YOUTUBE_RESTRICT_OFF      == 0 &&
Nit: I think it would be more readable to move the static_assert outside the
loop.

Also see comment about static_assert in policy_browsertest.

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/saf...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.h:28: const int kYouTubeRestrictModeCount =
3;  // Enum counter
Please make this an enum entry YOUTUBE_RESTRICT_COUNT = 3.  For better or worse,
that's the standard practice in Chrome.

[ljusten (tachyonic), 2016-10-06 15:16:49]

tnagel: PTAL

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1184: // Go over all combinations of
(undefined,true,false) resp.
On 2016/10/06 14:05:57, Thiemo Nagel (slow) wrote:
> Nit: Could you please insert spaces after the commas?

Done.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1197: "Adjust 'youtube' to match
YouTubeRestrictMode enum");
On 2016/10/06 14:05:57, Thiemo Nagel (slow) wrote:
> Nit: I think it would be more readable to move the static_assert outside the
> loop.
> 
> Nit: I was scratching my head quite a bit about the purpose of that
> static_assert (especially since I hadn't read further below).  How about
> replacing the assert message by something along the lines of "this test
depends
> on enum entries having specific values"?
> 
> Nit: 'youtube' has been renamed to 'youtube_restrict'.

I've refined this a little.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1213: // The legacy ForceSafeSearch
policy should only have an effect if
On 2016/10/06 14:05:57, Thiemo Nagel (slow) wrote:
> Nit: Please wrap comments at 80 characters.

Done.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1244: EXPECT_EQ(youtube_restrict !=
0,
On 2016/10/06 14:05:57, Thiemo Nagel (slow) wrote:
> That doesn't seem to make sense when youtube_restrict == 0.  Maybe it would be
> better to write this as
> 
> if (youtube_restrict != 0) {
>   EXPECT_EQ(youtube_restrict - 1, prefs
> >GetInteger(prefs::kForceYouTubeRestrict));
> }
> 
> or probably rather add an else branch to the conditional below?

Done.

https://codereview.chromium.org/2239753002/diff/160001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1253:
EXPECT_EQ(prefs->GetInteger(prefs::kForceYouTubeRestrict),expected_mode);
On 2016/10/06 14:05:57, Thiemo Nagel (slow) wrote:
> Nit: Missing blank after comma.

Done.

[ljusten (tachyonic), 2016-10-06 16:16:59]

tnagel:PTAL

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:321: if (value >
safe_search_util::YOUTUBE_RESTRICT_OFF &&
On 2016/10/06 14:05:58, Thiemo Nagel (slow) wrote:
> This doesn't call ForceYouTubeRestrict() and thus doesn't strip headers when
> restrict is off.
> 
> Also it depends on OFF being the first enum entry.  I'd thus suggest to add a
> static_assert(OFF==0).

Changed as discussed.

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/saf...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/2239753002/diff/180001/chrome/browser/net/saf...
chrome/browser/net/safe_search_util.h:28: const int kYouTubeRestrictModeCount =
3;  // Enum counter
On 2016/10/06 14:05:58, Thiemo Nagel (slow) wrote:
> Please make this an enum entry YOUTUBE_RESTRICT_COUNT = 3.  For better or
worse,
> that's the standard practice in Chrome.

Done.

[Thiemo Nagel, 2016-10-06 16:26:41]

LGTM.  (I haven't looked at supervised users as that was covered by Marc.)

I'll tick the commit box for you since the branch point is close.

[Thiemo Nagel, 2016-10-06 16:26:51]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-10-06 16:26:52]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
mef@chromium.org, erg@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2239753002/#ps220001
(title: "Comments")

[commit-bot: I haz the power, 2016-10-06 16:27:26]

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-10-06 17:31:25]

Description was changed from

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

ForceYouTubeRestrict allows 3 settings: off/moderate/strict.
ForceYouTubeSafetyMode only allowed off/on.
If ForceYouTubeRestrict is set, ForceYouTubeSafetyMode is ignored.
If ForceYouTubeRestrict is not set and ForceYouTubeSafetyMode is set, 'on' is
mapped to 'moderate'.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, 'on' is mapped to 'moderate' as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

to

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

ForceYouTubeRestrict allows 3 settings: off/moderate/strict.
ForceYouTubeSafetyMode only allowed off/on.
If ForceYouTubeRestrict is set, ForceYouTubeSafetyMode is ignored.
If ForceYouTubeRestrict is not set and ForceYouTubeSafetyMode is set, 'on' is
mapped to 'moderate'.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, 'on' is mapped to 'moderate' as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

[commit-bot: I haz the power, 2016-10-06 17:31:27]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-10-06 17:35:22]

Description was changed from

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

ForceYouTubeRestrict allows 3 settings: off/moderate/strict.
ForceYouTubeSafetyMode only allowed off/on.
If ForceYouTubeRestrict is set, ForceYouTubeSafetyMode is ignored.
If ForceYouTubeRestrict is not set and ForceYouTubeSafetyMode is set, 'on' is
mapped to 'moderate'.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, 'on' is mapped to 'moderate' as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478
==========

to

==========
Added a ForceYouTubeRestrict policy and deprecated the old
ForceYouTubeSafetyMode policy

ForceYouTubeRestrict allows 3 settings: off/moderate/strict.
ForceYouTubeSafetyMode only allowed off/on.
If ForceYouTubeRestrict is set, ForceYouTubeSafetyMode is ignored.
If ForceYouTubeRestrict is not set and ForceYouTubeSafetyMode is set, 'on' is
mapped to 'moderate'.
If the deprecated ForceSafeSearch policy is set and neither
ForceYouTubeSafetyMode nor
ForceYouTubeRestrict are set, 'on' is mapped to 'moderate' as well.
The restrict mode is sent to YouTube by adding a "YouTube-Restrict" HTML request
header
with values "Moderate" resp. "Strict". The old "YouTube-Safety-Mode" is no
longer sent,
even if ForceYouTubeRestrict is not set. YouTube already properly interprets
"YouTube-Restrict".

BUG=590478

Committed: https://crrev.com/c42c0dddcb3c630cb30096d16a4485bd1025eccc
Cr-Commit-Position: refs/heads/master@{#423571}
==========

[commit-bot: I haz the power, 2016-10-06 17:35:24]

Patchset 12 (id:??) landed as
https://crrev.com/c42c0dddcb3c630cb30096d16a4485bd1025eccc
Cr-Commit-Position: refs/heads/master@{#423571}

[ljusten (tachyonic), 2016-10-07 12:10:44]

Patchset #14 (id:260001) has been deleted

[ljusten (tachyonic), 2016-10-07 12:10:56]

Patchset #13 (id:240001) has been deleted

[ljusten (tachyonic), 2016-10-11 10:22:22]

Patchset #13 (id:280001) has been deleted

[Thiemo Nagel, 2016-10-17 09:32:43]

This has been reverted in https://codereview.chromium.org/2401733002 and
re-landed in https://codereview.chromium.org/2401743003/.