Docserver: Add support for viewing docs with a codereview patch applied

chrome/common/extensions/docs/server2/compiled_file_system.py

Index: chrome/common/extensions/docs/server2/compiled_file_system.py
===================================================================
--- chrome/common/extensions/docs/server2/compiled_file_system.py	(revision 198562)
+++ chrome/common/extensions/docs/server2/compiled_file_system.py	(working copy)
@@ -2,9 +2,11 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from file_system import FileNotFoundError
+
 class _CacheEntry(object):
   def __init__(self, cache_data, version):
-    self._cache_data = cache_data
+    self.cache_data = cache_data
     self.version = version
 
 class CompiledFileSystem(object):
@@ -13,9 +15,20 @@
   class Factory(object):
     """A class to build a CompiledFileSystem backed by |file_system|.
     """
-    def __init__(self, file_system, object_store_creator_factory):
+    def __init__(self,
+                 file_system,
+                 object_store_creator_factory,
+                 patched_file_system=None,
+                 patched_object_store_creator_factory=None):

[not at google - send to devlin, 2013/05/07 05:45:40]

this class is conceptually quite simple and now... it isn't. such a generic
class shouldn't have knowledge of server implementation details such as there
being such a thing as a "patched file system".

is there a different way you can achieve what you're going for? which is...
using the old data for the stuff which hasn't been patched, right?

like I said... is it really such a big deal? how much difference does it make?
can you measure it beyond just the first load (which I wouldn't expect to be so
bad since you can use the existing data anyway)? is it just a matter of saving
datastore space, because we really do have infinity storage basically.

[方觉(Fang Jue), 2013/05/07 06:03:55]

It's definitely not a good way to achieve it. What I think would be better is to
add a class called ChainedCompiledFileSystem (or something like that) and handle
these things there. I'm still thinking about how to do that.

For your 'storagetest' patch, without this change the first load takes about 15
seconds. With this change, it usually takes less than 5 seconds. So it's not a
small deal.

[not at google - send to devlin, 2013/05/07 06:14:31]

That sounds interesting, though it seems to me like the problem is at the
FileSystem not the CompiledFileSystem level, i.e. what sounds more promising is
a CacheChainFilleSystem or something (using the same analogy). I wouldn't want
to modify CachingFileSystem at all - it's already complex enough - so maybe just
a ChainedFileSystem would be enough, which tries one after another (it would be
constructed with the patched then the compiled obviously). Make
PatchedFileSystem throw FileNotFound errors for files it doesn't have
modifications for (hm, though you'd need to distinguish between that and the
file being removed).

Anyway, I'll leave it to you. Getting late here, brain not 100%.
Ok. Are subsequent loads the same? I presume so. What about loads after there
has been a patch update? That is an important question. I would expect them to
be quite comparable to each other, very quick.

+      assert ((patched_file_system is None and
+               patched_object_store_creator_factory is None) or
+              (patched_file_system is not None and
+               patched_object_store_creator_factory is not None))
       self._file_system = file_system
       self._object_store_creator_factory = object_store_creator_factory
+      self._patched_file_system = patched_file_system
+      self._patched_object_store_creator_factory = (
+          patched_object_store_creator_factory)
 
     def Create(self, populate_function, cls, category=None):
       """Create a CompiledFileSystem that populates the cache by calling
@@ -31,11 +44,22 @@
         full_name = '%s/%s' % (full_name, category)
       object_store_creator = self._object_store_creator_factory.Create(
           CompiledFileSystem)
-      return CompiledFileSystem(
+      base_compiled_fs = CompiledFileSystem(
           self._file_system,
           populate_function,
           object_store_creator.Create(category='%s/file' % full_name),
           object_store_creator.Create(category='%s/list' % full_name))
+      if self._patched_file_system:
+        patched_object_store_creator = (self.
+            _patched_object_store_creator_factory.Create(CompiledFileSystem))
+        return CompiledFileSystem(
+            self._patched_file_system,
+            populate_function,
+            patched_object_store_creator.Create(category='%s/file' % full_name),
+            patched_object_store_creator.Create(category='%s/list' % full_name),
+            base_compiled_fs)
+      else:
+        return base_compiled_fs
 
     def CreateIdentity(self, cls):
       '''Handy helper to get or create the identity compiled file system.
@@ -48,11 +72,13 @@
                file_system,
                populate_function,
                file_object_store,
-               list_object_store):
+               list_object_store,
+               base_compiled_fs=None):
     self._file_system = file_system
     self._populate_function = populate_function
     self._file_object_store = file_object_store
     self._list_object_store = list_object_store
+    self._base_compiled_fs = base_compiled_fs
 
   def _RecursiveList(self, path):
     files = []
@@ -70,15 +96,30 @@
     apply for the first time the file is fetched; if already cached, |binary|
     will be ignored.
     """
+    if self._base_compiled_fs:
+      # It's possible that a new file is added in this compiled file system
+      # and it doesn't exist in base_compiled_fs.
+      try:
+        version = self._file_system.Stat(path).version
+        cache_entry = self._base_compiled_fs._GetCacheEntryFromFile(path,
+                                                                    binary)
+        if version == cache_entry.version:
+          return cache_entry.cache_data
+      except FileNotFoundError:
+        pass
+    return self._GetCacheEntryFromFile(path, binary).cache_data
+
+  def _GetCacheEntryFromFile(self, path, binary=False):
     version = self._file_system.Stat(path).version
     cache_entry = self._file_object_store.Get(path).Get()
     if (cache_entry is not None) and (version == cache_entry.version):
-      return cache_entry._cache_data
+      return cache_entry
     cache_data = self._populate_function(
         path,
         self._file_system.ReadSingle(path, binary=binary))
-    self._file_object_store.Set(path, _CacheEntry(cache_data, version))
-    return cache_data
+    cache_entry = _CacheEntry(cache_data, version)
+    self._file_object_store.Set(path, cache_entry)
+    return cache_entry
 
   def GetFromFileListing(self, path):
     """Calls |populate_function| on the listing of the files at |path|.
@@ -86,10 +127,24 @@
     """
     if not path.endswith('/'):
       path += '/'
+    if self._base_compiled_fs:
+      try:
+        version = self._file_system.Stat(path).version
+        cache_entry = self._base_compiled_fs._GetCacheEntryFromFileListing(
+            path)
+        if version == cache_entry.version:
+          return cache_entry.cache_data
+      except FileNotFoundError:
+        pass
+    return self._GetCacheEntryFromFileListing(path).cache_data
+
+  def _GetCacheEntryFromFileListing(self, path):
+    assert path.endswith('/')
     version = self._file_system.Stat(path).version
     cache_entry = self._list_object_store.Get(path).Get()
     if (cache_entry is not None) and (version == cache_entry.version):
-        return cache_entry._cache_data
+        return cache_entry
     cache_data = self._populate_function(path, self._RecursiveList(path))
-    self._list_object_store.Set(path, _CacheEntry(cache_data, version))
-    return cache_data
+    cache_entry = _CacheEntry(cache_data, version)
+    self._list_object_store.Set(path, cache_entry)
+    return cache_entry