HostContentSettingsMap refactoring.

Modifications to cookie settings.

Adding CookieSettings and moving the cookie logic away from
HostContentSettingsMap. Making it explicit that the "is
cookie session only" decision does not depend on the first
party origin.


BUG=93335
TEST=CookieSettings.*

[marja, 2011-08-23 09:14:08]

Hi Markus,

Here's the cl we talked about yesterday. It's... messy. I'm not at all sure if
this is the right thing :)

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

Here are some first comments.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:428:
CookieContentSettings::CookieContentSettings(
Please move the CookieContentSettings to a separate file:
cookie_content_settings.cc

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:212: //
FIXME(marja): This should be moved into a separate file.
Please move to a new file: cookie_settings.h

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:213: class
CookieContentSettings
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.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:222: bool
Allow(const GURL& url,
I guess you should add a comment here that describes what this method does.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:222: bool
Allow(const GURL& url,
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.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:226: bool
EnforceSessionOnly(const GURL& url) const;
Add a comment please.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:226: bool
EnforceSessionOnly(const GURL& url) const;
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.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:245: // Detaches the
HostContentSettingsMap from all Profile-related objects like
Please update the comment. s/HostContentSettingsMap/CookieContentSettings/g

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:254: void
MigrateObsoleteCookiePref();
This method is around for quite a while. I guess we could get rid of it now.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:1021: // Create CookieContentSettings,
too, since it takes care of migrating
Are you sure about this? If you only read cookie settings through the
CookieSettings class then it should not matter. But I guess we can remove this
migration code soon anyway.

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

Marking some changes done.

Adding the new content setting type (patch set 3) turned out messier than I
would've expected. The writing API of ContentSettings is kinda nice and clean,
but the additional content setting type breaks lots of things... and still, I'm
not sure whether the illegal content settings are now really prevented or not.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:428:
CookieContentSettings::CookieContentSettings(
On 2011/08/24 12:29:40, markusheintz_ wrote:
> Please move the CookieContentSettings to a separate file:
> cookie_content_settings.cc

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:212: //
FIXME(marja): This should be moved into a separate file.
On 2011/08/24 12:29:40, markusheintz_ wrote:
> Please move to a new file: cookie_settings.h

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:213: class
CookieContentSettings
On 2011/08/24 12:29:40, markusheintz_ wrote:
> 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.

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:222: bool
Allow(const GURL& url,
On 2011/08/24 12:29:40, markusheintz_ wrote:
> 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.

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:222: bool
Allow(const GURL& url,
On 2011/08/24 12:29:40, markusheintz_ wrote:
> I guess you should add a comment here that describes what this method does.

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:226: bool
EnforceSessionOnly(const GURL& url) const;
On 2011/08/24 12:29:40, markusheintz_ wrote:
> Add a comment please.

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:226: bool
EnforceSessionOnly(const GURL& url) const;
On 2011/08/24 12:29:40, markusheintz_ wrote:
> 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.

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:245: // Detaches the
HostContentSettingsMap from all Profile-related objects like
On 2011/08/24 12:29:40, markusheintz_ wrote:
> Please update the comment. s/HostContentSettingsMap/CookieContentSettings/g

Done.

http://codereview.chromium.org/7713034/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:1021: // Create CookieContentSettings,
too, since it takes care of migrating
On 2011/08/24 12:29:40, markusheintz_ wrote:
> Are you sure about this? If you only read cookie settings through the
> CookieSettings class then it should not matter. But I guess we can remove this
> migration code soon anyway.

There was a unit test depending on this, and only using HostContentSettingsMap
(never creating CookieSettings). I modified the test to use CookieSettings.
There is no real problem afaik.

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:20: // cookie-specific logic
such as blocking third-party cookies.
You should add some comments about thread safety of this class, i.e. which
method can be called on which thread.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
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.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:36: bool IsCookieAllowed(const
GURL& url,
I think splitting this method up into two methods for reading and writing
cookies might be nicer.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:67: bool
IsBlockThirdPartyCookiesManaged() const {
Is this method called anywhere?

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:75: void
SetBlockThirdPartyCookies(bool block);
In the interest of simplicity, we could remove this method and make clients
directly use PrefService (same for |BlockThirdPartyCookies|).

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:112: // Misc global settings.
Nit: This comment could be updated.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:272:
CookieSettings* HostContentSettingsMap::GetCookieSettings() {
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.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:149:
setting_cookie)) {
Nit: indent

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:65:
{CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY, "cookies-session-only"},
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".

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:729:
ContentSettingsPattern::FromString(pattern), session_only);
Maybe we could add a convenience method for this to CookieSettings?

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:52:
CONTENT_SETTINGS_TYPE_COOKIES,
You also need to add new policies and new preferences to backup the policies.
But I guess we should do that in a separate CL.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:43: // TODO(markusheintz):
Discuss whether it is sensible to move migration code
I think this TODO is not needed anymore so if you remove it you get the credit
for fixing it :). Thanks

I may want to think about removing the migration code anyway.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> 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.

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.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:272:
CookieSettings* HostContentSettingsMap::GetCookieSettings() {
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> 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.

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-08-30 10:10:38]

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:65:
{CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY, "cookies-session-only"},
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> 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".

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)?

[markusheintz_, 2011-08-30 10:22:32]

On 2011/08/30 10:10:38, marja wrote:
>
http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
> File chrome/browser/ui/webui/options/content_settings_handler.cc (right):
> 
>
http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
> chrome/browser/ui/webui/options/content_settings_handler.cc:65:
> {CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY, "cookies-session-only"},
> On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > 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".
> 
> 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)?

I'm think keeping a separate list of content types that are displayed in the UI
in the UI code is probably the right thing to do. This way the UI does not
depend on any order of content types in the content_settings_types enum, and the
any other backend code does not need to know what is displayed.

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:65:
{CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY, "cookies-session-only"},
On 2011/08/30 10:10:38, marja wrote:
> On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > 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".
> 
> 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)?

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]

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:65:
{CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY, "cookies-session-only"},
On 2011/08/30 11:03:12, Bernhard Bauer wrote:
> On 2011/08/30 10:10:38, marja wrote:
> > On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > > 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".
> > 
> > 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)?
> 
> 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.

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

[marja, 2011-08-30 13:14:07]

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> 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.

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]

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
On 2011/08/30 13:14:08, marja wrote:
> On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > 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.
> 
> 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...

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]

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7713034/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:254: void
MigrateObsoleteCookiePref();
On 2011/08/24 12:29:40, markusheintz_ wrote:
> This method is around for quite a while. I guess we could get rid of it now.

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?

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:43: // TODO(markusheintz):
Discuss whether it is sensible to move migration code
On 2011/08/29 08:47:09, markusheintz_ wrote:
> I think this TODO is not needed anymore so if you remove it you get the credit
> for fixing it :). Thanks
> 
> I may want to think about removing the migration code anyway.

Migration code removed.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:20: // cookie-specific logic
such as blocking third-party cookies.
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> You should add some comments about thread safety of this class, i.e. which
> method can be called on which thread.

Done.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
On 2011/08/30 13:20:25, Bernhard Bauer wrote:
> On 2011/08/30 13:14:08, marja wrote:
> > On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > > 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.
> > 
> > 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...
> 
> 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.

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).

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:36: bool IsCookieAllowed(const
GURL& url,
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> I think splitting this method up into two methods for reading and writing
> cookies might be nicer.

Done.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:67: bool
IsBlockThirdPartyCookiesManaged() const {
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> Is this method called anywhere?

No; I removed it.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:75: void
SetBlockThirdPartyCookies(bool block);
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> In the interest of simplicity, we could remove this method and make clients
> directly use PrefService (same for |BlockThirdPartyCookies|).

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:112: // Misc global settings.
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> Nit: This comment could be updated.

Done.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:272:
CookieSettings* HostContentSettingsMap::GetCookieSettings() {
On 2011/08/29 08:47:09, markusheintz_ wrote:
> On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > 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.
> 
> 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|.

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:149:
setting_cookie)) {
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> Nit: indent

Done.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:729:
ContentSettingsPattern::FromString(pattern), session_only);
On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> Maybe we could add a convenience method for this to CookieSettings?

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

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
On 2011/09/01 11:03:19, marja wrote:
> On 2011/08/30 13:20:25, Bernhard Bauer wrote:
> > On 2011/08/30 13:14:08, marja wrote:
> > > On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > > > 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.
> > > 
> > > 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...
> > 
> > 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.
> 
> 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).

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.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:75: void
SetBlockThirdPartyCookies(bool block);
On 2011/09/01 11:03:19, marja wrote:
> On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > In the interest of simplicity, we could remove this method and make clients
> > directly use PrefService (same for |BlockThirdPartyCookies|).
> 
> This CL is getting bigger than expected; can I leave that out?

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:729:
ContentSettingsPattern::FromString(pattern), session_only);
On 2011/09/01 11:03:19, marja wrote:
> On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > Maybe we could add a convenience method for this to CookieSettings?
> 
> Now there is CookieSettings::SetCookieSetting, which makes this a lot simpler.

Nice, thank you!

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:79: // FIXME(marja): Add
another content type here
Is this comment still up to date?

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:80: ContentSetting setting =
GetCookieContentSetting(
I think you could directly consult the HCSM here. Then, you can remove the
|content_type| parameter from GetCookieContentSetting.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:84: bool
BlockThirdPartyCookies() const { return block_third_party_cookies_; }
Nit: We could call this method |ShouldBlockThirdPartyCookies| or something, to
make it not appear as an action.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:101: void Shutdown();
This is a ProfileKeyedService method, right? In that case you should add a
comment, and mark it as virtual.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:111: // Weak pointer to the
data backend (which also owns this object).
HostContentSettingsMap doesn't own this object anymore, so can you update the
comment? And maybe keep a scoped_refptr to it?

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings_factory.h (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings_factory.h:21: // Returns the
CookieSettings that provides desktop notifications
"desktop notifications"? :-)

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings_factory.h:23: static
CookieSettings* GetForProfile(Profile* profile);
Nit: |CookieSettingsFactory::GetForProfile| sounds to me like it would return a
|CookieSettingsFactory|. Maybe call it |GetCookieSettingsForProfile|? You could
also make it non-static and add a method |CookieSettings::GetForProfile| that
calls it. That way, clients don't need to know about |CookieSettingsFactory|.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:681:
DCHECK(cookie_settings != NULL);
I think we should be a bit more defensive here, as |mode| is coming from a
renderer, and we don't want a compromised renderer to be able to crash the
browser. Maybe NOTREACHED() and return if it's NULL?

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

Thanks for comments, here are some more changes.

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:28: bool incognito);
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> On 2011/09/01 11:03:19, marja wrote:
> > On 2011/08/30 13:20:25, Bernhard Bauer wrote:
> > > On 2011/08/30 13:14:08, marja wrote:
> > > > On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > > > > 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.
> > > > 
> > > > 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...
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

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

http://codereview.chromium.org/7713034/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:75: void
SetBlockThirdPartyCookies(bool block);
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> On 2011/09/01 11:03:19, marja wrote:
> > On 2011/08/28 19:18:49, Bernhard Bauer wrote:
> > > In the interest of simplicity, we could remove this method and make
clients
> > > directly use PrefService (same for |BlockThirdPartyCookies|).
> > 
> > This CL is getting bigger than expected; can I leave that out?
> 
> Ok, can you add a TODO for me (or yourself ;-) ) to remove it?

Done.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:79: // FIXME(marja): Add
another content type here
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> Is this comment still up to date?

Removed.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:80: ContentSetting setting =
GetCookieContentSetting(
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> I think you could directly consult the HCSM here. Then, you can remove the
> |content_type| parameter from GetCookieContentSetting.

Hmm, not sure if I understand. Do you mean duplicating the ShouldAllowAllContent
+ GetNonDefaultContentSetting + GetDefaultContentSetting calls here?

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:84: bool
BlockThirdPartyCookies() const { return block_third_party_cookies_; }
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> Nit: We could call this method |ShouldBlockThirdPartyCookies| or something, to
> make it not appear as an action.

I renamed this, and left SetBlockThirdPartyCookies as is, since
BlockThirdPartyCookies(false) didn't make much sense.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:101: void Shutdown();
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> This is a ProfileKeyedService method, right? In that case you should add a
> comment, and mark it as virtual.

It was, but is not any more. I changed it back to ShutDownOnUIThread. The
Shutdown in CookieSettingsWrapper is now marked as virtual and OVERRIDE.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:111: // Weak pointer to the
data backend (which also owns this object).
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> HostContentSettingsMap doesn't own this object anymore, so can you update the
> comment? And maybe keep a scoped_refptr to it?

Done.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings_factory.h (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings_factory.h:21: // Returns the
CookieSettings that provides desktop notifications
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> "desktop notifications"? :-)

Oops :) Luckily, this file just got deleted.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings_factory.h:23: static
CookieSettings* GetForProfile(Profile* profile);
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> Nit: |CookieSettingsFactory::GetForProfile| sounds to me like it would return
a
> |CookieSettingsFactory|. Maybe call it |GetCookieSettingsForProfile|? You
could
> also make it non-static and add a method |CookieSettings::GetForProfile| that
> calls it. That way, clients don't need to know about |CookieSettingsFactory|.

This happened as a side product of the CookieSettingsWrapper addition. Now the
CookieSettings are retrieved with CookieSettings::GetForProfile.

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:681:
DCHECK(cookie_settings != NULL);
On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> I think we should be a bit more defensive here, as |mode| is coming from a
> renderer, and we don't want a compromised renderer to be able to crash the
> browser. Maybe NOTREACHED() and return if it's NULL?

Crash here would mean that GetContentSettingsMap returned non-NULL but
GetCookieSettings returned NULL, or GetOTRContentSettingsMap returned non-NULL
but GetOTRCookieSettings returned NULL, and that shouldn't happen. But I did
like you said, so, NOTREACHED + return.

[Bernhard Bauer, 2011-09-01 13:58:49]

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/26002/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:80: ContentSetting setting =
GetCookieContentSetting(
On 2011/09/01 13:34:48, marja wrote:
> On 2011/09/01 11:41:54, Bernhard Bauer wrote:
> > I think you could directly consult the HCSM here. Then, you can remove the
> > |content_type| parameter from GetCookieContentSetting.
> 
> Hmm, not sure if I understand. Do you mean duplicating the
ShouldAllowAllContent
> + GetNonDefaultContentSetting + GetDefaultContentSetting calls here?
> 

No, HostContentSettingsMap has a GetContentSetting method you could use which
does exactly that (CONTENT_SETTINGS_TYPE_COOKIES is the only type which has
special behavior here, to  block third party cookies).

http://codereview.chromium.org/7713034/diff/29036/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/29036/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:114: class Factory;
You should initialize the factory in profile_dependency_manager.cc, which might
require you to make it public (or to add a static initialization method here). I
defer to Elliot on where the best place for the factory is.

[marja, 2011-09-01 15:17:21]

Markus & Bernhard,

How about *not* fixing the kContentSettingsTypeGroupNames things as a part of
this changelist? I'm a bit afraid this refactoring is soon getting out of
hands... WDYT?

[Bernhard Bauer, 2011-09-05 11:05:37]

I started working on the TODO in this CL and came across a couple of things :)

BTW, I think fixing the TODO in this CL instead of a separate one wouldn't make
this one more complicated. It's basically touching only places you're already
touching in this one. If you want, I can send you my patch and you can patch it
into here.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:168: void
CookieSettings::SetDefaultSetting(ContentSetting setting) {
I think you could pull out methods here that return the correct internal setting
for cookies and session_only, and use that here and below in |SetCookieSetting|.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:172: ContentSetting allowed =
CONTENT_SETTING_BLOCK;
Nit: this line reads very confusing at first. Could you call the variable
something like |cookies_allowed|?

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:92: return
block_third_party_cookies_;
This isn't thread safe. You should wrap all accesses to
|block_third_party_cookies_| in a base::AutoLock.

Also, please move the implementation to the .cc file.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:153: if
(cookie_settings->IsCookieSessionOnly(primary_url))
For the content settings extension API we can expose the "raw" setting (i.e.
using the separate content type).

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:262: if
(cookie_settings->IsSettingCookieAllowed(url, url)) {
You could add a method GetCookieSetting to CookieSettings which does this.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:376: }
You're probably going to need to add a case for CONTENT_SETTINGS_TYPE_COOKIES
here.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:432:
UpdateExceptionsViewFromHostContentSettingsMap(type);
Same here.

http://codereview.chromium.org/7713034/diff/27074/chrome/common/content_setti...
File chrome/common/content_settings_types.h (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/common/content_setti...
chrome/common/content_settings_types.h:24:
CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY,
In the spirit of keeping content settings consistent, we could call the setting
that governs whether a cookie is session-only "COOKIES_PERSISTENT" or something,
where a setting of BLOCK means it's session-only, and ALLOW means the default
behavior.

[marja, 2011-09-06 08:30:15]

Thanks for comments again.

In addition to the changes here, I renamed CookieSettings::(G|S)etDefaultSetting
to CookieSettings::(G|S)etDefaultCookieSettings to be more consistent with the
other funcs, like (Reset|Set)CookieSetting.

Did we agree that I won't add private functions to HostContentSettingsMap to
ensure that only CookieSettings can set the cookie settings? I recall we
discussed it, and this is probably what we concluded, but I don't really
remember.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:168: void
CookieSettings::SetDefaultSetting(ContentSetting setting) {
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> I think you could pull out methods here that return the correct internal
setting
> for cookies and session_only, and use that here and below in
|SetCookieSetting|.

Done.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:172: ContentSetting allowed =
CONTENT_SETTING_BLOCK;
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> Nit: this line reads very confusing at first. Could you call the variable
> something like |cookies_allowed|?

Done.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:92: return
block_third_party_cookies_;
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> This isn't thread safe. You should wrap all accesses to
> |block_third_party_cookies_| in a base::AutoLock.
> 
> Also, please move the implementation to the .cc file.

Done.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_api.cc:153: if
(cookie_settings->IsCookieSessionOnly(primary_url))
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> For the content settings extension API we can expose the "raw" setting (i.e.
> using the separate content type).

Do you mean here we'd just return the raw setting from HostContentSettingsMap,
and e.g., not apply third-party cookie blocking rules?

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:262: if
(cookie_settings->IsSettingCookieAllowed(url, url)) {
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> You could add a method GetCookieSetting to CookieSettings which does this.

Hmm, it's like exposing the private CookieSettings::GetCookieContentSetting...
I'm not sure whether it's a good thing.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:376: }
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> You're probably going to need to add a case for CONTENT_SETTINGS_TYPE_COOKIES
> here.

Done.

http://codereview.chromium.org/7713034/diff/27074/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:432:
UpdateExceptionsViewFromHostContentSettingsMap(type);
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> Same here.

Done; for this I added CookieSettings::GetCookieSettings.

http://codereview.chromium.org/7713034/diff/27074/chrome/common/content_setti...
File chrome/common/content_settings_types.h (right):

http://codereview.chromium.org/7713034/diff/27074/chrome/common/content_setti...
chrome/common/content_settings_types.h:24:
CONTENT_SETTINGS_TYPE_COOKIES_SESSION_ONLY,
On 2011/09/05 11:05:38, Bernhard Bauer wrote:
> In the spirit of keeping content settings consistent, we could call the
setting
> that governs whether a cookie is session-only "COOKIES_PERSISTENT" or
something,
> where a setting of BLOCK means it's session-only, and ALLOW means the default
> behavior.

Done. (CONTENT_SETTINGS_TYPE_PERSISTENT_COOKIES.)

[Bernhard Bauer, 2011-09-14 16:01:35]

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:61:
prefs::kManagedCookiesSessionOnlyForUrls,
Wait, this doesn't seem quite right. Some of the prefs are duplicate, and the
mapping to content settings looks wrong as well.

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:93: //
PrefoileKeyedServiceFactory methods:
Nit: ProfileKeyedServiceFactory

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:194: // cookie_settings and
permanent_cookie_settings will contain the same
Nit: Please add bars around |cookie_settings| and |permanent_cookie_settings|.

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:202:
DCHECK(cookie_settings.size() == permanent_cookie_settings.size());
If you use DCHECK_EQ(expected, actual) you get nicer error messages.

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.h:101: const
NotificationDetails& details);
Could you add OVERRIDE here?

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/7713034/diff/40001/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:263: if
(cookie_settings->IsSettingCookieAllowed(url, url)) {
I think you could get this setting via
|cookie_settings->GetCookieContentSetting| as well.

[marja, 2011-09-20 08:24:02]

Hi and thanks for comments,

There is one part of this CL which is getting uglier than I would've expected.
It's the conversion between the 2 systems of cookie settings: 1)
CONTENT_SETTINGS_TYPE_COOKIES that can be either ALLOW, SESSION_ONLY or BLOCK,
and 2) CONTENT_SETTINGS_TYPE_COOKIES that can be ALLOW or BLOCK and
CONTENT_SETTINGS_TYPE_PERMANENT_COOKIES that can be ALLOW or BLOCK. It's
especially messy since CONTENT_SETTINGS_TYPE_COOKIES is used in both places, and
in some places it's allowed to have the value SESSION_ONLY and in some places
not. I think this CL doesn't make sense unless I fix that (e.g., by adding a
third content setting type).

[Bernhard Bauer, 2011-09-20 08:28:22]

On 2011/09/20 08:24:02, marja wrote:
> Hi and thanks for comments,
> 
> There is one part of this CL which is getting uglier than I would've expected.
> It's the conversion between the 2 systems of cookie settings: 1)
> CONTENT_SETTINGS_TYPE_COOKIES that can be either ALLOW, SESSION_ONLY or BLOCK,
> and 2) CONTENT_SETTINGS_TYPE_COOKIES that can be ALLOW or BLOCK and
> CONTENT_SETTINGS_TYPE_PERMANENT_COOKIES that can be ALLOW or BLOCK. It's
> especially messy since CONTENT_SETTINGS_TYPE_COOKIES is used in both places,
and
> in some places it's allowed to have the value SESSION_ONLY and in some places
> not. I think this CL doesn't make sense unless I fix that (e.g., by adding a
> third content setting type).

Makes sense. Maybe we could add a content type COOKIES_INTERNAL (and rename
PERMANENT so that it also contains INTERNAL, if it doesn't make the type too
long).

[marja, 2011-10-28 10:38:04]

The good parts of this CL got in, see: http://codereview.chromium.org/8383004 .

I'd rather forget the bad and ugly parts, so, closing this codereview.