Remove HostContentSettingsMap::ContentTypeHasCompoundValue function

components/content_settings/core/browser/content_settings_pref.cc

 // Copyright 2015 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 "components/content_settings/core/browser/content_settings_pref.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/strings/string_split.h"
 #include "base/time/clock.h"
+#include "components/content_settings/core/browser/content_settings_registry.h"
 #include "components/content_settings/core/browser/content_settings_rule.h"
 #include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/pref_names.h"
 #include "url/gurl.h"
 
 namespace {
 
@@ skipping 277 matching lines @@
               new base::FundamentalValue(setting));
           value_map_.SetValue(pattern_pair.first,
                               pattern_pair.second,
                               content_type_,
                               resource_identifier,
                               setting_ptr->DeepCopy());
         }
       }
     }
     base::Value* value = NULL;
-    if (HostContentSettingsMap::ContentTypeHasCompoundValue(content_type_)) {
-      const base::DictionaryValue* setting = NULL;
-      // TODO(xians): Handle the non-dictionary types.
-      if (settings_dictionary->GetDictionaryWithoutPathExpansion(
-              kSettingPath, &setting)) {
-        DCHECK(!setting->empty());
-        value = setting->DeepCopy();
-      }
-    } else {
+    if (ContentSettingsRegistry::GetInstance()->Get(content_type_)) {
       int setting = CONTENT_SETTING_DEFAULT;
       if (settings_dictionary->GetIntegerWithoutPathExpansion(
               kSettingPath, &setting)) {
         DCHECK_NE(CONTENT_SETTING_DEFAULT, setting);
         setting = FixObsoleteCookiePromptMode(content_type_,

[Bernhard Bauer, 2015/09/23 11:30:37]

Actually, we can probably get rid of this migration code now (it's about 4 years
old at this point). Then we could also relax the restriction that non-content
website settings have to be a dictionary, simplify this so it will always make a
deep copy, and just DCHECK that the value is allowed (via the new
HostContentSettingsMap method).

[raymes, 2015/09/24 03:07:51]

Let me know if I did what you were thinking :) 

One downside is that we no longer check that the value is not
CONTENT_SETTING_DEFAULT (it's allowed by IsValueAllowedForType). But I also feel
CONTENT_SETTING_DEFAULT can be confusing/ambiguous in many circumstances. It's a
value that can be used in the code but never persisted. Maybe we should remove
it eventually?

Also, I'm wondering whether if the value isn't allowed, we should actually not
load it (as opposed to just DCHECKing). AFAIK this value is coming from a file
and may not be valid. WDYT?

[Bernhard Bauer, 2015/09/24 08:48:26]

CONTENT_SETTING_DEFAULT is useful for interfaces that deal with a plain
ContentSetting, to indicate the absence of a value. For base::Value* though,
absence is better indicated with a nullptr, so we could actually include the
check for DEFAULT.
Here's my thinking: In a Debug build, we want to notify the user if the value is
invalid, to catch cases where e.g. we write out invalid data, and a DCHECK will
do that.

In a Release build, we don't want to crash (as we need to be able to deal with
arbitrary data there), but the question is whether we want to silently drop
those values or just keep ignoring them. I'm slightly leaning towards keeping
them, because that means we won't start deleting data if we get a bug in the
logic that causes us to think all values are invalid, and we also won't have to
deal with syncing between clients with different versions if we ever change the
logic. If you feel strongly about it though, I'd be okay with dropping the
values.

[raymes, 2015/09/28 06:45:23]

Sounds good. I've left the behavior as it is for now but I added a check for
CONTENT_SETTING_DEFAULT :)

I propose we change the name for CONTENT_SETTING_DEFAULT to
CONTENT_SETTING_INVALID. I think it can be easily confused to have something to
do with the default value but that's not necessarily true. WDYT?

                                               ContentSetting(setting));
         value = new base::FundamentalValue(setting);
       }
+    } else {
+      const base::DictionaryValue* setting = NULL;
+      // TODO(xians): Handle the non-dictionary types.
+      if (settings_dictionary->GetDictionaryWithoutPathExpansion(kSettingPath,
+                                                                 &setting)) {
+        DCHECK(!setting->empty());
+        value = setting->DeepCopy();
+      }
     }
 
     if (value != NULL) {
       scoped_ptr<base::Value> value_ptr(value);
       value_map_.SetValue(pattern_pair.first,
                           pattern_pair.second,
                           content_type_,
                           ResourceIdentifier(),
                           value->DeepCopy());
       if (content_type_ == CONTENT_SETTINGS_TYPE_COOKIES) {
@@ skipping 170 matching lines @@
 
 void ContentSettingsPref::AssertLockNotHeld() const {
 #if !defined(NDEBUG)
   // |Lock::Acquire()| will assert if the lock is held by this thread.
   lock_.Acquire();
   lock_.Release();
 #endif
 }
 
 }  // namespace content_settings