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

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

Index: chrome/common/extensions/docs/server2/chained_compiled_file_system.py
===================================================================
--- chrome/common/extensions/docs/server2/chained_compiled_file_system.py	(revision 0)
+++ chrome/common/extensions/docs/server2/chained_compiled_file_system.py	(revision 0)
@@ -0,0 +1,61 @@
+# Copyright 2013 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+from compiled_file_system import CompiledFileSystem
+from file_system import FileNotFoundError
+
+class ChainedCompiledFileSystem(object):
+  ''' A CompiledFileSystem implementation that fetches data from a chain of
+  CompiledFileSystems.
+
+  The rules for the compiled file system chain are:
+    - Versions are fetched from the first compiled file system's underlying
+      file system.
+    - Each compiled file system is read in the reverse order (the last one is
+      read first). If the version matches, return the data. Otherwise, read
+      from the previous compiled file system until the first one is read.
+
+  It is used to chain two compiled file systems whose underlying file systems
+  are slightly different. This makes it possible to reuse cached compiled data

[not at google - send to devlin, 2013/05/12 03:28:27]

Nice comment, thanks.

But - s/two compiled file systems/compiled file systems/

(no "two")

[方觉(Fang Jue), 2013/05/12 04:22:07]

Done.

+  in one of them without recompiling everything that is shared by them.
+  '''
+  class Factory(CompiledFileSystem.Factory):
+    def __init__(self,
+                 factory_chain):
+      self._factory_chain = factory_chain
+
+    def Create(self, populate_function, cls, category=None):
+      return ChainedCompiledFileSystem(
+          [factory.Create(populate_function, cls, category)
+           for factory in self._factory_chain])
+
+  def __init__(self, compiled_fs_chain):
+    assert len(compiled_fs_chain) > 0
+    self._compiled_fs_chain = compiled_fs_chain
+
+  def GetFromFile(self, path, binary=False):
+    # It's possible that a new file is added in the first compiled file system
+    # and it doesn't exist in other compiled file systems.
+    try:
+      version = self._compiled_fs_chain[0]._file_system.Stat(path).version
+      for compiled_fs in reversed(self._compiled_fs_chain):
+        cache_entry = compiled_fs._GetCacheEntryFromFile(path, binary)

[not at google - send to devlin, 2013/05/12 03:28:27]

Is the version stuff necessary if you check them in the other order, and
implement that thing in PatchedFileSystem where you raise a
FileNotFoundException that I mentioned?

If it's *really* necessary to do this then you should inject file_system (like
you had before - I was operating under the assumption in the last paragraph) and
actually exposed a GetCachedVersion or something method on CompiledFileSystem.

[方觉(Fang Jue), 2013/05/12 04:22:07]

Yes. It's necessary. version is used to determine whether the file in
compiled_fs_chain[0] is different from that in other compiled fs. If it is the
same, data cached in other compiled fs can be reused. Otherwise, it has to be
read from compiled_fs_chain[0].

If checked in the other order (I suppose you mean from the first to the last?),
why is this class necessary instead of directly using the first compiled fs?

And RietveldPatcher.GetVersion does not return None now (it throws an exception
instead). Maybe add an assert version is not None in PatchedFileSystem?
If I inject file_system, I must inject object_store_creator at the same time
(CompiledFileSystem.Factory has two parameters!).

_GetCacheEntryFromFile is almost the same as GetFromFile except for it returns
cache_entry instead of cache_entry.cache_data (we need cache_entry.version). It
probably makes others confused and think that it only returns cached data and
never actually read from file system but I can't find a better name.

[not at google - send to devlin, 2013/05/12 05:11:41]

Ok, never mind, I found a problem with what I was thinking anyway.
Sure, not what I meant, but this sounds good.
I meant both file system and the compiled file systems separately. That would
mean less magic in querying compiled_fs_chain[0]._file_system which is bad
because (a) assuming something about compiled_fs_chain[0] and (b) accessing
private member of a compiled file system.
I'm just saying elevate it from private to public if you need to access it. Make
CompiledFileSystem have GetFromFile, GetFromFileListing, and GetVersion ("gets
the version of the data that would be returned from calling either
GetFromFile/GetFromFileListing with |path|").

+        if cache_entry.version == version:
+          return cache_entry.cache_data
+    except FileNotFoundError:
+      pass
+    return self._compiled_fs_chain[0].GetFromFile(path, binary)

[not at google - send to devlin, 2013/05/12 03:28:27]

Coud you add the comment here and below "Try first operation again to generate
the correct stack trace".

[方觉(Fang Jue), 2013/05/12 04:22:07]

Done.

+
+  def GetFromFileListing(self, path):
+    if not path.endswith('/'):
+      path += '/'
+    try:
+      version = self._compiled_fs_chain[0]._file_system.Stat(path).version
+      for compiled_fs in reversed(self._compiled_fs_chain):
+        cache_entry = compiled_fs._GetCacheEntryFromFileListing(path)
+        if cache_entry.version == version:
+          return cache_entry.cache_data
+    except FileNotFoundError:
+      pass
+    return self._compiled_fs_chain[0].GetFromFileListing(path)
Property changes on: chrome/common/extensions/docs/server2/chained_compiled_file_system.py
___________________________________________________________________
Added: svn:eol-style
   + LF