Making structure for ContentSettings and its corresponding strings.

components/content_settings/core/browser/content_settings_utils.cc

Index: components/content_settings/core/browser/content_settings_utils.cc
diff --git a/components/content_settings/core/browser/content_settings_utils.cc b/components/content_settings/core/browser/content_settings_utils.cc
index 8a629b0fa5c20a53a40d759a46c1ad5a3faae9aa..0073c528cc433f05e71dde414140546533212ad9 100644
--- a/components/content_settings/core/browser/content_settings_utils.cc
+++ b/components/content_settings/core/browser/content_settings_utils.cc
@@ -25,42 +25,42 @@ 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;