Adding CookieSettings for storing cookie content settings.

Adding CookieSettings for storing cookie content settings.

Moving cookie-spesific logic from HostContentSettingsMap to CookieSettings.
Making it explicit that the "is cookie session only" decision only depends on the
primary url.

Also removes the migration code for the obsolete kCookieBehavior preference.

BUG=93335
TEST=CookieSettings.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107705

[marja, 2011-10-25 12:20:24]

Hi,

This CL contains some parts from http://codereview.chromium.org/7713034 .

This CL adds the CookieSettings frontend which uses HostContentSettingsMap as a
backend. It contains the "block third party cookies" logic. This CL does not add
any new content types.

Cookie settings set by extensions are still stored in
ExtensionContentSettingsStore and they are not affected by this CL.

Does this approach look ok?

[Bernhard Bauer, 2011-10-25 13:11:20]

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.cc:53: static Factory*
GetInstance();
You're probably going to have to initialize the factory during startup (see
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...),
so you're going to have to make it publicy accessible.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.cc:141: DCHECK(setting ==
CONTENT_SETTING_ALLOW ||
Can you pull this out into a shared method?

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:5: // Maps hostnames to cookie
settings. Written on the UI thread and read on any
Nit: If this is a class-level comment, you should merge it with the one below.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:88: //
CONTENT_SETTING_SESSION_ONLY for (url, first_party_url) pairs where url
|first_party_url| does not exist here. Also, the comment is a bit redundant (see
below).

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:92: void
SetCookieSetting(const ContentSettingsPattern& primary_pattern,
We should not have two methods here.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:97: // first_party_url matches
|secondary_pattern|.
This comment is a bit redundant (you're describing how patterns work).

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:100: void
SetCookieSetting(const ContentSettingsPattern& primary_pattern,
Who calls this method, BTW?

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:108: void
ResetCookieSetting(const ContentSettingsPattern& primary_pattern);
We should not have two methods there either.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:124: // |Profile|. Afterwards,
none of the methods above that should only be called
I think we can simplify the comment to "Afterwards, only const methods can be
called".

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:135: // This setting trumps
any host-specific settings.
This comment isn't true anymore.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(left):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:704:
prefs->SetInteger(prefs::kCookieBehavior,
Can you update the CL description that this removes the migration code for
|kCookieBehavior|?

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.h:169: CookieSettings*
cookie_settings_;
Put it into a scoped_refptr?

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:723:
ContentSettingsPattern::FromString(pattern));
Is it actually necessary to go through CookieSettings here?

[Bernhard Bauer, 2011-10-25 13:14:23]

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: CookieSettings*
GetCookieSettings() const;
Where is this method actually called?

[marja, 2011-10-26 13:03:21]

Thanks for comments,

I also noticed some leftover code in HostContentSettingsMap
(SetBlockThirdPartyCookies, BlockThirdPartyCookies, Observe) and I remove that,
too.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/cookie_settings.cc (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.cc:53: static Factory*
GetInstance();
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> You're probably going to have to initialize the factory during startup (see
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...),
> so you're going to have to make it publicy accessible.

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.cc:141: DCHECK(setting ==
CONTENT_SETTING_ALLOW ||
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> Can you pull this out into a shared method?

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:5: // Maps hostnames to cookie
settings. Written on the UI thread and read on any
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> Nit: If this is a class-level comment, you should merge it with the one below.

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:88: //
CONTENT_SETTING_SESSION_ONLY for (url, first_party_url) pairs where url
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> |first_party_url| does not exist here. Also, the comment is a bit redundant
(see
> below).

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:92: void
SetCookieSetting(const ContentSettingsPattern& primary_pattern,
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> We should not have two methods here.

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:97: // first_party_url matches
|secondary_pattern|.
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> This comment is a bit redundant (you're describing how patterns work).

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:100: void
SetCookieSetting(const ContentSettingsPattern& primary_pattern,
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> Who calls this method, BTW?

Currently nobody was setting cookie settings where the secondary pattern would
be something else than wildcard, but do we want that limitation in
CookieSettings? (I removed the one-parameter version and made
ContentSettingsHandler and CookiesTreeModel call this one.)

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:108: void
ResetCookieSetting(const ContentSettingsPattern& primary_pattern);
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> We should not have two methods there either.

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:124: // |Profile|. Afterwards,
none of the methods above that should only be called
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> I think we can simplify the comment to "Afterwards, only const methods can be
> called".

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:135: // This setting trumps
any host-specific settings.
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> This comment isn't true anymore.

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(left):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:704:
prefs->SetInteger(prefs::kCookieBehavior,
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> Can you update the CL description that this removes the migration code for
> |kCookieBehavior|?

Did that, and removed the migration code, and the test forked from this one.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: CookieSettings*
GetCookieSettings() const;
On 2011/10/25 13:14:23, Bernhard Bauer wrote:
> Where is this method actually called?

This should've been called by ContentSettingsHandler; I fixed it so that it
really does call it.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.h:169: CookieSettings*
cookie_settings_;
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> Put it into a scoped_refptr?

Done.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:723:
ContentSettingsPattern::FromString(pattern));
On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> Is it actually necessary to go through CookieSettings here?

There are 2 approaches: 1) Everything goes via CookieSettings (except extensions
which write directly to ExtensionContentSettingsStore) 2) CookieSettings is just
an alternative frontend to HostContentSettingsMap. I tried to do 1), to be able
to change the representation format inside CookieSettings without causing
changes in other components. But it's not that clear which is better.

[Bernhard Bauer, 2011-10-26 13:24:09]

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
File chrome/browser/content_settings/cookie_settings.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/content_setti...
chrome/browser/content_settings/cookie_settings.h:100: void
SetCookieSetting(const ContentSettingsPattern& primary_pattern,
On 2011/10/26 13:03:21, marja wrote:
> On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> > Who calls this method, BTW?
> 
> Currently nobody was setting cookie settings where the secondary pattern would
> be something else than wildcard, but do we want that limitation in
> CookieSettings? (I removed the one-parameter version and made
> ContentSettingsHandler and CookiesTreeModel call this one.)

Yes, this is nicer than having overloaded methods, and one of them not even
called :)

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: CookieSettings*
GetCookieSettings() const;
On 2011/10/26 13:03:21, marja wrote:
> On 2011/10/25 13:14:23, Bernhard Bauer wrote:
> > Where is this method actually called?
> 
> This should've been called by ContentSettingsHandler; I fixed it so that it
> really does call it.

Does it? ContentSettingsHandler seems to get the CookieSettings from the Profile
(as it lives on the UI thread anyway). I think this method here is unnecessary.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:723:
ContentSettingsPattern::FromString(pattern));
On 2011/10/26 13:03:21, marja wrote:
> On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> > Is it actually necessary to go through CookieSettings here?
> 
> There are 2 approaches: 1) Everything goes via CookieSettings (except
extensions
> which write directly to ExtensionContentSettingsStore) 2) CookieSettings is
just
> an alternative frontend to HostContentSettingsMap. I tried to do 1), to be
able
> to change the representation format inside CookieSettings without causing
> changes in other components. But it's not that clear which is better.

I think it's fine if code that is not specific to cookies goes directly through
the HostContentSettingsMap. 

If we really want to do 1) we should make sure that no one accesses the
HostContentSettingsMap directly (but then we'd need to duplicate the code that
finds the correct map at all call sites, which is what you're starting to do
here. I just think that's unnecessary).

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

http://codereview.chromium.org/8383004/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:143: DCHECK(setting ==
CONTENT_SETTING_ALLOW ||
Now you could also pull this out into a method |IsValidSetting| or so :)

http://codereview.chromium.org/8383004/diff/11001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/8383004/diff/11001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_dependency_manager.cc:51:
CookieSettings::Factory::GetInstance();
Nit: It looks like this was alphabetized at some point, then
ExtensionSpeechInputManager was added at the end. Could you re-sort?

[marja, 2011-10-27 08:48:51]

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: CookieSettings*
GetCookieSettings() const;
On 2011/10/26 13:24:09, Bernhard Bauer wrote:
> On 2011/10/26 13:03:21, marja wrote:
> > On 2011/10/25 13:14:23, Bernhard Bauer wrote:
> > > Where is this method actually called?
> > 
> > This should've been called by ContentSettingsHandler; I fixed it so that it
> > really does call it.
> 
> Does it? ContentSettingsHandler seems to get the CookieSettings from the
Profile
> (as it lives on the UI thread anyway). I think this method here is
unnecessary.

Oops, not ContentSettingsHandler. This one is called by
ChromeWorkerMessageFilter, and it used to get the HostContentSettingsMap the
same way when it used it.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:723:
ContentSettingsPattern::FromString(pattern));
On 2011/10/26 13:24:09, Bernhard Bauer wrote:
> On 2011/10/26 13:03:21, marja wrote:
> > On 2011/10/25 13:11:21, Bernhard Bauer wrote:
> > > Is it actually necessary to go through CookieSettings here?
> > 
> > There are 2 approaches: 1) Everything goes via CookieSettings (except
> extensions
> > which write directly to ExtensionContentSettingsStore) 2) CookieSettings is
> just
> > an alternative frontend to HostContentSettingsMap. I tried to do 1), to be
> able
> > to change the representation format inside CookieSettings without causing
> > changes in other components. But it's not that clear which is better.
> 
> I think it's fine if code that is not specific to cookies goes directly
through
> the HostContentSettingsMap. 
> 
> If we really want to do 1) we should make sure that no one accesses the
> HostContentSettingsMap directly (but then we'd need to duplicate the code that
> finds the correct map at all call sites, which is what you're starting to do
> here. I just think that's unnecessary).

Ok, I removed the CookieSettings usage here.

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

http://codereview.chromium.org/8383004/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/cookie_settings.cc:143: DCHECK(setting ==
CONTENT_SETTING_ALLOW ||
On 2011/10/26 13:24:09, Bernhard Bauer wrote:
> Now you could also pull this out into a method |IsValidSetting| or so :)

Done.

http://codereview.chromium.org/8383004/diff/11001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/8383004/diff/11001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_dependency_manager.cc:51:
CookieSettings::Factory::GetInstance();
On 2011/10/26 13:24:09, Bernhard Bauer wrote:
> Nit: It looks like this was alphabetized at some point, then
> ExtensionSpeechInputManager was added at the end. Could you re-sort?

Done.

[Bernhard Bauer, 2011-10-27 09:27:04]

LGTM.

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/8383004/diff/5101/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.h:105: CookieSettings*
GetCookieSettings() const;
On 2011/10/27 08:48:51, marja wrote:
> On 2011/10/26 13:24:09, Bernhard Bauer wrote:
> > On 2011/10/26 13:03:21, marja wrote:
> > > On 2011/10/25 13:14:23, Bernhard Bauer wrote:
> > > > Where is this method actually called?
> > > 
> > > This should've been called by ContentSettingsHandler; I fixed it so that
it
> > > really does call it.
> > 
> > Does it? ContentSettingsHandler seems to get the CookieSettings from the
> Profile
> > (as it lives on the UI thread anyway). I think this method here is
> unnecessary.
> 
> Oops, not ContentSettingsHandler. This one is called by
> ChromeWorkerMessageFilter, and it used to get the HostContentSettingsMap the
> same way when it used it.

Ah, OK then.

[commit-bot: I haz the power, 2011-10-27 09:30:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8383004/20038

[commit-bot: I haz the power, 2011-10-27 09:30:43]

Presubmit check for 8383004-20038 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile_io_data.h,chrome/browser/profiles/profile_impl.cc,chrome/browser/net/cookie_policy_browsertest.cc,chrome/browser/profiles/profile_io_data.cc,chrome/browser/profiles/profile_dependency_manager.cc

Presubmit checks took 3.9s to calculate.

[marja, 2011-10-27 09:39:27]

Oops, OWNERS check.

{mirandac,battre}: Could you review the following parts:

mirandac:
(Changes: In addition to the HostContentSettingsMap, we now have CookieSettings.
It is accessible via ProfileIOData.) 

battre:
chrome/browser/net/cookie_policy_browsertest.cc
(HostContentSettingsMap::SetBlockThirdPartyCookies is gone, made the test set
the preference directly.)

[marja, 2011-10-27 09:40:01]

On 2011/10/27 09:39:27, marja wrote:
> Oops, OWNERS check.
> 
> {mirandac,battre}: Could you review the following parts:
> 
> mirandac:

Oops2, this should've read:

chrome/browser/profiles/profile_io_data.h
chrome/browser/profiles/profile_impl.cc
chrome/browser/profiles/profile_io_data.cc
chrome/browser/profiles/profile_dependency_manager.cc

[battre, 2011-10-27 13:23:46]

LGTM @ chrome/browser/net/cookie_policy_browsertest.cc

[Miranda Callahan, 2011-10-27 14:57:46]

On 2011/10/27 13:23:46, battre wrote:
> LGTM @ chrome/browser/net/cookie_policy_browsertest.cc

LGTM for profiles/*

[commit-bot: I haz the power, 2011-10-28 06:52:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8383004/20038

[commit-bot: I haz the power, 2011-10-28 06:53:00]

Can't apply patch for file
chrome/browser/content_settings/tab_specific_content_settings.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/content_settings/tab_specific_content_settings.cc
Hunk #1 FAILED at 488.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/content_settings/tab_specific_content_settings.cc.rej

[commit-bot: I haz the power, 2011-10-28 08:21:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8383004/28001

[commit-bot: I haz the power, 2011-10-28 08:52:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8383004/31001

[commit-bot: I haz the power, 2011-10-28 09:49:08]

Change committed as 107705