HostContentSettingsMap refactoring.

chrome/browser/content_settings/host_content_settings_map.cc

Index: chrome/browser/content_settings/host_content_settings_map.cc
diff --git a/chrome/browser/content_settings/host_content_settings_map.cc b/chrome/browser/content_settings/host_content_settings_map.cc
index 3d15073c40ece187724b2e385ef29fceae11819d..737e45025f6cbe5b9404ec526a257ef816a55988 100644
--- a/chrome/browser/content_settings/host_content_settings_map.cc
+++ b/chrome/browser/content_settings/host_content_settings_map.cc
@@ -7,8 +7,6 @@
 #include <list>
 
 #include "base/command_line.h"
-#include "base/string_util.h"
-#include "base/utf_string_conversions.h"
 #include "chrome/browser/content_settings/content_settings_details.h"
 #include "chrome/browser/content_settings/content_settings_extension_provider.h"
 #include "chrome/browser/content_settings/content_settings_observable_provider.h"
@@ -16,10 +14,10 @@
 #include "chrome/browser/content_settings/content_settings_pref_provider.h"
 #include "chrome/browser/content_settings/content_settings_provider.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
+#include "chrome/browser/content_settings/cookie_settings.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
@@ -35,16 +33,6 @@
 
 namespace {
 
-// Returns true if we should allow all content types for this URL.  This is
-// true for various internal objects like chrome:// URLs, so UI and other
-// things users think of as "not webpages" don't break.
-static bool ShouldAllowAllContent(const GURL& url) {
-  return url.SchemeIs(chrome::kChromeDevToolsScheme) ||
-         url.SchemeIs(chrome::kChromeInternalScheme) ||
-         url.SchemeIs(chrome::kChromeUIScheme) ||
-         url.SchemeIs(chrome::kExtensionScheme);
-}
-
 typedef linked_ptr<content_settings::DefaultProviderInterface>
     DefaultContentSettingsProviderPtr;
 typedef std::vector<DefaultContentSettingsProviderPtr>::iterator
@@ -73,10 +61,7 @@ HostContentSettingsMap::HostContentSettingsMap(
     ExtensionService* extension_service,
     bool incognito)
     : prefs_(prefs),
-      is_off_the_record_(incognito),
-      updating_preferences_(false),
-      block_third_party_cookies_(false),
-      is_block_third_party_cookies_managed_(false) {
+      is_off_the_record_(incognito) {
   // The order in which the default content settings providers are created is
   // critical, as providers that are further down in the list (i.e. added later)
   // override providers further up.
@@ -92,22 +77,7 @@ HostContentSettingsMap::HostContentSettingsMap(
   default_content_settings_providers_.push_back(
       make_linked_ptr(policy_default_provider));
 
-  // TODO(markusheintz): Discuss whether it is sensible to move migration code
-  // to PrefContentSettingsProvider.
-  MigrateObsoleteCookiePref();
-
   // Read misc. global settings.
-  block_third_party_cookies_ =
-      prefs_->GetBoolean(prefs::kBlockThirdPartyCookies);
-  if (block_third_party_cookies_) {
-    UserMetrics::RecordAction(
-        UserMetricsAction("ThirdPartyCookieBlockingEnabled"));
-  } else {
-    UserMetrics::RecordAction(
-        UserMetricsAction("ThirdPartyCookieBlockingDisabled"));
-  }
-  is_block_third_party_cookies_managed_ =
-      prefs_->IsManagedPreference(prefs::kBlockThirdPartyCookies);
 
   // User defined non default content settings are provided by the PrefProvider.
   // The order in which the content settings providers are created is critical,
@@ -129,9 +99,6 @@ HostContentSettingsMap::HostContentSettingsMap(
   provider = new content_settings::PrefProvider(prefs_, is_off_the_record_);
   provider->AddObserver(this);
   content_settings_providers_.push_back(make_linked_ptr(provider));
-
-  pref_change_registrar_.Init(prefs_);
-  pref_change_registrar_.Add(prefs::kBlockThirdPartyCookies, this);
 }
 
 // static
@@ -180,42 +147,6 @@ ContentSettings HostContentSettingsMap::GetDefaultContentSettings() const {
   return output;
 }
 
-ContentSetting HostContentSettingsMap::GetCookieContentSetting(
-    const GURL& url,
-    const GURL& first_party_url,
-    bool setting_cookie) const {
-  if (ShouldAllowAllContent(first_party_url))
-    return CONTENT_SETTING_ALLOW;
-
-  // First get any host-specific settings.
-  ContentSetting setting = GetNonDefaultContentSetting(url,
-      first_party_url, CONTENT_SETTINGS_TYPE_COOKIES, "");
-
-  // If no explicit exception has been made and third-party cookies are blocked
-  // by default, apply that rule.
-  if (setting == CONTENT_SETTING_DEFAULT && BlockThirdPartyCookies()) {
-    bool strict = CommandLine::ForCurrentProcess()->HasSwitch(
-        switches::kBlockReadingThirdPartyCookies);
-    net::StaticCookiePolicy policy(strict ?
-        net::StaticCookiePolicy::BLOCK_ALL_THIRD_PARTY_COOKIES :
-        net::StaticCookiePolicy::BLOCK_SETTING_THIRD_PARTY_COOKIES);
-    int rv;
-    if (setting_cookie)
-      rv = policy.CanSetCookie(url, first_party_url);
-    else
-      rv = policy.CanGetCookies(url, first_party_url);
-    DCHECK_NE(net::ERR_IO_PENDING, rv);
-    if (rv != net::OK)
-      setting = CONTENT_SETTING_BLOCK;
-  }
-
-  // If no other policy has changed the setting, use the default.
-  if (setting == CONTENT_SETTING_DEFAULT)
-    setting = GetDefaultContentSetting(CONTENT_SETTINGS_TYPE_COOKIES);
-
-  return setting;
-}
-
 ContentSetting HostContentSettingsMap::GetContentSetting(
     const GURL& primary_url,
     const GURL& secondary_url,
@@ -338,6 +269,13 @@ void HostContentSettingsMap::GetSettingsForOneType(
   }
 }
 
+CookieSettings* HostContentSettingsMap::GetCookieSettings() {

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

Uhh, this isn't thread safe at all. CookieSettings needs to be constructed on
the UI thread, and additionally there's a race condition here. Why the lazy
initialization anyway? And strictly speaking, HostContentSettingsMap shouldn't
even know about CookieSettings.

[markusheintz_, 2011/08/29 08:47:09]

I guess that's my fault as I suggested to have such a setter. I still wanted the
host content settings map to be the single source of content settings by
providing. Anyway what you suggest might actually be better.

However if cookie settings are provided by the |CookieSettings| I don't want
that cookie settings can be set through the host content settings map as there
should be only one single authority for cookie settings. I don't want some
clients to use |CookieSettings| and other the |HostContentSettingsMap|.

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

This code is removed now, as CookieSettings is a ProfileKeyedService.

+  if (!cookie_settings_.get()) {
+    cookie_settings_ = new CookieSettings(this, prefs_, is_off_the_record_);
+  }
+  return cookie_settings_.get();
+}
+
 void HostContentSettingsMap::SetDefaultContentSetting(
     ContentSettingsType content_type,
     ContentSetting setting) {
@@ -420,7 +358,7 @@ bool HostContentSettingsMap::IsSettingAllowedForType(
     return true;
   }
   switch (content_type) {
-    case CONTENT_SETTINGS_TYPE_COOKIES:
+    case CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY:
       return (setting == CONTENT_SETTING_SESSION_ONLY);
     case CONTENT_SETTINGS_TYPE_PLUGINS:
       return (setting == CONTENT_SETTING_ASK &&
@@ -435,32 +373,6 @@ bool HostContentSettingsMap::IsSettingAllowedForType(
   }
 }
 
-void HostContentSettingsMap::SetBlockThirdPartyCookies(bool block) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(prefs_);
-
-  // This setting may not be directly modified for OTR sessions.  Instead, it
-  // is synced to the main profile's setting.
-  if (is_off_the_record_) {
-    NOTREACHED();
-    return;
-  }
-
-  // If the preference block-third-party-cookies is managed then do not allow to
-  // change it.
-  if (prefs_->IsManagedPreference(prefs::kBlockThirdPartyCookies)) {
-    NOTREACHED();
-    return;
-  }
-
-  {
-    base::AutoLock auto_lock(lock_);
-    block_third_party_cookies_ = block;
-  }
-
-  prefs_->SetBoolean(prefs::kBlockThirdPartyCookies, block);
-}
-
 void HostContentSettingsMap::OnContentSettingChanged(
     ContentSettingsPattern primary_pattern,
     ContentSettingsPattern secondary_pattern,
@@ -476,31 +388,15 @@ void HostContentSettingsMap::OnContentSettingChanged(
       Details<const ContentSettingsDetails>(&details));
 }
 
-void HostContentSettingsMap::Observe(int type,
-                                     const NotificationSource& source,
-                                     const NotificationDetails& details) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (type == chrome::NOTIFICATION_PREF_CHANGED) {
-    DCHECK_EQ(prefs_, Source<PrefService>(source).ptr());
-    if (updating_preferences_)
-      return;
-
-    std::string* name = Details<std::string>(details).ptr();
-    if (*name == prefs::kBlockThirdPartyCookies) {
-      base::AutoLock auto_lock(lock_);
-      block_third_party_cookies_ = prefs_->GetBoolean(
-          prefs::kBlockThirdPartyCookies);
-      is_block_third_party_cookies_managed_ =
-          prefs_->IsManagedPreference(
-              prefs::kBlockThirdPartyCookies);
-    } else {
-      NOTREACHED() << "Unexpected preference observed";
-      return;
-    }
-  } else {
-    NOTREACHED() << "Unexpected notification";
-  }
+// static
+// Returns true if we should allow all content types for this URL.  This is
+// true for various internal objects like chrome:// URLs, so UI and other
+// things users think of as "not webpages" don't break.
+bool HostContentSettingsMap::ShouldAllowAllContent(const GURL& url) {
+  return url.SchemeIs(chrome::kChromeDevToolsScheme) ||
+         url.SchemeIs(chrome::kChromeInternalScheme) ||
+         url.SchemeIs(chrome::kChromeUIScheme) ||
+         url.SchemeIs(chrome::kExtensionScheme);
 }
 
 HostContentSettingsMap::~HostContentSettingsMap() {
@@ -521,7 +417,10 @@ bool HostContentSettingsMap::IsDefaultContentSettingManaged(
 void HostContentSettingsMap::ShutdownOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prefs_);
-  pref_change_registrar_.RemoveAll();
+
+  if (cookie_settings_.get())
+    cookie_settings_->ShutdownOnUIThread();
+
   prefs_ = NULL;
   for (ProviderIterator it = content_settings_providers_.begin();
        it != content_settings_providers_.end();
@@ -534,19 +433,3 @@ void HostContentSettingsMap::ShutdownOnUIThread() {
     (*it)->ShutdownOnUIThread();
   }
 }
-
-void HostContentSettingsMap::MigrateObsoleteCookiePref() {
-  if (prefs_->HasPrefPath(prefs::kCookieBehavior)) {
-    int cookie_behavior = prefs_->GetInteger(prefs::kCookieBehavior);
-    prefs_->ClearPref(prefs::kCookieBehavior);
-    if (!prefs_->HasPrefPath(prefs::kDefaultContentSettings)) {
-        SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_COOKIES,
-            (cookie_behavior == net::StaticCookiePolicy::BLOCK_ALL_COOKIES) ?
-                CONTENT_SETTING_BLOCK : CONTENT_SETTING_ALLOW);
-    }
-    if (!prefs_->HasPrefPath(prefs::kBlockThirdPartyCookies)) {
-      SetBlockThirdPartyCookies(cookie_behavior ==
-          net::StaticCookiePolicy::BLOCK_SETTING_THIRD_PARTY_COOKIES);
-    }
-  }
-}