Add PathService::RemoveOverride to clear path overrides.

This is especially useful for unit tests because they live in the same process
and share all the singletons including PathService. One can of course override
the overrides but it is much cleaner and sometimes the only good solution to
return to the original PathProvider instead.

BUG=149161
TEST=base_unittests:PathServiceTest.*


Review URL: https://chromiumcodereview.appspot.com/10909228

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

1 files changed, 59 insertions, 43 deletions

diff --git a/base/path_service.cc b/base/path_service.cc
index 7a696e9..2697653 100644
--- a/base/path_service.cc
+++ b/base/path_service.cc
@@ -142,14 +142,8 @@ static PathData* GetPathData() {
   return g_path_data.Pointer();
 }
 
-}  // namespace
-
-
-// static
-bool PathService::GetFromCache(int key, FilePath* result) {
-  PathData* path_data = GetPathData();
-  base::AutoLock scoped_lock(path_data->lock);
-
+// Tries to find |key| in the cache. |path_data| should be locked by the caller!
+bool LockedGetFromCache(int key, const PathData* path_data, FilePath* result) {
   // check for a cached version
   PathMap::const_iterator it = path_data->cache.find(key);
   if (it != path_data->cache.end()) {
@@ -159,27 +153,20 @@ bool PathService::GetFromCache(int key, FilePath* result) {
   return false;
 }
 
-// static
-bool PathService::GetFromOverrides(int key, FilePath* result) {
-  PathData* path_data = GetPathData();
-  base::AutoLock scoped_lock(path_data->lock);
-
+// Tries to find |key| in the overrides map. |path_data| should be locked by the
+// caller!
+bool LockedGetFromOverrides(int key, PathData* path_data, FilePath* result) {
   // check for an overridden version.
   PathMap::const_iterator it = path_data->overrides.find(key);
   if (it != path_data->overrides.end()) {
+    path_data->cache[key] = it->second;
     *result = it->second;
     return true;
   }
   return false;
 }
 
-// static
-void PathService::AddToCache(int key, const FilePath& path) {
-  PathData* path_data = GetPathData();
-  base::AutoLock scoped_lock(path_data->lock);
-  // Save the computed path in our cache.
-  path_data->cache[key] = path;
-}
+}  // namespace
 
 // TODO(brettw): this function does not handle long paths (filename > MAX_PATH)
 // characters). This isn't supported very well by Windows right now, so it is
@@ -195,18 +182,23 @@ bool PathService::Get(int key, FilePath* result) {
   if (key == base::DIR_CURRENT)
     return file_util::GetCurrentDirectory(result);
 
-  if (GetFromCache(key, result))
-    return true;
+  Provider* provider = NULL;
+  {
+    base::AutoLock scoped_lock(path_data->lock);
+    if (LockedGetFromCache(key, path_data, result))
+      return true;
 
-  if (GetFromOverrides(key, result))
-    return true;
+    if (LockedGetFromOverrides(key, path_data, result))
+      return true;
+
+    // Get the beginning of the list while it is still locked.
+    provider = path_data->providers;
+  }
 
   FilePath path;
 
-  // search providers for the requested path
-  // NOTE: it should be safe to iterate here without the lock
-  // since RegisterProvider always prepends.
-  Provider* provider = path_data->providers;
+  // Iterating does not need the lock because only the list head might be
+  // modified on another thread.
   while (provider) {
     if (provider->func(key, &path))
       break;
@@ -217,17 +209,21 @@ bool PathService::Get(int key, FilePath* result) {
   if (path.empty())
     return false;
 
-  AddToCache(key, path);
-
   *result = path;
+
+  base::AutoLock scoped_lock(path_data->lock);
+  path_data->cache[key] = path;
+
   return true;
 }
 
+// static
 bool PathService::Override(int key, const FilePath& path) {
   // Just call the full function with true for the value of |create|.
   return OverrideAndCreateIfNeeded(key, path, true);
 }
 
+// static
 bool PathService::OverrideAndCreateIfNeeded(int key,
                                             const FilePath& path,
                                             bool create) {
@@ -260,38 +256,58 @@ bool PathService::OverrideAndCreateIfNeeded(int key,
   // on the value we are overriding, and are now out of sync with reality.
   path_data->cache.clear();
 
-  path_data->cache[key] = file_path;
   path_data->overrides[key] = file_path;
 
   return true;
 }
 
+// static
+bool PathService::RemoveOverride(int key) {
+  PathData* path_data = GetPathData();
+  DCHECK(path_data);
+
+  base::AutoLock scoped_lock(path_data->lock);
+
+  if (path_data->overrides.find(key) == path_data->overrides.end())
+    return false;
+
+  // Clear the cache now. Some of its entries could have depended on the value
+  // we are going to remove, and are now out of sync.
+  path_data->cache.clear();
+
+  path_data->overrides.erase(key);
+
+  return true;
+}
+
+// static
 void PathService::RegisterProvider(ProviderFunc func, int key_start,
                                    int key_end) {
   PathData* path_data = GetPathData();
   DCHECK(path_data);
   DCHECK_GT(key_end, key_start);
 
-  base::AutoLock scoped_lock(path_data->lock);
-
   Provider* p;
 
-#ifndef NDEBUG
-  p = path_data->providers;
-  while (p) {
-    DCHECK(key_start >= p->key_end || key_end <= p->key_start) <<
-      "path provider collision";
-    p = p->next;
-  }
-#endif
-
   p = new Provider;
   p->is_static = false;
   p->func = func;
-  p->next = path_data->providers;
 #ifndef NDEBUG
   p->key_start = key_start;
   p->key_end = key_end;
 #endif
+
+  base::AutoLock scoped_lock(path_data->lock);
+
+#ifndef NDEBUG
+  Provider *iter = path_data->providers;
+  while (iter) {
+    DCHECK(key_start >= iter->key_end || key_end <= iter->key_start) <<
+      "path provider collision";
+    iter = iter->next;
+  }
+#endif
+
+  p->next = path_data->providers;
   path_data->providers = p;
 }