HostContentSettingsMap refactoring.

chrome/browser/content_settings/cookie_settings.h

+// Copyright (c) 2011 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.
+
+#ifndef CHROME_BROWSER_CONTENT_SETTINGS_COOKIE_SETTINGS_H_
+#define CHROME_BROWSER_CONTENT_SETTINGS_COOKIE_SETTINGS_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "chrome/browser/prefs/pref_change_registrar.h"
+#include "chrome/common/content_settings.h"
+#include "content/common/notification_observer.h"
+
+class ContentSettingsPattern;
+class GURL;
+class HostContentSettingsMap;
+
+// A frontend to the cookie-specific settings of HostContentSettingsMap. Handles
+// cookie-specific logic such as blocking third-party cookies.

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

You should add some comments about thread safety of this class, i.e. which
method can be called on which thread.

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

Done.

+class CookieSettings
+    : public NotificationObserver,
+      public base::RefCountedThreadSafe<CookieSettings> {
+ public:
+  CookieSettings(
+      HostContentSettingsMap* host_content_settings_map,
+      PrefService* prefs,
+      bool incognito);

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

These are all attributes of the Profile, so we could just initialize it with a
Profile? Going a bit further, we might actually make this a ProfileKeyedService
instead of being owned by the HostContentSettingsMap.

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

If we do not pass in the profile but the stuff that is actually needed, we see
immediately the dependencies of this class. I think that is much more straight
forward unless you plane to use other parts of the profile as well.

[marja, 2011/08/30 13:14:08]

I bumped into a problem when trying to make it a ProfileKeyedService:
ChromeContentBrowserClient::AllowAppcache (et.al.) used to get a ProfileIOData*
from ResourceContext::GetUserData(), but I don't see where they could get a
Profile* from...

[Bernhard Bauer, 2011/08/30 13:20:25]

Right (also, I guess you're on the IO thread there, and
ProfileKeyedServiceFactory is not thread safe). For the HostContentSettingsMap,
we solve this by having ProfileIOData keep a reference to it as well. We could
do the same thing for CookieSettings.

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

Ok, I made it a ProfileKeyedService, and made ProfileIOData hold a raw pointer
to CookieSettings (tried scoped_refptr first, but that failed because
ProfileKeyedServiceFactory has the ownership and just deletes the service at
some point).

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

OK… but what happens after the profile is shutdown? We wouldn't want to have a
stale pointer then.

Another pattern which I have used is to not have ProfileKeyedService be
implemented by CookieSettings directly, but by a wrapper class which keeps a
scoped_refptr<CookieSettings>. That way, the reference is dropped when the
profile is destroyed, but other objects (like ProfileIOData) can keep their
references; see plugin_prefs.cc.

[marja, 2011/09/01 13:34:48]

Thanks for the hint. I did that, and changed raw pointers to  scoped_refptr.

+
+  // Returns the default content setting (CONTENT_SETTING_ALLOW,
+  // CONTENT_SETTING_BLOCK, or CONTENT_SETTING_SESSION_ONLY) for cookies.
+  ContentSetting GetDefaultSetting() const;
+
+  // Returns true if the page identified by (url, first_party_url) is allowed to
+  // set a cookie (permanent or session only).
+  bool IsCookieAllowed(const GURL& url,

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

I think splitting this method up into two methods for reading and writing
cookies might be nicer.

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

Done.

+                       const GURL& first_party_url,
+                       bool setting_cookie) const;
+
+  // Returns true if the cookie set by a page identified by url should be
+  // session only. Querying this only makes sense if IsCookieAllowed has
+  // returned true.
+  bool IsCookieSessionOnly(const GURL& url) const;
+
+  // Sets the default content setting (CONTENT_SETTING_ALLOW,
+  // CONTENT_SETTING_BLOCK, or CONTENT_SETTING_SESSION_ONLY) for cookies.
+  void SetDefaultSetting(ContentSetting setting);
+
+  // Sets cookies allowed for (url, first_party_url) patterns matching
+  // (primary_pattern, secondary_pattern). The cookies can be permanent or
+  // session-only.
+  void SetCookieAllowed(const ContentSettingsPattern& primary_pattern,
+                        const ContentSettingsPattern& secondary_pattern,
+                        bool allowed);
+
+  // Sets whether an origin matching primary pattern can set session-only
+  // cookies or permanent cookies.
+  void SetCookieSessionOnly(const ContentSettingsPattern& primary_pattern,
+                            bool session_only);
+
+  void ResetCookieAllowed(const ContentSettingsPattern& primary_pattern,
+                          const ContentSettingsPattern& secondary_pattern);
+  void ResetCookieSessionOnly(const ContentSettingsPattern& primary_pattern);
+
+  // This setting trumps any host-specific settings.
+  bool BlockThirdPartyCookies() const { return block_third_party_cookies_; }
+  bool IsBlockThirdPartyCookiesManaged() const {

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

Is this method called anywhere?

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

No; I removed it.

+    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);

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

In the interest of simplicity, we could remove this method and make clients
directly use PrefService (same for |BlockThirdPartyCookies|).

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

This CL is getting bigger than expected; can I leave that out?

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

Ok, can you add a TODO for me (or yourself ;-) ) to remove it?

[marja, 2011/09/01 13:34:48]

Done.

+
+  // NotificationObserver implementation.
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
+  // Detaches the CookieSettings from all Profile-related objects like
+  // 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();
+
+  // A helper for retrieving data for CONTENT_TYPE_COOKIES and
+  // CONTENT_TYPE_COOKIES_SESSION_ONLY.
+  ContentSetting GetCookieContentSetting(const GURL& url,
+                                         const GURL& first_party_url,
+                                         ContentSettingsType content_type,
+                                         bool setting_cookie) const;
+
+  // Weak pointer to the data backend (which also owns this object).
+  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.

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

Nit: This comment could be updated.

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

Done.

+  bool block_third_party_cookies_;
+  bool is_block_third_party_cookies_managed_;
+};
+
+#endif  // CHROME_BROWSER_CONTENT_SETTINGS_COOKIE_SETTINGS_H_