Add ObservableDefaultProvider class and let the HostContentSettingsMap and it's DefaultProviders communicate through an observer interface.

BUG=63656
TEST=host_content_settings_map_unittest.cc,
     content_settings_pref_provider_unittests.cc,
     content_settings_policy_provider_unittests.cc

Review URL: http://codereview.chromium.org/7368004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92664 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 116 insertions, 107 deletions

diff --git a/chrome/browser/content_settings/content_settings_observable_provider.cc b/chrome/browser/content_settings/content_settings_observable_provider.cc
index f904de3..cef0dd7 100644
--- a/chrome/browser/content_settings/content_settings_observable_provider.cc
+++ b/chrome/browser/content_settings/content_settings_observable_provider.cc
@@ -6,6 +6,45 @@
 
 namespace content_settings {
 
+// ////////////////////////////////////////////////////////////////////////////
+// ObservableDefaultProvider
+//
+ObservableDefaultProvider::ObservableDefaultProvider() {
+}
+
+ObservableDefaultProvider::~ObservableDefaultProvider() {
+}
+
+void ObservableDefaultProvider::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void ObservableDefaultProvider::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void ObservableDefaultProvider::RemoveAllObservers() {
+  observer_list_.Clear();
+}
+
+void ObservableDefaultProvider::NotifyObservers(
+    ContentSettingsPattern primary_pattern,
+    ContentSettingsPattern secondary_pattern,
+    ContentSettingsType content_type,
+    std::string resource_identifier) {
+  FOR_EACH_OBSERVER(Observer,
+                    observer_list_,
+                    OnContentSettingChanged(
+                        primary_pattern,
+                        secondary_pattern,
+                        content_type,
+                        resource_identifier));
+}
+
+// ////////////////////////////////////////////////////////////////////////////
+// ObservableProvider
+//
+
 ObservableProvider::ObservableProvider() {
 }
 
diff --git a/chrome/browser/content_settings/content_settings_observable_provider.h b/chrome/browser/content_settings/content_settings_observable_provider.h
index 86dd80c..84e0d4e 100644
--- a/chrome/browser/content_settings/content_settings_observable_provider.h
+++ b/chrome/browser/content_settings/content_settings_observable_provider.h
@@ -14,6 +14,25 @@
 
 namespace content_settings {
 
+class ObservableDefaultProvider : public DefaultProviderInterface {
+ public:
+  ObservableDefaultProvider();
+  virtual ~ObservableDefaultProvider();
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+ protected:
+  void NotifyObservers(ContentSettingsPattern primary_pattern,
+                       ContentSettingsPattern secondary_pattern,
+                       ContentSettingsType content_type,
+                       std::string resource_identifier);
+  void RemoveAllObservers();
+
+ private:
+  ObserverList<Observer, true> observer_list_;
+};
+
 class ObservableProvider : public ProviderInterface {
  public:
   ObservableProvider();
diff --git a/chrome/browser/content_settings/content_settings_policy_provider.cc b/chrome/browser/content_settings/content_settings_policy_provider.cc
index 47d321d..5274b1a 100644
--- a/chrome/browser/content_settings/content_settings_policy_provider.cc
+++ b/chrome/browser/content_settings/content_settings_policy_provider.cc
@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "base/command_line.h"
-#include "chrome/browser/content_settings/content_settings_details.h"
 #include "chrome/browser/content_settings/content_settings_pattern.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -99,10 +98,8 @@ const PrefsForManagedContentSettingsMapEntry
 
 namespace content_settings {
 
-PolicyDefaultProvider::PolicyDefaultProvider(HostContentSettingsMap* map,
-                                             PrefService* prefs)
-    : host_content_settings_map_(map),
-      prefs_(prefs) {
+PolicyDefaultProvider::PolicyDefaultProvider(PrefService* prefs)
+    : prefs_(prefs) {
   // Read global defaults.
   DCHECK_EQ(arraysize(kPrefToManageType),
             static_cast<size_t>(CONTENT_SETTINGS_NUM_TYPES));
@@ -172,11 +169,10 @@ void PolicyDefaultProvider::Observe(int type,
       return;
     }
 
-    ContentSettingsDetails details(ContentSettingsPattern(),
-                                   ContentSettingsPattern(),
-                                   CONTENT_SETTINGS_TYPE_DEFAULT,
-                                   std::string());
-    NotifyObservers(details);
+    NotifyObservers(ContentSettingsPattern(),
+                    ContentSettingsPattern(),
+                    CONTENT_SETTINGS_TYPE_DEFAULT,
+                    std::string());
   } else {
     NOTREACHED() << "Unexpected notification";
   }
@@ -185,21 +181,9 @@ void PolicyDefaultProvider::Observe(int type,
 void PolicyDefaultProvider::ShutdownOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prefs_);
+  RemoveAllObservers();
   pref_change_registrar_.RemoveAll();
   prefs_ = NULL;
-  host_content_settings_map_ = NULL;
-}
-
-
-void PolicyDefaultProvider::NotifyObservers(
-    const ContentSettingsDetails& details) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (host_content_settings_map_ == NULL)
-    return;
-  NotificationService::current()->Notify(
-      chrome::NOTIFICATION_CONTENT_SETTINGS_CHANGED,
-      Source<HostContentSettingsMap>(host_content_settings_map_),
-      Details<const ContentSettingsDetails>(&details));
 }
 
 void PolicyDefaultProvider::ReadManagedDefaultSettings() {
diff --git a/chrome/browser/content_settings/content_settings_policy_provider.h b/chrome/browser/content_settings/content_settings_policy_provider.h
index 34a3bf2..ab72012 100644
--- a/chrome/browser/content_settings/content_settings_policy_provider.h
+++ b/chrome/browser/content_settings/content_settings_policy_provider.h
@@ -25,10 +25,10 @@ class PrefService;
 
 namespace content_settings {
 
-class PolicyDefaultProvider : public DefaultProviderInterface,
+class PolicyDefaultProvider : public ObservableDefaultProvider,
                               public NotificationObserver {
  public:
-  PolicyDefaultProvider(HostContentSettingsMap* map, PrefService* prefs);
+  explicit PolicyDefaultProvider(PrefService* prefs);
   virtual ~PolicyDefaultProvider();
 
   // DefaultContentSettingsProvider implementation.
@@ -48,12 +48,6 @@ class PolicyDefaultProvider : public DefaultProviderInterface,
                        const NotificationDetails& details);
 
  private:
-  // Informs observers that content settings have changed. Make sure that
-  // |lock_| is not held when calling this, as listeners will usually call one
-  // of the GetSettings functions in response, which would then lead to a
-  // mutex deadlock.
-  void NotifyObservers(const ContentSettingsDetails& details);
-
   // Reads the policy managed default settings.
   void ReadManagedDefaultSettings();
 
@@ -63,7 +57,6 @@ class PolicyDefaultProvider : public DefaultProviderInterface,
   // Copies of the pref data, so that we can read it on the IO thread.
   ContentSettings managed_default_content_settings_;
 
-  HostContentSettingsMap* host_content_settings_map_;
   PrefService* prefs_;
 
   // Used around accesses to the managed_default_content_settings_ object to
diff --git a/chrome/browser/content_settings/content_settings_policy_provider_unittest.cc b/chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
index 04e3d08..c827f188 100644
--- a/chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
+++ b/chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
@@ -7,7 +7,6 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "chrome/browser/content_settings/content_settings_mock_observer.h"
-#include "chrome/browser/content_settings/mock_settings_observer.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
@@ -37,7 +36,7 @@ class PolicyDefaultProviderTest : public TestingBrowserProcessTest {
 TEST_F(PolicyDefaultProviderTest, DefaultValues) {
   TestingProfile profile;
   TestingPrefService* prefs = profile.GetTestingPrefService();
-  PolicyDefaultProvider provider(profile.GetHostContentSettingsMap(), prefs);
+  PolicyDefaultProvider provider(prefs);
 
   // By default, policies should be off.
   ASSERT_FALSE(
@@ -64,26 +63,30 @@ TEST_F(PolicyDefaultProviderTest, DefaultValues) {
 // if the managed setting is removed.
 TEST_F(PolicyDefaultProviderTest, ObserveManagedSettingsChange) {
   TestingProfile profile;
-  MockSettingsObserver observer;
-  // Make sure the content settings map exists.
-  profile.GetHostContentSettingsMap();
   TestingPrefService* prefs = profile.GetTestingPrefService();
+  PolicyDefaultProvider provider(prefs);
+
+  MockObserver mock_observer;
+  EXPECT_CALL(mock_observer,
+              OnContentSettingChanged(_,
+                                      _,
+                                      CONTENT_SETTINGS_TYPE_DEFAULT,
+                                      ""));
+  provider.AddObserver(&mock_observer);
 
   // Set the managed default-content-setting.
-  EXPECT_CALL(observer,
-              OnContentSettingsChanged(profile.GetHostContentSettingsMap(),
-                                       CONTENT_SETTINGS_TYPE_DEFAULT, true,
-                                       _, _, true));
   prefs->SetManagedPref(prefs::kManagedDefaultImagesSetting,
                         Value::CreateIntegerValue(CONTENT_SETTING_BLOCK));
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
+  ::testing::Mock::VerifyAndClearExpectations(&mock_observer);
 
-  EXPECT_CALL(observer,
-              OnContentSettingsChanged(profile.GetHostContentSettingsMap(),
-                                       CONTENT_SETTINGS_TYPE_DEFAULT, true,
-                                       _, _, true));
+  EXPECT_CALL(mock_observer,
+              OnContentSettingChanged(_,
+                                      _,
+                                      CONTENT_SETTINGS_TYPE_DEFAULT,
+                                      ""));
   // Remove the managed default-content-setting.
   prefs->RemoveManagedPref(prefs::kManagedDefaultImagesSetting);
+  provider.ShutdownOnUIThread();
 }
 
 class PolicyProviderTest : public testing::Test {
diff --git a/chrome/browser/content_settings/content_settings_pref_provider.cc b/chrome/browser/content_settings/content_settings_pref_provider.cc
index bd50379..af92515a 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider.cc
@@ -12,7 +12,6 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
-#include "chrome/browser/content_settings/content_settings_details.h"
 #include "chrome/browser/content_settings/content_settings_pattern.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -149,11 +148,9 @@ ContentSetting FixObsoleteCookiePromptMode(ContentSettingsType content_type,
 
 namespace content_settings {
 
-PrefDefaultProvider::PrefDefaultProvider(HostContentSettingsMap* map,
-                                         PrefService* prefs,
+PrefDefaultProvider::PrefDefaultProvider(PrefService* prefs,
                                          bool incognito)
-    : host_content_settings_map_(map),
-      prefs_(prefs),
+    : prefs_(prefs),
       is_incognito_(incognito),
       updating_preferences_(false) {
   DCHECK(prefs_);
@@ -217,12 +214,10 @@ void PrefDefaultProvider::UpdateDefaultSetting(
     }
   }
 
-  ContentSettingsDetails details(
-      ContentSettingsPattern(),
-      ContentSettingsPattern(),
-      content_type,
-      std::string());
-  NotifyObservers(details);
+  NotifyObservers(ContentSettingsPattern(),
+                  ContentSettingsPattern(),
+                  content_type,
+                  std::string());
 }
 
 bool PrefDefaultProvider::DefaultSettingIsManaged(
@@ -248,11 +243,10 @@ void PrefDefaultProvider::Observe(int type,
       return;
     }
 
-    ContentSettingsDetails details(ContentSettingsPattern(),
-                                   ContentSettingsPattern(),
-                                   CONTENT_SETTINGS_TYPE_DEFAULT,
-                                   std::string());
-    NotifyObservers(details);
+    NotifyObservers(ContentSettingsPattern(),
+                    ContentSettingsPattern(),
+                    CONTENT_SETTINGS_TYPE_DEFAULT,
+                    std::string());
   } else {
     NOTREACHED() << "Unexpected notification";
   }
@@ -261,9 +255,9 @@ void PrefDefaultProvider::Observe(int type,
 void PrefDefaultProvider::ShutdownOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prefs_);
+  RemoveAllObservers();
   pref_change_registrar_.RemoveAll();
   prefs_ = NULL;
-  host_content_settings_map_ = NULL;
 }
 
 void PrefDefaultProvider::ReadDefaultSettings(bool overwrite) {
@@ -320,17 +314,6 @@ void PrefDefaultProvider::GetSettingsFromDictionary(
                        settings->settings[CONTENT_SETTINGS_TYPE_PLUGINS]);
 }
 
-void PrefDefaultProvider::NotifyObservers(
-    const ContentSettingsDetails& details) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (host_content_settings_map_ == NULL)
-    return;
-  NotificationService::current()->Notify(
-      chrome::NOTIFICATION_CONTENT_SETTINGS_CHANGED,
-      Source<HostContentSettingsMap>(host_content_settings_map_),
-      Details<const ContentSettingsDetails>(&details));
-}
-
 void PrefDefaultProvider::MigrateObsoleteNotificationPref() {
   if (prefs_->HasPrefPath(prefs::kDesktopNotificationDefaultContentSetting)) {
     ContentSetting setting = IntToContentSetting(
diff --git a/chrome/browser/content_settings/content_settings_pref_provider.h b/chrome/browser/content_settings/content_settings_pref_provider.h
index d5d0d3e..59ce76d 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider.h
+++ b/chrome/browser/content_settings/content_settings_pref_provider.h
@@ -33,11 +33,10 @@ namespace content_settings {
 
 // Content settings provider that provides default content settings based on
 // user prefs.
-class PrefDefaultProvider : public DefaultProviderInterface,
+class PrefDefaultProvider : public ObservableDefaultProvider,
                             public NotificationObserver {
  public:
-  PrefDefaultProvider(HostContentSettingsMap* map,
-                      PrefService* prefs,
+  PrefDefaultProvider(PrefService* prefs,
                       bool incognito);
   virtual ~PrefDefaultProvider();
 
@@ -58,12 +57,6 @@ class PrefDefaultProvider : public DefaultProviderInterface,
                        const NotificationDetails& details);
 
  private:
-  // Informs observers that content settings have changed. Make sure that
-  // |lock_| is not held when calling this, as listeners will usually call one
-  // of the GetSettings functions in response, which would then lead to a
-  // mutex deadlock.
-  void NotifyObservers(const ContentSettingsDetails& details);
-
   // Sets the fields of |settings| based on the values in |dictionary|.
   void GetSettingsFromDictionary(const base::DictionaryValue* dictionary,
                                  ContentSettings* settings);
@@ -81,7 +74,6 @@ class PrefDefaultProvider : public DefaultProviderInterface,
   // Copies of the pref data, so that we can read it on the IO thread.
   ContentSettings default_content_settings_;
 
-  HostContentSettingsMap* host_content_settings_map_;
   PrefService* prefs_;
 
   // Whether this settings map is for an Incognito session.
diff --git a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
index 181afac..c05422c 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
@@ -7,7 +7,6 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "chrome/browser/content_settings/content_settings_mock_observer.h"
-#include "chrome/browser/content_settings/mock_settings_observer.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/prefs/default_pref_store.h"
 #include "chrome/browser/prefs/incognito_user_pref_store.h"
@@ -32,9 +31,7 @@ class PrefDefaultProviderTest : public TestingBrowserProcessTest {
  public:
   PrefDefaultProviderTest()
       : ui_thread_(BrowserThread::UI, &message_loop_),
-        provider_(profile_.GetHostContentSettingsMap(),
-                  profile_.GetPrefs(),
-                  false) {
+        provider_(profile_.GetPrefs(), false) {
   }
   ~PrefDefaultProviderTest() {
     provider_.ShutdownOnUIThread();
@@ -61,18 +58,14 @@ TEST_F(PrefDefaultProviderTest, DefaultValues) {
 }
 
 TEST_F(PrefDefaultProviderTest, Observer) {
-  MockSettingsObserver observer;
-
-  EXPECT_CALL(observer,
-              OnContentSettingsChanged(profile_.GetHostContentSettingsMap(),
-                                       CONTENT_SETTINGS_TYPE_IMAGES, false,
-                                       _, _, true));
-  // Expect a second call because the PrefDefaultProvider in the TestingProfile
-  // also observes the default content settings preference.
-  EXPECT_CALL(observer,
-              OnContentSettingsChanged(profile_.GetHostContentSettingsMap(),
-                                       CONTENT_SETTINGS_TYPE_DEFAULT, true,
-                                       _, _, true));
+  MockObserver mock_observer;
+  EXPECT_CALL(mock_observer,
+              OnContentSettingChanged(_,
+                                      _,
+                                      CONTENT_SETTINGS_TYPE_IMAGES,
+                                      ""));
+  provider_.AddObserver(&mock_observer);
+
   provider_.UpdateDefaultSetting(
       CONTENT_SETTINGS_TYPE_IMAGES, CONTENT_SETTING_BLOCK);
 }
@@ -105,8 +98,7 @@ TEST_F(PrefDefaultProviderTest, ObserveDefaultPref) {
 }
 
 TEST_F(PrefDefaultProviderTest, OffTheRecord) {
-  PrefDefaultProvider otr_provider(profile_.GetHostContentSettingsMap(),
-                                   profile_.GetPrefs(),
+  PrefDefaultProvider otr_provider(profile_.GetPrefs(),
                                    true);
 
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
@@ -125,7 +117,7 @@ TEST_F(PrefDefaultProviderTest, OffTheRecord) {
 
   // Changing content settings on the incognito provider should be ignored.
   otr_provider.UpdateDefaultSetting(CONTENT_SETTINGS_TYPE_COOKIES,
-                                   CONTENT_SETTING_ALLOW);
+                                    CONTENT_SETTING_ALLOW);
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             provider_.ProvideDefaultSetting(CONTENT_SETTINGS_TYPE_COOKIES));
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
diff --git a/chrome/browser/content_settings/host_content_settings_map.cc b/chrome/browser/content_settings/host_content_settings_map.cc
index 2607007..8aee58b 100644
--- a/chrome/browser/content_settings/host_content_settings_map.cc
+++ b/chrome/browser/content_settings/host_content_settings_map.cc
@@ -78,11 +78,15 @@ HostContentSettingsMap::HostContentSettingsMap(
   // The order in which the default content settings providers are created is
   // critical, as providers that are further down in the list (i.e. added later)
   // override providers further up.
+  content_settings::PrefDefaultProvider* default_provider =
+      new content_settings::PrefDefaultProvider(prefs_, is_off_the_record_);
+  default_provider->AddObserver(this);
   default_content_settings_providers_.push_back(
-      make_linked_ptr(new content_settings::PrefDefaultProvider(
-          this, prefs_, is_off_the_record_)));
-  content_settings::DefaultProviderInterface* policy_default_provider =
-      new content_settings::PolicyDefaultProvider(this, prefs_);
+      make_linked_ptr(default_provider));
+
+  content_settings::PolicyDefaultProvider* policy_default_provider =
+      new content_settings::PolicyDefaultProvider(prefs_);
+  policy_default_provider->AddObserver(this);
   default_content_settings_providers_.push_back(
       make_linked_ptr(policy_default_provider));