HostContentSettingsMap refactoring.

chrome/browser/content_settings/host_content_settings_map.h

Index: chrome/browser/content_settings/host_content_settings_map.h
diff --git a/chrome/browser/content_settings/host_content_settings_map.h b/chrome/browser/content_settings/host_content_settings_map.h
index eae366e3dd05d672c0a2970bb6d5d80aa4ba742d..771095622014891f31f5fbcfa2dfe3a4953e976e 100644
--- a/chrome/browser/content_settings/host_content_settings_map.h
+++ b/chrome/browser/content_settings/host_content_settings_map.h
@@ -39,7 +39,6 @@ class Profile;
 
 class HostContentSettingsMap
     : public content_settings::Observer,
-      public NotificationObserver,
       public base::RefCountedThreadSafe<HostContentSettingsMap> {
  public:
   typedef Tuple4<ContentSettingsPattern,
@@ -77,16 +76,6 @@ class HostContentSettingsMap
       ContentSettingsType content_type,
       const std::string& resource_identifier) const;
 
-  // Gets the content setting for cookies. This takes the third party cookie
-  // flag into account, and therefore needs to know whether we read or write a
-  // cookie.
-  //
-  // This may be called on any thread.
-  ContentSetting GetCookieContentSetting(
-      const GURL& url,
-      const GURL& first_party_url,
-      bool setting_cookie) const;
-
   // Returns a single ContentSetting which applies to the given URLs or
   // CONTENT_SETTING_DEFAULT, if no exception applies. Note that certain
   // internal schemes are whitelisted. For ContentSettingsTypes that require an
@@ -172,18 +161,6 @@ class HostContentSettingsMap
   static bool IsSettingAllowedForType(ContentSetting setting,
                                       ContentSettingsType content_type);
 
-  // This setting trumps any host-specific settings.
-  bool BlockThirdPartyCookies() const { return block_third_party_cookies_; }
-  bool IsBlockThirdPartyCookiesManaged() const {
-    return is_block_third_party_cookies_managed_;
-  }
-
-  // Sets whether we block all third-party cookies. This method must not be
-  // invoked on an incognito map.
-  //
-  // This should only be called on the UI thread.
-  void SetBlockThirdPartyCookies(bool block);
-
   // Returns true if the default setting for the |content_type| is managed.
   bool IsDefaultContentSettingManaged(ContentSettingsType content_type) const;
 
@@ -200,11 +177,6 @@ class HostContentSettingsMap
       ContentSettingsType content_type,
       std::string resource_identifier);
 
-  // NotificationObserver implementation.
-  virtual void Observe(int type,
-                       const NotificationSource& source,
-                       const NotificationDetails& details);
-
  private:
   friend class base::RefCountedThreadSafe<HostContentSettingsMap>;
 
@@ -216,15 +188,9 @@ class HostContentSettingsMap
       ContentSettingsType content_type,
       const std::string& resource_identifier) const;
 
-  // Various migration methods (old cookie, popup and per-host data gets
-  // migrated to the new format).
-  void MigrateObsoleteCookiePref();
-
   // Weak; owned by the Profile.
   PrefService* prefs_;
 
-  PrefChangeRegistrar pref_change_registrar_;
-
   // Whether this settings map is for an OTR session.
   bool is_off_the_record_;
 
@@ -240,14 +206,71 @@ class HostContentSettingsMap
   std::vector<linked_ptr<content_settings::ProviderInterface> >
       content_settings_providers_;
 
+  DISALLOW_COPY_AND_ASSIGN(HostContentSettingsMap);
+};
+
+// FIXME(marja): This should be moved into a separate file.

[markusheintz_, 2011/08/24 12:29:40]

Please move to a new file: cookie_settings.h

[marja, 2011/08/25 13:56:11]

Done.

+class CookieContentSettings

[markusheintz_, 2011/08/24 12:29:40]

Maybe we could just call it 'CookieSettings' since this class allows to query
settings for cookies.

I'd like to get rid of the 'Content' part of the names as content settings will
be called website settings in the future.

[marja, 2011/08/25 13:56:11]

Done.

+    : public NotificationObserver,
+      public base::RefCountedThreadSafe<CookieContentSettings>{
+ public:
+  CookieContentSettings(
+      HostContentSettingsMap* host_content_settings_map,
+      PrefService* prefs,
+      bool incognito);
+
+  bool Allow(const GURL& url,

[markusheintz_, 2011/08/24 12:29:40]

I guess you should add a comment here that describes what this method does.

[markusheintz_, 2011/08/24 12:29:40]

I just wonder whether this method should be called 'IsCookieAllowed'.

The reason why I don't like the name 'Allow' here is that is suggests that the
method takes some action to allow something. But it only returns a state.

[marja, 2011/08/25 13:56:11]

Done.

[marja, 2011/08/25 13:56:11]

Done.

+             const GURL& first_party_url,
+             bool setting_cookie) const;
+
+  bool EnforceSessionOnly(const GURL& url) const;

[markusheintz_, 2011/08/24 12:29:40]

Add a comment please.

[markusheintz_, 2011/08/24 12:29:40]

Analog to the proposed name of the method above this method could be alled
'IsCookieSessionOnly'.

The reason why I don't like the name 'EnforceSessionOnly' is that is suggest
that the method enforces something. But all the method does is getting a state.

[marja, 2011/08/25 13:56:11]

Done.

[marja, 2011/08/25 13:56:11]

Done.

+
+  // This setting trumps any host-specific settings.
+  bool BlockThirdPartyCookies() const { return block_third_party_cookies_; }
+  bool IsBlockThirdPartyCookiesManaged() const {
+    return is_block_third_party_cookies_managed_;
+  }
+
+  // Sets whether we block all third-party cookies. This method must not be
+  // invoked on an incognito map.
+  //
+  // This should only be called on the UI thread.
+  void SetBlockThirdPartyCookies(bool block);
+
+  // NotificationObserver implementation.
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
+  // Detaches the HostContentSettingsMap from all Profile-related objects like

[markusheintz_, 2011/08/24 12:29:40]

Please update the comment. s/HostContentSettingsMap/CookieContentSettings/g

[marja, 2011/08/25 13:56:11]

Done.

+  // PrefService. This methods needs to be called before destroying the Profile.
+  // Afterwards, none of the methods above that should only be called on the UI
+  // thread should be called anymore.
+  void ShutdownOnUIThread();
+
+ private:
+  // Various migration methods (old cookie, popup and per-host data gets
+  // migrated to the new format).
+  void MigrateObsoleteCookiePref();

[markusheintz_, 2011/08/24 12:29:40]

This method is around for quite a while. I guess we could get rid of it now.

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

Should I also remove const char kCookieBehavior[] from pref_names.cc, and
prefs->RegisterIntegerPref(prefs::kCookieBehavior, ...) from
host_content_settings_map.cc, or will it break if I remove those, too?

+
+  ContentSetting GetCookieContentSetting(const GURL& url,
+                                         const GURL& first_party_url,
+                                         bool setting_cookie) const;
+
+  scoped_refptr<HostContentSettingsMap> host_content_settings_map_;
+
+  // Weak; owned by the profile.
+  PrefService* prefs_;
+  PrefChangeRegistrar pref_change_registrar_;
+
+  bool is_off_the_record_;
+
   // Used around accesses to the following objects to guarantee thread safety.
   mutable base::Lock lock_;
 
   // Misc global settings.
   bool block_third_party_cookies_;
   bool is_block_third_party_cookies_managed_;
-
-  DISALLOW_COPY_AND_ASSIGN(HostContentSettingsMap);
 };
 
 #endif  // CHROME_BROWSER_CONTENT_SETTINGS_HOST_CONTENT_SETTINGS_MAP_H_