Making structure for ContentSettings and its corresponding strings.

components/content_settings/core/browser/content_settings_utils.cc

 // Copyright (c) 2012 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_utils.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/prefs/pref_registry.h"
 #include "base/strings/string_split.h"
 #include "base/values.h"
 #include "components/content_settings/core/browser/content_settings_provider.h"
 #include "components/content_settings/core/browser/content_settings_rule.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/browser/website_settings_info.h"
 #include "components/content_settings/core/browser/website_settings_registry.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "url/gurl.h"
 
 namespace {
 
 const char kPatternSeparator[] = ",";
 
+struct ContentSettingsToFromString {

[Bernhard Bauer, 2015/10/01 14:36:09]

Nit: ContentSettingsStringMapping might be better. Same for the constant.

[Deepak, 2015/10/03 09:02:11]

Done.

+  ContentSetting content_setting;
+  const char* content_setting_str;
+};
+const ContentSettingsToFromString kContentSettingsToFromString[] = {
+    {CONTENT_SETTING_DEFAULT, "default"},
+    {CONTENT_SETTING_ALLOW, "allow"},
+    {CONTENT_SETTING_BLOCK, "block"},
+    {CONTENT_SETTING_ASK, "ask"},
+    {CONTENT_SETTING_SESSION_ONLY, "session"},
+    {CONTENT_SETTING_DETECT_IMPORTANT_CONTENT, "detect"},
+};
+static_assert(arraysize(kContentSettingsToFromString) ==
+                  CONTENT_SETTING_NUM_SETTINGS,
+              "kContentSettingsToFromString should have "
+              "CONTENT_SETTING_NUM_SETTINGS elements");
 }  // namespace
 
 namespace content_settings {
 
 std::string ContentSettingToString(ContentSetting setting) {
-  switch (setting) {
-    case CONTENT_SETTING_ALLOW:
-      return "allow";
-    case CONTENT_SETTING_ASK:
-      return "ask";
-    case CONTENT_SETTING_BLOCK:
-      return "block";
-    case CONTENT_SETTING_SESSION_ONLY:
-      return "session";
-    case CONTENT_SETTING_DETECT_IMPORTANT_CONTENT:
-      return "detect";
-    case CONTENT_SETTING_DEFAULT:
-      return "default";
-    case CONTENT_SETTING_NUM_SETTINGS:
-      NOTREACHED();
-  }
-
-  return ResourceIdentifier();
+  DCHECK(setting >= CONTENT_SETTING_DEFAULT &&

[Bernhard Bauer, 2015/10/01 14:36:09]

Split this up into two checks, and use DCHECK_GE / DCHECK_LT.

[Deepak, 2015/10/03 09:02:11]

Done.

+         setting < CONTENT_SETTING_NUM_SETTINGS);
+  return kContentSettingsToFromString[setting].content_setting_str;

[Bernhard Bauer, 2015/10/01 14:36:09]

There is an implicit assumption here that
`kContentSettingsToFromString[setting].content_setting == setting`. At the very
least, we should verify that with a DCHECK, and add a unit test that exercises
this for all possible values. Even that is not great, as DCHECKs are not always
enabled in Release builds (and if they are, the stack traces are hard to
decipher), so a unit test on a Release bot will be kind of worthless.

But testing the roundtrip (content setting to string to content setting) should
work in a unit test, and fail if someone updates the ContentSetting enum without
updating this file.

[Deepak, 2015/10/03 09:02:12]

Done.

 }
 
 ContentSetting ContentSettingFromString(const std::string& name) {
-  if (name == "allow")
-    return CONTENT_SETTING_ALLOW;
-  if (name == "ask")
-    return CONTENT_SETTING_ASK;
-  if (name == "block")
-    return CONTENT_SETTING_BLOCK;
-  if (name == "session")
-    return CONTENT_SETTING_SESSION_ONLY;
-  if (name == "detect")
-    return CONTENT_SETTING_DETECT_IMPORTANT_CONTENT;
+  size_t i = 0;

[Bernhard Bauer, 2015/10/01 14:36:09]

Inline this into the loop?

[Deepak, 2015/10/03 09:02:11]

Done.

+  for (i = 0; i < arraysize(kContentSettingsToFromString); ++i) {
+    if (name == kContentSettingsToFromString[i].content_setting_str)
+      break;

[Bernhard Bauer, 2015/10/01 14:36:09]

You could directly return here, then you only need to do the NOTREACHED() below.

[Deepak, 2015/10/03 09:02:11]

Done.

+  }
+  ContentSetting content_setting = static_cast<ContentSetting>(i);
+  if (content_setting != CONTENT_SETTING_NUM_SETTINGS &&
+      content_setting != CONTENT_SETTING_DEFAULT)
+    return kContentSettingsToFromString[i].content_setting;
 
   NOTREACHED() << name << " is not a recognized content setting.";
   return CONTENT_SETTING_DEFAULT;
 }
 
 std::string CreatePatternString(
     const ContentSettingsPattern& item_pattern,
     const ContentSettingsPattern& top_level_frame_pattern) {
   return item_pattern.ToString()
          + std::string(kPatternSeparator)
@@ skipping 115 matching lines @@
       CONTENT_SETTINGS_TYPE_IMAGES,
       ResourceIdentifier(),
       &(rules->image_rules));
   map->GetSettingsForOneType(
       CONTENT_SETTINGS_TYPE_JAVASCRIPT,
       ResourceIdentifier(),
       &(rules->script_rules));
 }
 
 }  // namespace content_settings