Make ExtensionContentSettingsStore refcounted.

Make ExtensionContentSettingsStore refcounted.

BUG=83657
TESt=no crash

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

[Bernhard Bauer, 2011-07-03 18:08:29]

Please review.

[battre, 2011-07-06 09:12:05]

LGTM with some nits and optional stuff.

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_extension_provider.cc
(right):

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.cc:60: const
ContentSettingsDetails& details) {
should we DCHECK on the map_ here?

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_extension_provider.h
(right):

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:17: // A
content settings provider which uses settings defined by extensions.
nit: can we replace "uses" by "manages"?

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:21:
ExtensionProvider(HostContentSettingsMap* map,
nit: For someone not familiar with the code, it would be helpful to describe the
roles of the parameters here.

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

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:85: //
ShutdownOnUIThread has been called.
Just a thought:

should we codify this contract? I.e. do something like

void SetContentSetting(...) {
  DCHECK(not_shut_down);
  SetContentSettingImpl(...);
}
protected:
virtual void SetContentSettingImpl(...) = 0;

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:112: // This should
only be called on the UI thread  , and not after
nit: space

[Bernhard Bauer, 2011-07-06 12:06:51]

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_extension_provider.cc
(right):

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.cc:60: const
ContentSettingsDetails& details) {
On 2011/07/06 09:12:05, battre wrote:
> should we DCHECK on the map_ here?

Done.

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_extension_provider.h
(right):

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:17: // A
content settings provider which uses settings defined by extensions.
On 2011/07/06 09:12:05, battre wrote:
> nit: can we replace "uses" by "manages"?

Done.

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:21:
ExtensionProvider(HostContentSettingsMap* map,
On 2011/07/06 09:12:05, battre wrote:
> nit: For someone not familiar with the code, it would be helpful to describe
the
> roles of the parameters here.

I added comments to the member variables below.

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

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:85: //
ShutdownOnUIThread has been called.
On 2011/07/06 09:12:05, battre wrote:
> Just a thought:
> 
> should we codify this contract? I.e. do something like
> 
> void SetContentSetting(...) {
>   DCHECK(not_shut_down);
>   SetContentSettingImpl(...);
> }
> protected:
> virtual void SetContentSettingImpl(...) = 0;

http://codereview.chromium.org/7218073 adds a DCHECK like that in
content_settings::PrefProvider (which has the only non-empty implementation).

http://codereview.chromium.org/7275018/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:112: // This should
only be called on the UI thread  , and not after
On 2011/07/06 09:12:05, battre wrote:
> nit: space

Done.

[Bernhard Bauer, 2011-07-06 12:27:00]

Miranda, can you take a look at the profile changes? Thanks!

[Miranda Callahan, 2011-07-06 14:16:18]

On 2011/07/06 12:27:00, Bernhard Bauer wrote:
> Miranda, can you take a look at the profile changes? Thanks!

LGTM for profile changes.