HostContentSettingsMap refactoring.

chrome/browser/ui/webui/options/content_settings_handler.cc

Index: chrome/browser/ui/webui/options/content_settings_handler.cc
diff --git a/chrome/browser/ui/webui/options/content_settings_handler.cc b/chrome/browser/ui/webui/options/content_settings_handler.cc
index 6ef0239cccb81f8c07f55017bdbd8b8a0da91012..413649d59178485dd7e21867ede7e297de9d55be 100644
--- a/chrome/browser/ui/webui/options/content_settings_handler.cc
+++ b/chrome/browser/ui/webui/options/content_settings_handler.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/content_settings/content_settings_details.h"
 #include "chrome/browser/content_settings/content_settings_pattern.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
+#include "chrome/browser/content_settings/cookie_settings.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
@@ -61,6 +62,7 @@ const ContentSettingsTypeNameEntry kContentSettingsTypeGroupNames[] = {
   {CONTENT_SETTINGS_TYPE_GEOLOCATION, "location"},
   {CONTENT_SETTINGS_TYPE_NOTIFICATIONS, "notifications"},
   {CONTENT_SETTINGS_TYPE_INTENTS, "intents"},
+  {CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY, "cookies-session-only"},

[Bernhard Bauer, 2011/08/28 19:18:49]

This seems weird, as this content type isn't exposed in the UI. Also, this map
is only used for looking up content types from strings, so it will never be
called with "cookies-session-only".

[marja, 2011/08/30 10:10:38]

What breaks if that is simply removed: ContentSettingsHandler is reading the
"list of exceptions" to dispay in the UI, for all content types. It loops from
CONTENT_SETTINGS_TYPE_DEFAULT + 1 to CONTENT_SETTING_NUM_TYPES, and for other
types than geolocation or notifications, it calls
UpdateExeceptionsViewFromHostContentSettingsMap, and that calls
ContentSettingsTypeToGroupName. That just asserts, if there is no entry for the
content setting in kContentSettingsTypeGroupNames.

Should I e.g., make it loop through a separate list of "ui content types"
instead of all content types, as a part of this cl? Or some other fix (like,
loop through all types as before, but ignore it if it doesn't have an entry in
kContentSettingsTypeGroupNames)?

[Bernhard Bauer, 2011/08/30 11:03:12]

Ah, I missed |ContentSettingsTypeToGroupName| below (and there is UI for
intents, it's just hidden).

I still think what we should do here is return NULL in
|ContentSettingsTypeToGroupName| for an unknown content type, and not update the
UI in that case. That should allow us to add more content types without having
to change the Web UI code.

[markusheintz_, 2011/08/30 11:49:34]

How about providing a method that converts a ContentSettingsType to a string and
vice versa? Then we don't need this array at all. This method should ideally be
provided by content_settings_type.h

 };
 
 ContentSettingsType ContentSettingsTypeFromGroupName(const std::string& name) {
@@ -607,6 +609,10 @@ void ContentSettingsHandler::SetContentFilter(const ListValue* args) {
     Profile* profile = Profile::FromWebUI(web_ui_);
     DesktopNotificationServiceFactory::GetForProfile(profile)->
         SetDefaultContentSetting(default_setting);
+  } else if (content_type == CONTENT_SETTINGS_TYPE_COOKIES) {
+    GetContentSettingsMap()->GetCookieSettings()->SetDefaultSetting(
+        default_setting);
+
   } else {
     GetContentSettingsMap()->
         SetDefaultContentSetting(content_type, default_setting);
@@ -662,15 +668,24 @@ void ContentSettingsHandler::RemoveException(const ListValue* args) {
     HostContentSettingsMap* settings_map =
         mode == "normal" ? GetContentSettingsMap() :
                            GetOTRContentSettingsMap();
+
     // The settings map could be null if the mode was OTR but the OTR profile
     // got destroyed before we received this message.
     if (settings_map) {
-      settings_map->SetContentSetting(
-          ContentSettingsPattern::FromString(pattern),
-          ContentSettingsPattern::Wildcard(),
-          ContentSettingsTypeFromGroupName(type_string),
-          "",
-          CONTENT_SETTING_DEFAULT);
+      if (type == CONTENT_SETTINGS_TYPE_COOKIES) {
+        settings_map->GetCookieSettings()->ResetCookieAllowed(
+            ContentSettingsPattern::FromString(pattern),
+            ContentSettingsPattern::Wildcard());
+        settings_map->GetCookieSettings()->ResetCookieSessionOnly(
+            ContentSettingsPattern::FromString(pattern));
+      } else {
+        settings_map->SetContentSetting(
+            ContentSettingsPattern::FromString(pattern),
+            ContentSettingsPattern::Wildcard(),
+            ContentSettingsTypeFromGroupName(type_string),
+            "",
+            CONTENT_SETTING_DEFAULT);
+      }
     }
   }
 }
@@ -696,16 +711,29 @@ void ContentSettingsHandler::SetException(const ListValue* args) {
   HostContentSettingsMap* settings_map =
       mode == "normal" ? GetContentSettingsMap() :
                          GetOTRContentSettingsMap();
-
   // The settings map could be null if the mode was OTR but the OTR profile
   // got destroyed before we received this message.
   if (!settings_map)
     return;
-  settings_map->SetContentSetting(ContentSettingsPattern::FromString(pattern),
-                                  ContentSettingsPattern::Wildcard(),
-                                  type,
-                                  "",
-                                  ContentSettingFromString(setting));
+
+  if (type == CONTENT_SETTINGS_TYPE_COOKIES) {
+    ContentSetting content_setting = ContentSettingFromString(setting);
+    CookieSettings* cookie_settings = settings_map->GetCookieSettings();
+    bool allow = (content_setting == CONTENT_SETTING_ALLOW ||
+                  content_setting == CONTENT_SETTING_SESSION_ONLY);
+    bool session_only = (content_setting == CONTENT_SETTING_SESSION_ONLY);
+    cookie_settings->SetCookieAllowed(
+          ContentSettingsPattern::FromString(pattern),
+          ContentSettingsPattern::Wildcard(), allow);
+    cookie_settings->SetCookieSessionOnly(
+        ContentSettingsPattern::FromString(pattern), session_only);

[Bernhard Bauer, 2011/08/28 19:18:49]

Maybe we could add a convenience method for this to CookieSettings?

[marja, 2011/09/01 11:03:19]

Now there is CookieSettings::SetCookieSetting, which makes this a lot simpler.

[Bernhard Bauer, 2011/09/01 11:41:54]

Nice, thank you!

+  } else {
+    settings_map->SetContentSetting(ContentSettingsPattern::FromString(pattern),
+                                    ContentSettingsPattern::Wildcard(),
+                                    type,
+                                    "",
+                                    ContentSettingFromString(setting));
+  }
 }
 
 void ContentSettingsHandler::CheckExceptionPatternValidity(