Add content_settings::PrefProvider to HostContentSettingsMap.

Add content_settings::PrefProvider to HostContentSettingsMap.

BUG=63656
TEST=host_content_settings_unittest.cc

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

[markusheintz_, 2011-02-03 17:26:05]

Please start reviewing my cl.

It's quit a bit and I there are a few thing that need to be discussed.

- Pls check the init guard in content_settings::PrefProvider that is used to
prevent throwing Notifications while the Provider is created to prevent death
locks.

- Pls check the ResetToDefaults() I added to the ProviderInterface. I need this
method to get the number of send notifications right when a client resets the
HCSM

[Paweł Hajdan Jr., 2011-02-04 07:26:28]

Drive-by with a tiny suggestion. No need to wait for me.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_excep...
File chrome/browser/content_exceptions_table_model_unittest.cc (right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_excep...
chrome/browser/content_exceptions_table_model_unittest.cc:24: TestingProfile*
otr_profile = new TestingProfile();
nit: Can it be stack-allocated now?

[jochen (gone - plz use gerrit), 2011-02-04 08:15:20]

please remove files unrelated to this CL

[jochen (gone - plz use gerrit), 2011-02-04 08:38:43]

first pass of comments

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

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:81: // to
PrefContentSettingsProvider.
sure.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:92: // User defined
non default content settings are managed provided by the
s/managed //

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:93: //
PrefProvider.
please add a comment that the order is critical. Why don't you move this block
up to where the default providers are created?

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:156: // Check if
the URL is matched be a whitlisted scheme.
I don't think we need this comment, as the name of the function is
self-explanatory

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:181: if
(content_settings::PrefProvider::RequiresResourceIdentifier(
can't all providers just return DEFAULT in this case?

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:188: j !=
CONTENT_SETTINGS_TYPE_PLUGINS) ||
I think we could also remove this test, and just override it with the managed
default

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:206:
output.settings[j] = CONTENT_SETTING_DEFAULT;
again, just assume the provider returns _DEFAULT

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:229: // AI: Think
about and discuss what is sensible here.
i think all rules should be returned, the UI can then display whatever it wants

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:383:
CONTENT_SETTINGS_TYPE_DEFAULT,
indenting?

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

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:401: 
please don't remove this lines

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/mock_content_settings_provider.h (right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/mock_content_settings_provider.h:40: 
only one empty line here

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/mock_content_settings_provider.h:74:
DISALLOW_COPY_AND_ASSIGN(MockProvider);
empty line before this line

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/policy_content_settings_provider.cc:21:
namespace content_settings {
this should stay an anonymous namespace

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:82: 
no additional empty line

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:95: : public
ProviderInterface,
this should be on the previous line

[markusheintz_, 2011-02-04 10:08:03]

Worked on the comments.

There is at least one trailing space I forgot to remove will do that. sry.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_excep...
File chrome/browser/content_exceptions_table_model_unittest.cc (right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_excep...
chrome/browser/content_exceptions_table_model_unittest.cc:24: TestingProfile*
otr_profile = new TestingProfile();
On 2011/02/04 07:26:28, Paweł Hajdan Jr. wrote:
> nit: Can it be stack-allocated now?
 Sure. Don't see why not.

DONE.

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

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:81: // to
PrefContentSettingsProvider.
On 2011/02/04 08:38:43, jochen wrote:
> sure.

Cool:-) When I added that comment I was afraid that I might not be able to split
this from the third party cookie stuff. I don't also want to move the
Third_set_Third_party Cookie stuff with it.
But I guess it'll work :-)

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:92: // User defined
non default content settings are managed provided by the
On 2011/02/04 08:38:43, jochen wrote:
> s/managed //

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:93: //
PrefProvider.
On 2011/02/04 08:38:43, jochen wrote:
> please add a comment that the order is critical. Why don't you move this block
> up to where the default providers are created?

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:156: // Check if
the URL is matched be a whitlisted scheme.
On 2011/02/04 08:38:43, jochen wrote:
> I don't think we need this comment, as the name of the function is
> self-explanatory

Removed this line.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:181: if
(content_settings::PrefProvider::RequiresResourceIdentifier(
On 2011/02/04 08:38:43, jochen wrote:
> can't all providers just return DEFAULT in this case?
Yeah that would be much nicer. I will need to check for the presence of an non
empty resource identifier.

Done

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:188: j !=
CONTENT_SETTINGS_TYPE_PLUGINS) ||
On 2011/02/04 08:38:43, jochen wrote:
> I think we could also remove this test, and just override it with the managed
> default

This would also set the content settings CONTENT_SETTING_DEFAULT for plugins
explicitly if an resource identifier is required. I think that is nice, but it
would not match the current behavior.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:206:
output.settings[j] = CONTENT_SETTING_DEFAULT;
On 2011/02/04 08:38:43, jochen wrote:
> again, just assume the provider returns _DEFAULT

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:229: // AI: Think
about and discuss what is sensible here.
On 2011/02/04 08:38:43, jochen wrote:
> i think all rules should be returned, the UI can then display whatever it
wants

Cool with me.

Could we leave it this way for this CL then? 

I think there are 2 possibilities moving forward:
A)
Add (in a future CL) a read only flag to returned content settings if they can't
be changed. Outerwise the UI will not know which are read-only. We should then
also add who set the rule: POLICY, EXTENSION, USER

B) Provide 
  GetPolicySettingsForOneType
  GetExtensionSettingsForOneType
  GetUserSettingsForOneType

I kind of favor solution B) with a read-only flag.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:383:
CONTENT_SETTINGS_TYPE_DEFAULT,
On 2011/02/04 08:38:43, jochen wrote:
> indenting?

Done.

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

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:401: 
On 2011/02/04 08:38:43, jochen wrote:
> please don't remove this lines

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/mock_content_settings_provider.h (right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/mock_content_settings_provider.h:40: 
On 2011/02/04 08:38:43, jochen wrote:
> only one empty line here

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/mock_content_settings_provider.h:74:
DISALLOW_COPY_AND_ASSIGN(MockProvider);
On 2011/02/04 08:38:43, jochen wrote:
> empty line before this line

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/policy_content_settings_provider.cc:21:
namespace content_settings {
On 2011/02/04 08:38:43, jochen wrote:
> this should stay an anonymous namespace

Uuups sry. Done

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:82: 
On 2011/02/04 08:38:43, jochen wrote:
> no additional empty line

Done.

http://codereview.chromium.org/6410022/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:95: : public
ProviderInterface,
On 2011/02/04 08:38:43, jochen wrote:
> this should be on the previous line

Done.

[Bernhard Bauer, 2011-02-07 09:46:46]

Whoops, forgot to Publish+Mail Comments on Friday :-)

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

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:51: typedef
std::vector<ProviderPtr>::iterator provider_iterator;
Please use CamelCase for the type, lowercase types look very confusing.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:619: // If
resource content settings are enable GetContentSettings should return
Nit: "enabled"

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.cc (right):

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:449:
prefs->GetMutableDictionary(prefs::kContentSettingsPatterns);
Nit: indent two more spaces.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:458:
i(map_to_modify->find(pattern_str));
Nit: break after `i(' ?

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:543:
requested_setting(content_type, resource_identifier);
Nit: break after `requested_setting(' if it fits.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:552: setting =
i->second.content_settings_for_resources.find(
Nit: Please add braces for the two branches.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:597:
prefs->GetMutableDictionary(prefs::kContentSettingsPatterns);
Nit: indent two more spaces.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:615:
all_settings_dictionary->
Nit: braces.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:876:
!RequiresResourceIdentifier(ContentSettingsType(j)))
Nit: Braces.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider_unittest.cc
(right):

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider_unittest.cc:167:
/*
Ugh. Code that's commented out usually should be removed entirely.

http://codereview.chromium.org/6410022/diff/1028/ppapi/c/dev/ppb_opengles_dev.h
File ppapi/c/dev/ppb_opengles_dev.h (right):

http://codereview.chromium.org/6410022/diff/1028/ppapi/c/dev/ppb_opengles_dev...
ppapi/c/dev/ppb_opengles_dev.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
If you don't have any other changes, please remove this file from the CL.

http://codereview.chromium.org/6410022/diff/1028/ppapi/lib/gl/gles2/gles2.c
File ppapi/lib/gl/gles2/gles2.c (right):

http://codereview.chromium.org/6410022/diff/1028/ppapi/lib/gl/gles2/gles2.c#n...
ppapi/lib/gl/gles2/gles2.c:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
Same here.

http://codereview.chromium.org/6410022/diff/1028/webkit/plugins/ppapi/ppb_ope...
File webkit/plugins/ppapi/ppb_opengles_impl.cc (right):

http://codereview.chromium.org/6410022/diff/1028/webkit/plugins/ppapi/ppb_ope...
webkit/plugins/ppapi/ppb_opengles_impl.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
And here.

[markusheintz_, 2011-02-07 15:57:10]

@Pawel: Can't change the unittest :-)

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

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:51: typedef
std::vector<ProviderPtr>::iterator provider_iterator;
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Please use CamelCase for the type, lowercase types look very confusing.

Agree. Done. I just tried to follow the style I saw in other places.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:619: // If
resource content settings are enable GetContentSettings should return
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: "enabled"

Done.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.cc (right):

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:449:
prefs->GetMutableDictionary(prefs::kContentSettingsPatterns);
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: indent two more spaces.

Done.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:458:
i(map_to_modify->find(pattern_str));
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: break after `i(' ?

Done.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:543:
requested_setting(content_type, resource_identifier);
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: break after `requested_setting(' if it fits.

Done.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:552: setting =
i->second.content_settings_for_resources.find(
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: Please add braces for the two branches.

Done.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:597:
prefs->GetMutableDictionary(prefs::kContentSettingsPatterns);
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: indent two more spaces.

Done.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:615:
all_settings_dictionary->
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: braces.

Put everything in one line.

http://codereview.chromium.org/6410022/diff/1028/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:876:
!RequiresResourceIdentifier(ContentSettingsType(j)))
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Nit: Braces.

Done.

http://codereview.chromium.org/6410022/diff/1028/ppapi/c/dev/ppb_opengles_dev.h
File ppapi/c/dev/ppb_opengles_dev.h (right):

http://codereview.chromium.org/6410022/diff/1028/ppapi/c/dev/ppb_opengles_dev...
ppapi/c/dev/ppb_opengles_dev.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> If you don't have any other changes, please remove this file from the CL.

Done. Not sure how this change happend. I didn't touch the file.

http://codereview.chromium.org/6410022/diff/1028/ppapi/lib/gl/gles2/gles2.c
File ppapi/lib/gl/gles2/gles2.c (right):

http://codereview.chromium.org/6410022/diff/1028/ppapi/lib/gl/gles2/gles2.c#n...
ppapi/lib/gl/gles2/gles2.c:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> Same here.

Done. Not sure how this change happend. I didn't touch the file.

http://codereview.chromium.org/6410022/diff/1028/webkit/plugins/ppapi/ppb_ope...
File webkit/plugins/ppapi/ppb_opengles_impl.cc (right):

http://codereview.chromium.org/6410022/diff/1028/webkit/plugins/ppapi/ppb_ope...
webkit/plugins/ppapi/ppb_opengles_impl.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/02/07 09:46:46, Bernhard Bauer wrote:
> And here.
Done. Not sure how this change happend. I didn't touch the file.

[Bernhard Bauer, 2011-02-07 16:20:31]

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

http://codereview.chromium.org/6410022/diff/8025/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:46:
default_ProviderIterator;
Please use full camel case here and below.

http://codereview.chromium.org/6410022/diff/8025/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:56: rules_iterator;
Please use camel case here as well.

[markusheintz_, 2011-02-07 16:29:26]

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

http://codereview.chromium.org/6410022/diff/8025/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:46:
default_ProviderIterator;
On 2011/02/07 16:20:31, Bernhard Bauer wrote:
> Please use full camel case here and below.

Oh sry I totally missed that after %s/.../.../g

THANKS!!

http://codereview.chromium.org/6410022/diff/8025/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:56: rules_iterator;
On 2011/02/07 16:20:31, Bernhard Bauer wrote:
> Please use camel case here as well.

Done.

[Bernhard Bauer, 2011-02-07 16:35:17]

LGTM.