Simplify the geolocation content settings map: it's only ever used from the UI thread, so no need for it to cache a copy of the prefs values

BUG=none
TEST=GeolocationContentSettingsMapTests.*

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

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

3 files changed, 98 insertions, 218 deletions

diff --git a/chrome/browser/geolocation/geolocation_content_settings_map.cc b/chrome/browser/geolocation/geolocation_content_settings_map.cc
index 7bae5d2..da9739d 100755
--- a/chrome/browser/geolocation/geolocation_content_settings_map.cc
+++ b/chrome/browser/geolocation/geolocation_content_settings_map.cc
@@ -2,6 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Implementation of the geolocation content settings map. Styled on
+// HostContentSettingsMap however unlike that class, this one does not hold
+// an additional in-memory copy of the settings as it does not need to support
+// thread safe synchronous access to the settings; all geolocation permissions
+// are read and written in the UI thread. (If in future this is no longer the
+// case, refer to http://codereview.chromium.org/1525018 for a previous version
+// with caching. Note that as we must observe the prefs store for settings
+// changes, e.g. coming from the sync engine, the simplest design would be to
+// always write-through changes straight to the prefs store, and rely on the
+// notification observer to subsequently update any cached copy).
+
 #include "chrome/browser/geolocation/geolocation_content_settings_map.h"
 
 #include "base/utf_string_conversions.h"
@@ -21,19 +32,8 @@ const ContentSetting
     GeolocationContentSettingsMap::kDefaultSetting = CONTENT_SETTING_ASK;
 
 GeolocationContentSettingsMap::GeolocationContentSettingsMap(Profile* profile)
-    : profile_(profile), updating_preferences_(false) {
-  PrefService* prefs = profile_->GetPrefs();
-
-  // Read global defaults.
-  default_content_setting_ = IntToContentSetting(
-      prefs->GetInteger(prefs::kGeolocationDefaultContentSetting));
-  if (default_content_setting_ == CONTENT_SETTING_DEFAULT)
-    default_content_setting_ = kDefaultSetting;
-
-  // Read exceptions from the preference service.
-  ReadExceptions();
-
-  prefs->AddPrefObserver(prefs::kGeolocationContentSettings, this);
+    : profile_(profile) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 }
 
 // static
@@ -53,49 +53,78 @@ std::string GeolocationContentSettingsMap::OriginToString(const GURL& origin) {
 }
 
 ContentSetting GeolocationContentSettingsMap::GetDefaultContentSetting() const {
-  AutoLock auto_lock(lock_);
-  return default_content_setting_;
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  const PrefService* prefs = profile_->GetPrefs();
+  const ContentSetting default_content_setting = IntToContentSetting(
+      prefs->GetInteger(prefs::kGeolocationDefaultContentSetting));
+  return default_content_setting == CONTENT_SETTING_DEFAULT ?
+         kDefaultSetting : default_content_setting;
 }
 
 ContentSetting GeolocationContentSettingsMap::GetContentSetting(
     const GURL& requesting_url,
     const GURL& embedding_url) const {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(requesting_url.is_valid() && embedding_url.is_valid());
   GURL requesting_origin(requesting_url.GetOrigin());
   GURL embedding_origin(embedding_url.GetOrigin());
   DCHECK(requesting_origin.is_valid() && embedding_origin.is_valid());
-  AutoLock auto_lock(lock_);
-  AllOriginsSettings::const_iterator i(content_settings_.find(
-      requesting_origin));
-  if (i != content_settings_.end()) {
-    OneOriginSettings::const_iterator j(i->second.find(embedding_origin));
-    if (j != i->second.end())
-      return j->second;
-    if (requesting_origin != embedding_origin) {
-      OneOriginSettings::const_iterator any_embedder(i->second.find(GURL()));
-      if (any_embedder != i->second.end())
-        return any_embedder->second;
+
+  const DictionaryValue* all_settings_dictionary =
+      profile_->GetPrefs()->GetDictionary(prefs::kGeolocationContentSettings);
+  // Careful: The returned value could be NULL if the pref has never been set.
+  if (all_settings_dictionary != NULL) {
+    DictionaryValue* requesting_origin_settings;
+    if (all_settings_dictionary->GetDictionaryWithoutPathExpansion(
+        UTF8ToWide(requesting_origin.spec()), &requesting_origin_settings)) {
+      int setting;
+      if (requesting_origin_settings->GetIntegerWithoutPathExpansion(
+          UTF8ToWide(embedding_origin.spec()), &setting))
+        return IntToContentSetting(setting);
+      // Check for any-embedder setting
+      if (requesting_origin != embedding_origin &&
+          requesting_origin_settings->GetIntegerWithoutPathExpansion(
+          L"", &setting))
+        return IntToContentSetting(setting);
     }
   }
-  return default_content_setting_;
+  return GetDefaultContentSetting();
 }
 
 GeolocationContentSettingsMap::AllOriginsSettings
     GeolocationContentSettingsMap::GetAllOriginsSettings() const {
-  AutoLock auto_lock(lock_);
-  return content_settings_;
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  AllOriginsSettings content_settings;
+  const DictionaryValue* all_settings_dictionary =
+      profile_->GetPrefs()->GetDictionary(prefs::kGeolocationContentSettings);
+  // Careful: The returned value could be NULL if the pref has never been set.
+  if (all_settings_dictionary != NULL) {
+    for (DictionaryValue::key_iterator i(all_settings_dictionary->begin_keys());
+         i != all_settings_dictionary->end_keys(); ++i) {
+      const std::wstring& wide_origin(*i);
+      GURL origin_as_url(WideToUTF8(wide_origin));
+      if (!origin_as_url.is_valid())
+        continue;
+      DictionaryValue* requesting_origin_settings_dictionary = NULL;
+      bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
+          wide_origin, &requesting_origin_settings_dictionary);
+      DCHECK(found);
+      if (!requesting_origin_settings_dictionary)
+        continue;
+      GetOneOriginSettingsFromDictionary(
+          requesting_origin_settings_dictionary,
+          &content_settings[origin_as_url]);
+    }
+  }
+  return content_settings;
 }
 
 void GeolocationContentSettingsMap::SetDefaultContentSetting(
     ContentSetting setting) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  {
-    AutoLock auto_lock(lock_);
-    default_content_setting_ =
-        (setting == CONTENT_SETTING_DEFAULT) ? kDefaultSetting : setting;
-  }
   profile_->GetPrefs()->SetInteger(prefs::kGeolocationDefaultContentSetting,
-                                   default_content_setting_);
+                                   setting == CONTENT_SETTING_DEFAULT ?
+                                       kDefaultSetting : setting);
 }
 
 void GeolocationContentSettingsMap::SetContentSetting(
@@ -107,141 +136,48 @@ void GeolocationContentSettingsMap::SetContentSetting(
   DCHECK(embedding_url.is_valid() || embedding_url.is_empty());
   GURL requesting_origin(requesting_url.GetOrigin());
   GURL embedding_origin(embedding_url.GetOrigin());
-  DCHECK(requesting_origin.is_valid() &&
-         (embedding_origin.is_valid() || embedding_url.is_empty()));
+  DCHECK(requesting_origin.is_valid());
+  DCHECK(embedding_origin.is_valid() || embedding_url.is_empty());
   std::wstring wide_requesting_origin(UTF8ToWide(requesting_origin.spec()));
   std::wstring wide_embedding_origin(UTF8ToWide(embedding_origin.spec()));
-  DictionaryValue* all_settings_dictionary =
-      profile_->GetPrefs()->GetMutableDictionary(
-          prefs::kGeolocationContentSettings);
-
-  updating_preferences_ = true;
-  {
-    ScopedPrefUpdate update(profile_->GetPrefs(),
-                            prefs::kGeolocationContentSettings);
-    AutoLock auto_lock(lock_);
-    DictionaryValue* requesting_origin_settings_dictionary;
-    all_settings_dictionary->GetDictionaryWithoutPathExpansion(
-        wide_requesting_origin, &requesting_origin_settings_dictionary);
-    if (setting == CONTENT_SETTING_DEFAULT) {
-      if (content_settings_.count(requesting_origin) &&
-          content_settings_[requesting_origin].count(embedding_origin)) {
-        if (content_settings_[requesting_origin].size() == 1) {
-          all_settings_dictionary->RemoveWithoutPathExpansion(
-              wide_requesting_origin, NULL);
-          content_settings_.erase(requesting_origin);
-        } else {
-          requesting_origin_settings_dictionary->RemoveWithoutPathExpansion(
-              wide_embedding_origin, NULL);
-          content_settings_[requesting_origin].erase(embedding_origin);
-        }
-      }
-    } else {
-      if (!content_settings_.count(requesting_origin)) {
-        requesting_origin_settings_dictionary = new DictionaryValue;
-        all_settings_dictionary->SetWithoutPathExpansion(
-            wide_requesting_origin, requesting_origin_settings_dictionary);
-      }
-      content_settings_[requesting_origin][embedding_origin] = setting;
-      DCHECK(requesting_origin_settings_dictionary);
-      requesting_origin_settings_dictionary->SetWithoutPathExpansion(
-          wide_embedding_origin, Value::CreateIntegerValue(setting));
-    }
-  }
-  updating_preferences_ = false;
-}
-
-void GeolocationContentSettingsMap::ClearOneRequestingOrigin(
-    const GURL& requesting_origin) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  DCHECK(requesting_origin.is_valid());
-
-  {
-    AutoLock auto_lock(lock_);
-    AllOriginsSettings::iterator i(content_settings_.find(requesting_origin));
-    if (i == content_settings_.end())
-      return;
-    content_settings_.erase(i);
-  }
-
   PrefService* prefs = profile_->GetPrefs();
-  DictionaryValue* all_settings_dictionary =
-      prefs->GetMutableDictionary(prefs::kGeolocationContentSettings);
-  updating_preferences_ = true;
-  {
-    ScopedPrefUpdate update(prefs, prefs::kGeolocationContentSettings);
-    all_settings_dictionary->RemoveWithoutPathExpansion(
-        UTF8ToWide(requesting_origin.spec()), NULL);
+  DictionaryValue* all_settings_dictionary = prefs->GetMutableDictionary(
+      prefs::kGeolocationContentSettings);
+  DCHECK(all_settings_dictionary);
+
+  ScopedPrefUpdate update(prefs, prefs::kGeolocationContentSettings);
+  DictionaryValue* requesting_origin_settings_dictionary = NULL;
+  all_settings_dictionary->GetDictionaryWithoutPathExpansion(
+      wide_requesting_origin, &requesting_origin_settings_dictionary);
+  if (setting == CONTENT_SETTING_DEFAULT) {
+    if (requesting_origin_settings_dictionary) {
+      requesting_origin_settings_dictionary->RemoveWithoutPathExpansion(
+          wide_embedding_origin, NULL);
+      if (requesting_origin_settings_dictionary->empty())
+        all_settings_dictionary->RemoveWithoutPathExpansion(
+            wide_requesting_origin, NULL);
+    }
+  } else {
+    if (!requesting_origin_settings_dictionary) {
+      requesting_origin_settings_dictionary = new DictionaryValue;
+      all_settings_dictionary->SetWithoutPathExpansion(
+          wide_requesting_origin, requesting_origin_settings_dictionary);
+    }
+    DCHECK(requesting_origin_settings_dictionary);
+    requesting_origin_settings_dictionary->SetWithoutPathExpansion(
+        wide_embedding_origin, Value::CreateIntegerValue(setting));
   }
-  updating_preferences_ = false;
 }
 
 void GeolocationContentSettingsMap::ResetToDefault() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
-  {
-    AutoLock auto_lock(lock_);
-    default_content_setting_ = kDefaultSetting;
-    content_settings_.clear();
-  }
-
   PrefService* prefs = profile_->GetPrefs();
-  updating_preferences_ = true;
-  {
-    prefs->ClearPref(prefs::kGeolocationDefaultContentSetting);
-    prefs->ClearPref(prefs::kGeolocationContentSettings);
-  }
-  updating_preferences_ = false;
-}
-
-void GeolocationContentSettingsMap::Observe(
-    NotificationType type,
-    const NotificationSource& source,
-    const NotificationDetails& details) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  DCHECK(NotificationType::PREF_CHANGED == type);
-  DCHECK_EQ(profile_->GetPrefs(), Source<PrefService>(source).ptr());
-  if (updating_preferences_)
-    return;
-
-  std::wstring* name = Details<std::wstring>(details).ptr();
-  if (prefs::kGeolocationContentSettings == *name) {
-   ReadExceptions();
-  } else {
-   NOTREACHED() << "Unexpected preference observed.";
-  }
+  prefs->ClearPref(prefs::kGeolocationDefaultContentSetting);
+  prefs->ClearPref(prefs::kGeolocationContentSettings);
 }
 
 GeolocationContentSettingsMap::~GeolocationContentSettingsMap() {
-  profile_->GetPrefs()->RemovePrefObserver(prefs::kGeolocationContentSettings,
-                                           this);
-}
-
-void GeolocationContentSettingsMap::ReadExceptions() {
-  PrefService* prefs = profile_->GetPrefs();
-  const DictionaryValue* all_settings_dictionary =
-      prefs->GetDictionary(prefs::kGeolocationContentSettings);
-  content_settings_.clear();
-  // Careful: The returned value could be NULL if the pref has never been set.
-  if (all_settings_dictionary != NULL) {
-    for (DictionaryValue::key_iterator i(
-             all_settings_dictionary->begin_keys());
-         i != all_settings_dictionary->end_keys(); ++i) {
-      const std::wstring& wide_origin(*i);
-      DictionaryValue* requesting_origin_settings_dictionary = NULL;
-      bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
-          wide_origin, &requesting_origin_settings_dictionary);
-      DCHECK(found);
-      GURL origin_as_url(WideToUTF8(wide_origin));
-      if (!origin_as_url.is_valid())
-        continue;
-      OneOriginSettings* requesting_origin_settings =
-          &content_settings_[origin_as_url];
-      GetOneOriginSettingsFromDictionary(
-          requesting_origin_settings_dictionary,
-          requesting_origin_settings);
-    }
-  }
 }
 
 // static
diff --git a/chrome/browser/geolocation/geolocation_content_settings_map.h b/chrome/browser/geolocation/geolocation_content_settings_map.h
index 82cffff..a5512de 100755
--- a/chrome/browser/geolocation/geolocation_content_settings_map.h
+++ b/chrome/browser/geolocation/geolocation_content_settings_map.h
@@ -14,14 +14,10 @@
 
 #include <map>
 #include <string>
-#include <utility>
-#include <vector>
 
 #include "base/basictypes.h"
-#include "base/lock.h"
 #include "base/ref_counted.h"
 #include "chrome/common/content_settings.h"
-#include "chrome/common/notification_observer.h"
 #include "googleurl/src/gurl.h"
 
 class DictionaryValue;
@@ -29,8 +25,7 @@ class PrefService;
 class Profile;
 
 class GeolocationContentSettingsMap
-    : public NotificationObserver,
-      public base::RefCountedThreadSafe<GeolocationContentSettingsMap> {
+    : public base::RefCountedThreadSafe<GeolocationContentSettingsMap> {
  public:
   typedef std::map<GURL, ContentSetting> OneOriginSettings;
   typedef std::map<GURL, OneOriginSettings> AllOriginsSettings;
@@ -45,7 +40,7 @@ class GeolocationContentSettingsMap
 
   // Returns the default setting.
   //
-  // This may be called on any thread.
+  // This should only be called on the UI thread.
   ContentSetting GetDefaultContentSetting() const;
 
   // Returns a single ContentSetting which applies to the given |requesting_url|
@@ -53,13 +48,14 @@ class GeolocationContentSettingsMap
   // setting for a top-level page, as opposed to a frame embedded in a page,
   // pass the page's URL for both arguments.
   //
-  // This may be called on any thread.  Both arguments should be valid GURLs.
+  // This should only be called on the UI thread.
+  // Both arguments should be valid GURLs.
   ContentSetting GetContentSetting(const GURL& requesting_url,
                                    const GURL& embedding_url) const;
 
   // Returns the settings for all origins with any non-default settings.
   //
-  // This may be called on any thread.
+  // This should only be called on the UI thread.
   AllOriginsSettings GetAllOriginsSettings() const;
 
   // Sets the default setting.
@@ -83,25 +79,11 @@ class GeolocationContentSettingsMap
                          const GURL& embedding_url,
                          ContentSetting setting);
 
-  // Clears all settings for |requesting_origin|.  Note: Unlike in the functions
-  // above, this is expected to be an origin, not some URL of which we'll take
-  // the origin; this is to prevent ambiguity where callers could think they're
-  // clearing something wider or narrower than they really are.
-  //
-  // This should only be called on the UI thread.  |requesting_origin| should be
-  // a valid GURL.
-  void ClearOneRequestingOrigin(const GURL& requesting_origin);
-
   // Resets all settings.
   //
   // This should only be called on the UI thread.
   void ResetToDefault();
 
-  // NotificationObserver implementation.
-  virtual void Observe(NotificationType type,
-                       const NotificationSource& source,
-                       const NotificationDetails& details);
-
  private:
   friend class base::RefCountedThreadSafe<GeolocationContentSettingsMap>;
 
@@ -110,9 +92,6 @@ class GeolocationContentSettingsMap
 
   ~GeolocationContentSettingsMap();
 
-  // Reads the exceptions from the preference service.
-  void ReadExceptions();
-
   // Sets the fields of |one_origin_settings| based on the values in
   // |dictionary|.
   static void GetOneOriginSettingsFromDictionary(
@@ -122,17 +101,6 @@ class GeolocationContentSettingsMap
   // The profile we're associated with.
   Profile* profile_;
 
-  // Copies of the pref data, so that we can read it on the IO thread.
-  ContentSetting default_content_setting_;
-  AllOriginsSettings content_settings_;
-
-  // Used around accesses to the settings objects to guarantee thread safety.
-  mutable Lock lock_;
-
-  // Whether we are currently updating preferences, this is used to ignore
-  // notifications from the preference service that we triggered ourself.
-  bool updating_preferences_;
-
   DISALLOW_COPY_AND_ASSIGN(GeolocationContentSettingsMap);
 };
 
diff --git a/chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc b/chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
index 98202ce..defba3a 100755
--- a/chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
+++ b/chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
@@ -123,30 +123,6 @@ TEST_F(GeolocationContentSettingsMapTests, SetContentSettingDefault) {
   EXPECT_EQ(CONTENT_SETTING_ASK, map->GetContentSetting(top_level, top_level));
 }
 
-TEST_F(GeolocationContentSettingsMapTests, ClearOneOrigin) {
-  TestingProfile profile;
-  GeolocationContentSettingsMap* map =
-      profile.GetGeolocationContentSettingsMap();
-  GURL requester_0("http://www.iframe0.com/foo/bar");
-  GURL requester_1("http://www.iframe1.com/foo/bar");
-  GURL embedder_0("http://www.toplevel0.com/foo/bar");
-  map->SetContentSetting(requester_0, embedder_0, CONTENT_SETTING_ALLOW);
-  map->SetContentSetting(requester_1, embedder_0, CONTENT_SETTING_ALLOW);
-  EXPECT_EQ(CONTENT_SETTING_ALLOW,
-            map->GetContentSetting(requester_0, embedder_0));
-  EXPECT_EQ(CONTENT_SETTING_ALLOW,
-            map->GetContentSetting(requester_1, embedder_0));
-
-  map->ClearOneRequestingOrigin(requester_0.GetOrigin());
-  EXPECT_EQ(CONTENT_SETTING_ASK,
-            map->GetContentSetting(requester_0, embedder_0));
-
-  // Passing a non-origin shouldn't do anything.
-  map->ClearOneRequestingOrigin(requester_1);
-  EXPECT_EQ(CONTENT_SETTING_ALLOW,
-            map->GetContentSetting(requester_1, embedder_0));
-}
-
 TEST_F(GeolocationContentSettingsMapTests, Reset) {
   TestingProfile profile;
   GeolocationContentSettingsMap* map =