Add content_settings::PolicyProvider and a set of new policies to managed content settings.

Add content_settings::PolicyProvider and a set of new policies to managed content settings.

BUG=63656, 63182
TEST=content_settings_policy_provider_unittest.cc,host_content_settings_map_unittests.cc

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

[markusheintz_, 2011-02-22 18:12:08]

I'd like to get some feedback on this CL before I finish it. So please do a
pre-review of this CL.

I know there are some nits to fix and some TODOs, so please check first whether:

- you like the changes in the content_settings_provider.*::ProviderInterface and

- how I handle resource identifier in content_settings_policy_provider.*

- I'd like to move code that updates content_settings in memory also to the
content_settings_base_provider. So check it. (Future re-factoring CL will pull
out the code from PrefProvider)

THANKS a lot

[Bernhard Bauer, 2011-02-23 13:18:53]

http://codereview.chromium.org/6542048/diff/2001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6542048/diff/2001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1193: 'name': 'PluginsAskForUrls',
We don't actually use the setting "Ask" for plug-ins, it's Click-to-play, which
just happens to be stored internally with the same value as ask.

Also (as discussed offline), we don't officially support click-to-play yet, so
we might not want to add it to policies yet.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:153: //TODO
Please expand.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:247:
std::string pattern_str(requesting_pattern.CanonicalizePattern());
I think you could move this down and get rid of the additional scope.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:254:
(*content_settings_map)[pattern_str].content_settings = ContentSettings();
Is this necessary? Shouldn't an empty ContentSettings() struct be constructed
automatically when you use the array subscript?

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:274:
ContentSettings& content_settings = i->second.content_settings;
Inline this local variable?

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:437: if
((*rule).c == CONTENT_SETTINGS_TYPE_PLUGINS &&
Nit: |rule->c| et al.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:444:
plugin_list->GetPluginGroups(true, &plugin_groups);
As discussed offline,  we should do this with a different approach (i.e. without
loading the plugin list).

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:533: } else
if (*name == prefs::kManagedCookiesBlockedForUrls) {
Collapse these into one big expression?

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:86:
: ui_thread_(BrowserThread::UI, &message_loop_) {
Is this because of a |DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))|
somewhere? If so, maybe we should think about making the class in question
derive from NonThreadSafe, which uses native thread identifiers instead of
BrowserThread IDs.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:238:
std::string resource1("shockwave");
I hope your unit tests don't depend on the actual, platform-dependent plugin
group definitions?

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:29: 
Nit: Remove blank line.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:104:
ProviderPtr(new content_settings::PolicyProvider(profile)));
I think you could just use |make_linked_ptr| instead of defining a FooPtr type
for every Foo.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:157: if (setting ==
CONTENT_SETTING_DEFAULT) {
You could get rid of the braces, but personally I don't really care that much.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:180: isManaged)
Doesn't this fit on one line?

[markusheintz_, 2011-02-23 18:42:09]

Fixed nits and TODO.

Please review.

Notice the change in the BaseProvider's GetContentSettings method.

http://codereview.chromium.org/6542048/diff/2001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6542048/diff/2001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1193: 'name': 'PluginsAskForUrls',
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> We don't actually use the setting "Ask" for plug-ins, it's Click-to-play,
which
> just happens to be stored internally with the same value as ask.
> 
> Also (as discussed offline), we don't officially support click-to-play yet, so
> we might not want to add it to policies yet.

Removed

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:153: //TODO
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Please expand.

Fixed the TODO

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:247:
std::string pattern_str(requesting_pattern.CanonicalizePattern());
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> I think you could move this down and get rid of the additional scope.

Yep. Done

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:254:
(*content_settings_map)[pattern_str].content_settings = ContentSettings();
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Is this necessary? Shouldn't an empty ContentSettings() struct be constructed
> automatically when you use the array subscript?

True.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.cc:274:
ContentSettings& content_settings = i->second.content_settings;
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Inline this local variable?

Done.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:437: if
((*rule).c == CONTENT_SETTINGS_TYPE_PLUGINS &&
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Nit: |rule->c| et al.
Obsolete now.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:444:
plugin_list->GetPluginGroups(true, &plugin_groups);
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> As discussed offline,  we should do this with a different approach (i.e.
without
> loading the plugin list).

Agree.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:533: } else
if (*name == prefs::kManagedCookiesBlockedForUrls) {
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Collapse these into one big expression?

Done.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:238:
std::string resource1("shockwave");
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> I hope your unit tests don't depend on the actual, platform-dependent plugin
> group definitions?
Not at all just wanted to have some names that look more real.

Revert these changes, to emphasis that we use plains strings here.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:29: 
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Nit: Remove blank line.

Done.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:104:
ProviderPtr(new content_settings::PolicyProvider(profile)));
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> I think you could just use |make_linked_ptr| instead of defining a FooPtr type
> for every Foo.

ABSOLUTELY!!! DONE

But having the ProviderPtr typedef is nice for the ProviderIterator typedefs.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:157: if (setting ==
CONTENT_SETTING_DEFAULT) {
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> You could get rid of the braces, but personally I don't really care that much.

Don't care much either. Flipped a coin and deleted them.

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:180: isManaged)
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Doesn't this fit on one line?

Done.

[markusheintz_, 2011-02-25 15:31:39]

Fixed unit_tests. Please review.

[Bernhard Bauer, 2011-02-25 16:08:01]

Looks pretty good.

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:86:
: ui_thread_(BrowserThread::UI, &message_loop_) {
On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> Is this because of a |DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))|
> somewhere? If so, maybe we should think about making the class in question
> derive from NonThreadSafe, which uses native thread identifiers instead of
> BrowserThread IDs.

Did you see this comment?

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

http://codereview.chromium.org/6542048/diff/18002/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:395: const
GURL& requesting_url,
Nit: this should be indented only four spaces.

http://codereview.chromium.org/6542048/diff/18002/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:457: //
settings change. The DefaultProvider will take care of that. WE are
Nit: "We"

[markusheintz_, 2011-02-28 09:08:43]

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

http://codereview.chromium.org/6542048/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:86:
: ui_thread_(BrowserThread::UI, &message_loop_) {
On 2011/02/25 16:08:01, Bernhard Bauer wrote:
> On 2011/02/23 13:18:53, Bernhard Bauer wrote:
> > Is this because of a |DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))|
> > somewhere? If so, maybe we should think about making the class in question
> > derive from NonThreadSafe, which uses native thread identifiers instead of
> > BrowserThread IDs.
> 
> Did you see this comment?

Sorry I forgot to add my reply. Yeah I agree. We should get rid of this. This is
true for other content_settings_providers as well. So I'd prefer to have a
sparate CL for that and fix all. Added a TODO.

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

http://codereview.chromium.org/6542048/diff/18002/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:395: const
GURL& requesting_url,
On 2011/02/25 16:08:01, Bernhard Bauer wrote:
> Nit: this should be indented only four spaces.

Done.

http://codereview.chromium.org/6542048/diff/18002/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:457: //
settings change. The DefaultProvider will take care of that. WE are
On 2011/02/25 16:08:01, Bernhard Bauer wrote:
> Nit: "We"

Done.

[Bernhard Bauer, 2011-02-28 09:21:43]

LGTM.

[markusheintz_, 2011-02-28 15:59:24]

@gfeher,jukummerow: Please check the policy template changes. THX

One more patchset:

- Add descriptions to the policy template.
- Fix some more nits

[gfeher, 2011-02-28 16:08:23]

http://codereview.chromium.org/6542048/diff/21004/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6542048/diff/21004/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:94: #   For your editing convenience:
highest ID currently used: 66
Please increase this number appropriately.

http://codereview.chromium.org/6542048/diff/21004/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1116: 'supported_on':
['chrome_frame:11-'],
Are these chrome_frame only changes? If not, then you should use chrome.*:11-
instead.
Please also add chrome_os:0.11- if this is supported on ChromeOS.

[markusheintz_, 2011-02-28 16:32:29]

http://codereview.chromium.org/6542048/diff/21004/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6542048/diff/21004/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:94: #   For your editing convenience:
highest ID currently used: 66
On 2011/02/28 16:08:23, gfeher wrote:
> Please increase this number appropriately.

Done.

http://codereview.chromium.org/6542048/diff/21004/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1116: 'supported_on':
['chrome_frame:11-'],
On 2011/02/28 16:08:23, gfeher wrote:
> Are these chrome_frame only changes? If not, then you should use chrome.*:11-
> instead.
> Please also add chrome_os:0.11- if this is supported on ChromeOS.

Done.

[markusheintz_, 2011-02-28 16:44:52]

s/chrome_os.*:0.11-/chrome_os:0.11-/

[gfeher, 2011-02-28 16:52:05]

policy_templates.json: LGTM

[Jakob Kummerow, 2011-02-28 17:18:27]

policy stuff LGTM as well.