Make the hcsm and its providers communicate via an observer interface.

Add a content_settings::Observer and make providers notify the host content settings map via the observer interface about content settings changes. The host content settings map now the single source for sending out CONTENT_SETTINGS_CHANGED notification.

BUG=TODO
TEST=host_content_settings_map_unittest.cc,
     content_settings_policy_provider_unittest.cc,
     content_settings_pref_provider_unittest.cc,

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

[markusheintz_, 2011-07-12 15:57:35]

Could you please already take a look at this CL before I start changing the
unit_tests? Thanks.

[Bernhard Bauer, 2011-07-13 07:58:18]

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.cc
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.cc:14:
AbstractProvider::~AbstractProvider() {
Can you add a |DCHECK(observers_.empty())|?

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.h
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:16: class
AbstractProvider : public ProviderInterface {
Seeing as this class mainly manages Observers, maybe you could call it
ObservingProvider?

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:26: //
ProviderInterface implementation
Do you need to declare these methods again?

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:52: void
RemoveAllObserver();
This method name should read |RemoveAllObservers| (plural).

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

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:25:
ExtensionProvider(content_settings::Observer* observer,
Nit: Namespace content_settings is unnecessary.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:31: //
AbstractProvider methods:
Nit: I think you could leave this comment as "ProviderInterface", because that's
how these methods will be called.

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

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:59: typedef
content_settings::ProviderInterface::Rules::iterator
While you're in here, could you do some additional cleanup? This typedef and the
one below it is not used.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:455: LOG(ERROR) <<
" *** OnContentSettingChanged event received:"
Please remember to remove this statement before committing.

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

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:195: //
ProviderObserverInterface implementation.
Nit: The interface is called just |content_settings::Observer| now?

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:236:
std::vector<linked_ptr<content_settings::AbstractProvider> >
Wait, I think we should still use the base interface here; that's the whole
point of having one ;-)

[markusheintz_, 2011-07-13 12:48:16]

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.cc
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.cc:14:
AbstractProvider::~AbstractProvider() {
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Can you add a |DCHECK(observers_.empty())|?

Done.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.h
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:16: class
AbstractProvider : public ProviderInterface {
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Seeing as this class mainly manages Observers, maybe you could call it
> ObservingProvider?

How about ObservableProvider as the provider can be observed but it does not
observe anything.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:26: //
ProviderInterface implementation
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Do you need to declare these methods again?

No I just though it might be helpful for people to notice that this class has
pure virtual methods.

I'm totally ok with removing them.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:52: void
RemoveAllObserver();
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> This method name should read |RemoveAllObservers| (plural).

Done.

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

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:25:
ExtensionProvider(content_settings::Observer* observer,
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Nit: Namespace content_settings is unnecessary.

I need it here since the name otherwise collides with the
ExtensionContentSettingsStore::Observer

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.h:31: //
AbstractProvider methods:
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Nit: I think you could leave this comment as "ProviderInterface", because
that's
> how these methods will be called.

Done. I also changed this comment in all the other providers.

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

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:59: typedef
content_settings::ProviderInterface::Rules::iterator
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> While you're in here, could you do some additional cleanup? This typedef and
the
> one below it is not used.

Done.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:455: LOG(ERROR) <<
" *** OnContentSettingChanged event received:"
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Please remember to remove this statement before committing.

removed

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

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:195: //
ProviderObserverInterface implementation.
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Nit: The interface is called just |content_settings::Observer| now?

Done.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:236:
std::vector<linked_ptr<content_settings::AbstractProvider> >
On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> Wait, I think we should still use the base interface here; that's the whole
> point of having one ;-)

Oh yeah. Forgot to change that back. Initially I registered the observer with
the AddObserver methoded and then the ProviderInterface didn't work.

Change back to ProviderInterface

[Bernhard Bauer, 2011-07-13 13:21:28]

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.cc
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.cc:14:
AbstractProvider::~AbstractProvider() {
On 2011/07/13 12:48:16, markusheintz_ wrote:
> On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> > Can you add a |DCHECK(observers_.empty())|?
> 
> Done.

Actually, now that you use an |ObserverList| you can declare it as
ObservList<Foo, true> to get automatic checking ;-)

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.h
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:16: class
AbstractProvider : public ProviderInterface {
On 2011/07/13 12:48:16, markusheintz_ wrote:
> On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> > Seeing as this class mainly manages Observers, maybe you could call it
> > ObservingProvider?
> 
> How about ObservableProvider as the provider can be observed but it does not
> observe anything.

Sounds good.

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.h (right):

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.h:84:
PolicyProvider(Observer* observer,
Come to think of it, do we actually *need* an observer in the constructor? If an
ObservableProvider has an arbitrary number of observers (as opposed to exactly
one), it should also be possible to have zero (like for the unit test case).

[markusheintz_, 2011-07-13 13:28:53]

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.h (right):

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.h:84:
PolicyProvider(Observer* observer,
On 2011/07/13 13:21:28, Bernhard Bauer wrote:
> Come to think of it, do we actually *need* an observer in the constructor? If
an
> ObservableProvider has an arbitrary number of observers (as opposed to exactly
> one), it should also be possible to have zero (like for the unit test case).

That is totally correct!

Basically the host content settings map need to call AddObserver(this) after
creating a content setting provider and RemoveObserver(this) when it is 
shutdown.

I implemented this originally and I'm very in favor of that solution.

I will also have to change the ProviderPtr type in the HostContentSettingsMap to
point the an ObservableProvider (AbstractProvider)

[Bernhard Bauer, 2011-07-13 13:41:32]

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.h (right):

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.h:84:
PolicyProvider(Observer* observer,
On 2011/07/13 13:28:53, markusheintz_ wrote:
> On 2011/07/13 13:21:28, Bernhard Bauer wrote:
> > Come to think of it, do we actually *need* an observer in the constructor?
If
> an
> > ObservableProvider has an arbitrary number of observers (as opposed to
exactly
> > one), it should also be possible to have zero (like for the unit test case).
> 
> That is totally correct!
> 
> Basically the host content settings map need to call AddObserver(this) after
> creating a content setting provider and RemoveObserver(this) when it is 
> shutdown.
> 
> I implemented this originally and I'm very in favor of that solution.
> 
> I will also have to change the ProviderPtr type in the HostContentSettingsMap
to
> point the an ObservableProvider (AbstractProvider)

No, you don't. At the point where you create the concrete Provider, you know its
exact type, so you can call ObservableProvider methods on it, but the elements
of the provider list only need to be of type Provider.

[markusheintz_, 2011-07-13 14:14:42]

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.cc
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.cc:14:
AbstractProvider::~AbstractProvider() {
On 2011/07/13 13:21:28, Bernhard Bauer wrote:
> On 2011/07/13 12:48:16, markusheintz_ wrote:
> > On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> > > Can you add a |DCHECK(observers_.empty())|?
> > 
> > Done.
> 
> Actually, now that you use an |ObserverList| you can declare it as
> ObservList<Foo, true> to get automatic checking ;-)

Done.

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_abstract_provider.h
(right):

http://codereview.chromium.org/7344008/diff/3026/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_abstract_provider.h:16: class
AbstractProvider : public ProviderInterface {
On 2011/07/13 13:21:28, Bernhard Bauer wrote:
> On 2011/07/13 12:48:16, markusheintz_ wrote:
> > On 2011/07/13 07:58:18, Bernhard Bauer wrote:
> > > Seeing as this class mainly manages Observers, maybe you could call it
> > > ObservingProvider?
> > 
> > How about ObservableProvider as the provider can be observed but it does not
> > observe anything.
> 
> Sounds good.

Done.

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.h (right):

http://codereview.chromium.org/7344008/diff/7005/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.h:84:
PolicyProvider(Observer* observer,
On 2011/07/13 13:41:32, Bernhard Bauer wrote:
> On 2011/07/13 13:28:53, markusheintz_ wrote:
> > On 2011/07/13 13:21:28, Bernhard Bauer wrote:
> > > Come to think of it, do we actually *need* an observer in the constructor?
> If
> > an
> > > ObservableProvider has an arbitrary number of observers (as opposed to
> exactly
> > > one), it should also be possible to have zero (like for the unit test
case).
> > 
> > That is totally correct!
> > 
> > Basically the host content settings map need to call AddObserver(this) after
> > creating a content setting provider and RemoveObserver(this) when it is 
> > shutdown.
> > 
> > I implemented this originally and I'm very in favor of that solution.
> > 
> > I will also have to change the ProviderPtr type in the
HostContentSettingsMap
> to
> > point the an ObservableProvider (AbstractProvider)
> 
> No, you don't. At the point where you create the concrete Provider, you know
its
> exact type, so you can call ObservableProvider methods on it, but the elements
> of the provider list only need to be of type Provider.

Ok.

[Bernhard Bauer, 2011-07-13 14:22:24]

LGTM with a nit:

http://codereview.chromium.org/7344008/diff/8006/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_mock_observer.h (right):

http://codereview.chromium.org/7344008/diff/8006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_mock_observer.h:15: class
MockObserver : public Observer {
This class is only used in one unit test, so you could just declare it there.