diff options
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 = |