Added a ForceYouTubeRestrict policy and deprecated the old ForceYouTubeSafetyMode policy

chrome/browser/policy/configuration_policy_handler_list_factory.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/policy/configuration_policy_handler_list_factory.h"
 
 #include <limits.h>
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_vector.h"
 #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"
 #include "chrome/browser/policy/network_prediction_policy_handler.h"
 #include "chrome/browser/profiles/incognito_mode_policy_handler.h"
 #include "chrome/browser/sessions/restore_on_startup_policy_handler.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/features.h"
 #include "chrome/common/pref_names.h"
 #include "components/bookmarks/common/bookmark_pref_names.h"
@@ skipping 78 matching lines @@
     base::Value::TYPE_BOOLEAN },
   { key::kPacHttpsUrlStrippingEnabled,
     prefs::kPacHttpsUrlStrippingEnabled,
     base::Value::TYPE_BOOLEAN },
   { key::kSafeBrowsingEnabled,
     prefs::kSafeBrowsingEnabled,
     base::Value::TYPE_BOOLEAN },
   { 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 },
   { key::kPrintingEnabled,
     prefs::kPrintingEnabled,
     base::Value::TYPE_BOOLEAN },
   { key::kDisablePrintPreview,
     prefs::kPrintPreviewDisabled,
     base::Value::TYPE_BOOLEAN },
   { key::kDefaultPrinterSelection,
@@ skipping 461 matching lines @@
 class ForceSafeSearchPolicyHandler : public TypeCheckingPolicyHandler {
  public:
   ForceSafeSearchPolicyHandler()
       : TypeCheckingPolicyHandler(key::kForceSafeSearch,
                                   base::Value::TYPE_BOOLEAN) {}
   ~ForceSafeSearchPolicyHandler() override {}
 
   // 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)));
     }
   }
 
  private:
   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) {
   // Mapping from extension type names to Manifest::Type.
   for (size_t index = 0;
        index < extensions::schema_constants::kAllowedTypesMapSize;
        ++index) {
     const extensions::schema_constants::AllowedTypesMapEntry& entry =
         extensions::schema_constants::kAllowedTypesMap[index];
     result->push_back(new StringMappingListPolicyHandler::MappingEntry(
@@ skipping 30 matching lines @@
           base::Bind(&GetChromePolicyDetails)));
   for (size_t i = 0; i < arraysize(kSimplePolicyMap); ++i) {
     handlers->AddHandler(base::WrapUnique(new SimplePolicyHandler(
         kSimplePolicyMap[i].policy_name, kSimplePolicyMap[i].preference_path,
         kSimplePolicyMap[i].value_type)));
   }
 
   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)));
   handlers->AddHandler(base::WrapUnique(new ProxyPolicyHandler()));
   handlers->AddHandler(base::WrapUnique(new URLBlacklistPolicyHandler()));
 
   handlers->AddHandler(base::WrapUnique(new SimpleSchemaValidatingPolicyHandler(
       key::kCertificateTransparencyEnforcementDisabledForUrls,
       certificate_transparency::prefs::kCTExcludedHosts, chrome_schema,
       SCHEMA_STRICT, SimpleSchemaValidatingPolicyHandler::RECOMMENDED_ALLOWED,
@@ skipping 189 matching lines @@
       SimpleSchemaValidatingPolicyHandler::RECOMMENDED_ALLOWED,
       SimpleSchemaValidatingPolicyHandler::MANDATORY_PROHIBITED)));
   handlers->AddHandler(base::WrapUnique(
       new chromeos::KeyPermissionsPolicyHandler(chrome_schema)));
 #endif  // defined(OS_CHROMEOS)
 
   return handlers;
 }
 
 }  // namespace policy