Added a ForceYouTubeRestrict policy and deprecated the old ForceYouTubeSafetyMode policy

chrome/browser/policy/configuration_policy_handler_list_factory.cc

Index: chrome/browser/policy/configuration_policy_handler_list_factory.cc
diff --git a/chrome/browser/policy/configuration_policy_handler_list_factory.cc b/chrome/browser/policy/configuration_policy_handler_list_factory.cc
index 92b9ebc819f3f9fc8ee9753bac4014346214e1a5..23b46497295bcc77aac850015330e6de7fdc23ee 100644
--- a/chrome/browser/policy/configuration_policy_handler_list_factory.cc
+++ b/chrome/browser/policy/configuration_policy_handler_list_factory.cc
@@ -16,6 +16,7 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/net/disk_cache_dir_policy_handler.h"
+#include "chrome/browser/net/safe_search_util.h"
 #include "chrome/browser/policy/file_selection_dialogs_policy_handler.h"
 #include "chrome/browser/policy/javascript_policy_handler.h"
 #include "chrome/browser/policy/managed_bookmarks_policy_handler.h"
@@ -114,9 +115,12 @@ const PolicyToPreferenceMapEntry kSimplePolicyMap[] = {
   { key::kForceGoogleSafeSearch,
     prefs::kForceGoogleSafeSearch,
     base::Value::TYPE_BOOLEAN },
-  { key::kForceYouTubeSafetyMode,
-    prefs::kForceYouTubeSafetyMode,
-    base::Value::TYPE_BOOLEAN },
+//{ key::kForceYouTubeSafetyMode,   // Deprecated, the ForceYouTubeSafetyMode

[Thiemo Nagel, 2016/08/12 12:12:20]

Nit: We don't keep commented-out code.  Just drop it.

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

Done.

+//  prefs::kForceYouTubeSafetyMode, // POLICY is converted to the ForceYouTube-
+//  base::Value::TYPE_BOOLEAN },    // Restrict SETTING in the code below.
+  { key::kForceYouTubeRestrict,
+    prefs::kForceYouTubeRestrict,
+    base::Value::TYPE_INTEGER},
   { key::kPasswordManagerEnabled,
     password_manager::prefs::kPasswordManagerSavingEnabled,
     base::Value::TYPE_BOOLEAN },
@@ -598,17 +602,29 @@ class ForceSafeSearchPolicyHandler : public TypeCheckingPolicyHandler {
   // ConfigurationPolicyHandler implementation:
   void ApplyPolicySettings(const PolicyMap& policies,
                            PrefValueMap* prefs) override {
-    // If either of the new GoogleSafeSearch or YouTubeSafetyMode policies is
-    // defined, then this one should be ignored. crbug.com/476908
-    // Note: Those policies are declared in kSimplePolicyMap above.
+    // If either of the new ForceGoogleSafeSearch, ForceYouTubeSafetyMode or
+    // ForceYouTubeRestrict policies are defined, then this one should be
+    // ignored. crbug.com/476908, crbug.com/590478
+    // Note: Those policies are declared in kSimplePolicyMap above. except
+    //       ForceYouTubeSafetyMode, which has been replaced by
+    //       ForceYouTubeRestrict.
     if (policies.GetValue(key::kForceGoogleSafeSearch) ||

[Thiemo Nagel, 2016/08/12 12:12:20]

In principle, we have the concept of a LegacyPoliciesDeprecatingPolicyHandler. 
Could you maybe use that for your purpose?

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

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.

-        policies.GetValue(key::kForceYouTubeSafetyMode)) {
+        policies.GetValue(key::kForceYouTubeSafetyMode) ||
+        policies.GetValue(key::kForceYouTubeRestrict)) {
       return;
     }
     const base::Value* value = policies.GetValue(policy_name());
     if (value) {
+      bool enabled;
       prefs->SetValue(prefs::kForceGoogleSafeSearch, value->CreateDeepCopy());
-      prefs->SetValue(prefs::kForceYouTubeSafetyMode, value->CreateDeepCopy());
+
+      // Note that ForceYouTubeRestrict is an int policy,
+      // we cannot simply deep copy value, which is a boolean.
+      if (value->GetAsBoolean(&enabled))
+        prefs->SetValue(prefs::kForceYouTubeRestrict,
+                        base::MakeUnique<base::FundamentalValue>(
+                            (int)(enabled ? safe_search_util::YTRM_MODERATE

[Thiemo Nagel, 2016/08/12 12:12:20]

I don't think the (int) cast is necessary.  In general we avoid casts like the
plague.

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

Done.

+                                          : safe_search_util::YTRM_OFF)));
     }
   }
 
@@ -616,6 +632,34 @@ class ForceSafeSearchPolicyHandler : public TypeCheckingPolicyHandler {
   DISALLOW_COPY_AND_ASSIGN(ForceSafeSearchPolicyHandler);
 };
 
+class ForceYouTubeSafetyModePolicyHandler : public TypeCheckingPolicyHandler {

[Thiemo Nagel, 2016/08/12 12:12:20]

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.

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

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?

+ public:
+  ForceYouTubeSafetyModePolicyHandler()
+      : TypeCheckingPolicyHandler(key::kForceYouTubeSafetyMode,
+                                  base::Value::TYPE_BOOLEAN) {}
+  ~ForceYouTubeSafetyModePolicyHandler() override {}
+
+  // ConfigurationPolicyHandler implementation:
+  void ApplyPolicySettings(const PolicyMap& policies,
+                           PrefValueMap* prefs) override {
+    // If only the deprecated ForceYouTubeSafetyMode policy is set,
+    // but not ForceYouTubeRestrict, set ForceYouTubeRestrict to Moderate.
+    if (policies.GetValue(key::kForceYouTubeRestrict))
+      return;
+
+    const base::Value* value = policies.GetValue(policy_name());
+    bool enabled;
+    if (value && value->GetAsBoolean(&enabled))
+      prefs->SetValue(prefs::kForceYouTubeRestrict,
+                      base::MakeUnique<base::FundamentalValue>(
+                          (int)(enabled ? safe_search_util::YTRM_MODERATE

[Thiemo Nagel, 2016/08/12 12:12:20]

The (int) cast doesn't seem necessary.

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

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

+                                        : safe_search_util::YTRM_OFF)));
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ForceYouTubeSafetyModePolicyHandler);
+};
+
 #if defined(ENABLE_EXTENSIONS)
 void GetExtensionAllowedTypesMap(
     ScopedVector<StringMappingListPolicyHandler::MappingEntry>* result) {
@@ -666,6 +710,8 @@ std::unique_ptr<ConfigurationPolicyHandlerList> BuildHandlerList(
   handlers->AddHandler(base::WrapUnique(new AutofillPolicyHandler()));
   handlers->AddHandler(base::WrapUnique(new DefaultSearchPolicyHandler()));
   handlers->AddHandler(base::WrapUnique(new ForceSafeSearchPolicyHandler()));
+  handlers->AddHandler(
+      base::WrapUnique(new ForceYouTubeSafetyModePolicyHandler()));
   handlers->AddHandler(base::WrapUnique(new IncognitoModePolicyHandler()));
   handlers->AddHandler(
       base::WrapUnique(new ManagedBookmarksPolicyHandler(chrome_schema)));