Replace SetContentSetting method of the content_settings::Provider interface with GetWebsiteSetting

Replace SetContentSetting method of the content_settings::Provider interface with GetWebsiteSetting.

This is part of a series of cleanup CLs to make all content settings Providers and the HostContentSetting map use only |Values| instead of |ContentSettings|. The HostContentSettingsMap should only contain conveniences methods that take |ContentSetting| parameters.

BUG=102637
TEST=HostContentSettingsMapTest*,
     PrefProviderTest*,
     PolicyProviderTest*,
     DefaultProviderTest*,
     ExtensionProviderTest*

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

[markusheintz_, 2011-11-11 12:22:42]

Please review this CL. Thanks.

[Bernhard Bauer, 2011-11-11 14:00:40]

I have a bunch of comments, the one in host_content_settings_map.cc:262 is the
most important one, though.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:73: //
TODO(markusheintz): |ContentSetting| should be replace with a |Value|.
Nit: "replaced".

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:115:
default_settings_[CONTENT_SETTINGS_TYPE_COOKIES]->GetAsInteger(&int_value);
Can you use the ValueToContentSetting helper method here? That would also be
able to deal with edge cases like if there's no value for a content type.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:168:
Value::CreateIntegerValue(kDefaultSettings[content_type]));
If you get the integer value from |value| you can avoid allocating a new value
here.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:209: int
int_value;
Initialize this value please.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:300: for
(size_t type = 0; type < CONTENT_SETTINGS_NUM_TYPES; ++type) {
Can you look up the |ContentSettingsType| for |content_type| (I know, confusing
names) directly with a helper method instead of this for loop?

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:306:
default_settings_[ContentSettingsType(type)].reset(value->DeepCopy());
If we don't find a value, the |DeepCopy| will crash in a Release build. If you
want to avoid that, you could keep the old version and just change this line to
create an IntegerValue from the |setting|.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:312: //
Migrate obsolete cookie prompt mode/
Nit: Slash at the end of the comment

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:313: if
(ValueEqualsContentSetting(
Oh, we have that method? Then you could use it in line 168 as well.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_default_provider_unittest.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider_unittest.cc:37:
scoped_ptr<base::Value> value_allow;
Class member names should end with an underscore.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:522: if
(!found && (value != NULL)) {
Nit: the |!= NULL| is unnecessary.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:131:
scoped_ptr<base::Value> value_allow;
Class member names end with an underscore.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider.h (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:50: // caller.
Provider copy the passed |value|.
The last sentence is unnecessary (what the provider does with the passed |value|
is an implementation detail).

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider_unittest.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider_unittest.cc:14:
TEST(ContentSettingsProviderTest, Mock) {
Yak shave: This should probably be a ContentSettingsMockProviderTest (and the
file renamed to content_settings_mock_provider_unittest.cc).

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_utils.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.cc:125: ContentSetting
setting) {
Couldn't you just implement this in terms of |ValueToContentSetting|?

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.cc:127:
DCHECK(value->GetType() == base::Value::TYPE_INTEGER);
This method DCHECKS if |value| is not an integer FundamentalValue. That's not
what the comment says.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.cc:131: return
ContentSetting(int_value == setting);
Ooooooh. I'm going to pretend I didn't see this line, okay? ;-)

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_utils.h (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.h:42: // Tests whether
the |value| equals the |setting|. If |value| is not an
Nit: "a |FundamentalValue|" (not "an").

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:87: class Value;
Here's a hint: You never need to forward-declare a class in a .cc file. If
you're just passing a pointer to somebody else, the class is already
forward-declared in another header (or otherwise that header wouldn't have been
able to declare the method you get it from), and if you actually need to
definition of the class, you directly #include its header.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:262:
scoped_ptr<base::Value> value(in_value);
So... maybe the PrefProviders should take ownership of the passed value? So far
I've seen them copy the value every time, and the passed value is freshly
constructed directly before, so there's an unnecessary allocation (plus the
interfaces of HostContentSettingsMap and Provider are slightly different).

The Provider could return a bool stating whether it takes ownership, and we
could stop the loop as soon as one provider returns true.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:286:
scoped_ptr<base::Value> value;
You don't need a scoped_ptr if you're releasing it anyway.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:20: #include
"base/values.h"
Why do you need this header? base::Value is already forward-declared.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:141: // loading
pages matching this pattern. For ContentSettingsTypes that require
Urgh, that comment is outdated :( If you're suffering from acute shortage of
yak-wool, you could remove the comment from the method above.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_api.cc:158:
scoped_ptr<base::Value> value(
Why this change? GetContentSetting is still available.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/geolocation/g...
File chrome/browser/geolocation/geolocation_settings_state.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_settings_state.cc:90:
profile_->GetHostContentSettingsMap()->GetWebsiteSetting(
Same here.

[markusheintz_, 2011-11-14 11:15:10]

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:73: //
TODO(markusheintz): |ContentSetting| should be replace with a |Value|.
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Nit: "replaced".

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:115:
default_settings_[CONTENT_SETTINGS_TYPE_COOKIES]->GetAsInteger(&int_value);
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Can you use the ValueToContentSetting helper method here? That would also be
> able to deal with edge cases like if there's no value for a content type.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:168:
Value::CreateIntegerValue(kDefaultSettings[content_type]));
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> If you get the integer value from |value| you can avoid allocating a new value
> here. 

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:209: int
int_value;
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Initialize this value please.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:300: for
(size_t type = 0; type < CONTENT_SETTINGS_NUM_TYPES; ++type) {
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Can you look up the |ContentSettingsType| for |content_type| (I know,
confusing
> names) directly with a helper method instead of this for loop?

Yes I could.But I thought I keep the change small first. There is so much more
to fix in this class.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:306:
default_settings_[ContentSettingsType(type)].reset(value->DeepCopy());
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> If we don't find a value, the |DeepCopy| will crash in a Release build. If you
> want to avoid that, you could keep the old version and just change this line
to
> create an IntegerValue from the |setting|.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:312: //
Migrate obsolete cookie prompt mode/
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Nit: Slash at the end of the comment

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider.cc:313: if
(ValueEqualsContentSetting(
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Oh, we have that method? Then you could use it in line 168 as well.

Obsolete as I removed the method ValueEqualsContentSetting.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_default_provider_unittest.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_default_provider_unittest.cc:37:
scoped_ptr<base::Value> value_allow;
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Class member names should end with an underscore.

Yeah so true :( . But I don't need this any more.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:522: if
(!found && (value != NULL)) {
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Nit: the |!= NULL| is unnecessary.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:131:
scoped_ptr<base::Value> value_allow;
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Class member names end with an underscore.

Shame on me. Glad I removed them now.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider.h (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:50: // caller.
Provider copy the passed |value|.
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> The last sentence is unnecessary (what the provider does with the passed
|value|
> is an implementation detail).

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider_unittest.cc
(right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider_unittest.cc:14:
TEST(ContentSettingsProviderTest, Mock) {
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Yak shave: This should probably be a ContentSettingsMockProviderTest (and the
> file renamed to content_settings_mock_provider_unittest.cc).

My new mantra is: Keep the CL as small as possible. I'll do this in a separate
CL. Promised.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_utils.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.cc:125: ContentSetting
setting) {
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Couldn't you just implement this in terms of |ValueToContentSetting|?

I removed this method again. Since using ValueToContentSettings solves all
use-cases.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.cc:127:
DCHECK(value->GetType() == base::Value::TYPE_INTEGER);
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> This method DCHECKS if |value| is not an integer FundamentalValue. That's not
> what the comment says.

Ahhh... these last minute code changes. Anyway this is obsolete now after
removing the method.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.cc:131: return
ContentSetting(int_value == setting);
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Ooooooh. I'm going to pretend I didn't see this line, okay? ;-)

No No No I did not write this :(. This was a change in the Matrix. :,(

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_utils.h (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_utils.h:42: // Tests whether
the |value| equals the |setting|. If |value| is not an
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Nit: "a |FundamentalValue|" (not "an").

Method removed.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:87: class Value;
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Here's a hint: You never need to forward-declare a class in a .cc file. If
> you're just passing a pointer to somebody else, the class is already
> forward-declared in another header (or otherwise that header wouldn't have
been
> able to declare the method you get it from), and if you actually need to
> definition of the class, you directly #include its header.

Another change in the matrix.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:262:
scoped_ptr<base::Value> value(in_value);
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> So... maybe the PrefProviders should take ownership of the passed value? So
far
> I've seen them copy the value every time, and the passed value is freshly
> constructed directly before, so there's an unnecessary allocation (plus the
> interfaces of HostContentSettingsMap and Provider are slightly different).
> 
> The Provider could return a bool stating whether it takes ownership, and we
> could stop the loop as soon as one provider returns true.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:286:
scoped_ptr<base::Value> value;
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> You don't need a scoped_ptr if you're releasing it anyway.

Yeah. I saw this somewhere else and though it might be a good practice to be
save. Certainly an overkill here. But you can't be save enough.

I changed it anyway.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:20: #include
"base/values.h"
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Why do you need this header? base::Value is already forward-declared.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:141: // loading
pages matching this pattern. For ContentSettingsTypes that require
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Urgh, that comment is outdated :( If you're suffering from acute shortage of
> yak-wool, you could remove the comment from the method above.

Done.

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_api.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_api.cc:158:
scoped_ptr<base::Value> value(
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Why this change? GetContentSetting is still available.

Originally I planed to rm the SetContentSetting method. This is obsolete. sry

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/geolocation/g...
File chrome/browser/geolocation/geolocation_settings_state.cc (right):

http://codereview.chromium.org/8539004/diff/2001/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_settings_state.cc:90:
profile_->GetHostContentSettingsMap()->GetWebsiteSetting(
On 2011/11/11 14:00:40, Bernhard Bauer wrote:
> Same here.

same as other

[Bernhard Bauer, 2011-11-15 15:40:28]

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_default_provider.cc:191:
prefs_->Set(prefs::kGeolocationDefaultContentSetting, *value);
I'm not happy that we pass ownership of |value| to |default_settings_dictionary|
but still keep using it. That can lead to subtle bugs if we change something and
|value| suddenly gets invalidated from under us.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:192:
value->DeepCopy());
You're leaking |value|.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:747:
value.get());
|SetWebsiteSetting| takes ownership of |value|, so this results in a double
free.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:770:
value.get());
Same here.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_provider.h (right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_provider.h:49: // the
|resource_identifier| must be non-empty. If the provider accepts the
The part about requiring a resource identifier is outdated.

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

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:261: bool owned =
provider->second->SetWebsiteSetting(
You could inline this and just do |if (provider->...) return|, then do a
NOTREACHED() at the end.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:130: //
|CONTENT_SETTING| as their data type. For content types that user other
Nit: "[...] that use other [...]"

[markusheintz_, 2011-11-15 17:36:58]

Thanks for the great catches!

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_default_provider.cc:191:
prefs_->Set(prefs::kGeolocationDefaultContentSetting, *value);
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> I'm not happy that we pass ownership of |value| to
|default_settings_dictionary|
> but still keep using it. That can lead to subtle bugs if we change something
and
> |value| suddenly gets invalidated from under us.

Done.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:192:
value->DeepCopy());
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> You're leaking |value|.

Done.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:747:
value.get());
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> |SetWebsiteSetting| takes ownership of |value|, so this results in a double
> free.

Done.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:770:
value.get());
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> Same here.

Done.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_provider.h (right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_provider.h:49: // the
|resource_identifier| must be non-empty. If the provider accepts the
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> The part about requiring a resource identifier is outdated.

Done.

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

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:261: bool owned =
provider->second->SetWebsiteSetting(
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> You could inline this and just do |if (provider->...) return|, then do a
> NOTREACHED() at the end.

Done.

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/8539004/diff/10018/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:130: //
|CONTENT_SETTING| as their data type. For content types that user other
On 2011/11/15 15:40:28, Bernhard Bauer wrote:
> Nit: "[...] that use other [...]"

Done.

[Bernhard Bauer, 2011-11-15 17:45:53]

LGTM.

Can you run this through valgrind to make sure we're not missing any more memory
errors?

http://codereview.chromium.org/8539004/diff/19001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/8539004/diff/19001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_default_provider.cc:153:
AutoReset<bool> auto_reset(&updating_preferences_, true);
I guess you don't need both of these scopes anymore (the objects stay in scope
until the end of the method)?

[markusheintz_, 2011-11-16 14:57:47]

I ran valgrind and fixed two memory leaks in the DefaultProvider unit_tests. I
guess we are good to go now.

Thanks for the review.

http://codereview.chromium.org/8539004/diff/19001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/8539004/diff/19001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_default_provider.cc:153:
AutoReset<bool> auto_reset(&updating_preferences_, true);
On 2011/11/15 17:45:53, Bernhard Bauer wrote:
> I guess you don't need both of these scopes anymore (the objects stay in scope
> until the end of the method)?

Done.

I thought it might be safer to first release the lock and then reset the
updating_preferences flag (see comment at 164. But it works removing one of the
anonymous scopes.

[commit-bot: I haz the power, 2011-11-16 15:32:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/8539004/33

[commit-bot: I haz the power, 2011-11-16 15:32:22]

Can't apply patch for file
chrome/browser/content_settings/content_settings_default_provider.cc.
While running patch -p1 --forward --force;
patching file
chrome/browser/content_settings/content_settings_default_provider.cc
Hunk #2 FAILED at 110.
Hunk #3 FAILED at 127.
Hunk #5 FAILED at 196.
Hunk #6 FAILED at 258.
4 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/content_settings/content_settings_default_provider.cc.rej

[commit-bot: I haz the power, 2011-11-17 12:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/8539004/33

[commit-bot: I haz the power, 2011-11-17 12:04:29]

Can't apply patch for file
chrome/browser/content_settings/content_settings_default_provider.cc.
While running patch -p1 --forward --force;
patching file
chrome/browser/content_settings/content_settings_default_provider.cc
Hunk #2 FAILED at 110.
Hunk #3 FAILED at 127.
Hunk #5 FAILED at 196.
Hunk #6 FAILED at 258.
4 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/content_settings/content_settings_default_provider.cc.rej

[commit-bot: I haz the power, 2011-11-17 13:28:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/8539004/28002

[commit-bot: I haz the power, 2011-11-17 16:13:16]

Change committed as 110500

[James Hawkins, 2011-11-17 22:06:50]

This commit introduced a leak; fix is here:

http://codereview.chromium.org/8592001/