Docserver: make file_system a property of Create

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

 # 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
 from future import Gettable, Future
 
 
 class ChainedCompiledFileSystem(object):
   ''' A CompiledFileSystem implementation that fetches data from a chain of
   CompiledFileSystems that have different file systems and separate cache
   namespaces.
 
   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 compiled file systems whose underlying file systems are
   slightly different. This makes it possible to reuse cached compiled data in
   one of them without recompiling everything that is shared by them.
   '''
   class Factory(CompiledFileSystem.Factory):
-    def __init__(self,
-                 factory_and_fs_chain):
-      self._factory_and_fs_chain = factory_and_fs_chain
+    def __init__(self, file_system_chain, object_store):
+      self._file_system_chain = file_system_chain
+      self._object_store = object_store
 
-    def Create(self, populate_function, cls, category=None):
-      return ChainedCompiledFileSystem(
-          [(factory.Create(populate_function, cls, category), fs)
-           for factory, fs in self._factory_and_fs_chain])
+    def Create(self, file_system, populate_function, cls, category=None):
+      return ChainedCompiledFileSystem(tuple(
+          CompiledFileSystem.Factory(self._object_store).Create(
+              fs, populate_function, cls, category=category)
+          for fs in [file_system] + self._file_system_chain))

[Jeffrey Yasskin, 2013/10/14 18:52:31]

This seems a little odd. Why do we want to pass the chain of filesystems in two
different places? Does the first element vary while the rest are constant, in
actual uses? If so, comment that. If this is an interface-matching thing,
comment that. Otherwise, I think you should pass the whole thing in either the
Factory constructor or the Create() function.

[not at google - send to devlin, 2013/10/14 21:03:34]

I'll clarify this in the docstring. Updated some of the other comments in this
file too.

 
   def __init__(self, compiled_fs_chain):
     '''|compiled_fs_chain| is a list of tuples (compiled_fs, file_system).
     '''
     assert len(compiled_fs_chain) > 0
     self._compiled_fs_chain = compiled_fs_chain
 
   def GetFromFile(self, path, binary=False):
     return self._GetImpl(
         path,
         lambda compiled_fs: compiled_fs.GetFromFile(path, binary=binary),
-        lambda compiled_fs: compiled_fs.StatFile(path))
+        lambda compiled_fs: compiled_fs.GetFileVersion(path))
 
   def GetFromFileListing(self, path):
     if not path.endswith('/'):
       path += '/'
     return self._GetImpl(
         path,
         lambda compiled_fs: compiled_fs.GetFromFileListing(path),
-        lambda compiled_fs: compiled_fs.StatFileListing(path))
+        lambda compiled_fs: compiled_fs.GetFileListingVersion(path))
 
-  def _GetImpl(self, path, reader, statter):
+  def _GetImpl(self, path, reader, version_getter):
     # 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.
     read_futures = [(reader(compiled_fs), compiled_fs)
-                    for compiled_fs, _ in self._compiled_fs_chain]
+                    for compiled_fs in self._compiled_fs_chain]
 
     def resolve():
       try:
-        first_compiled_fs, first_file_system = self._compiled_fs_chain[0]
+        first_compiled_fs = self._compiled_fs_chain[0]
         # The first file system contains both files of a newer version and
         # files shared with other compiled file systems. We are going to try
         # each compiled file system in the reverse order and return the data
         # when version matches. Data cached in other compiled file system will
         # be reused whenever possible so that we don't need to recompile things
         # that are not changed across these file systems.
-        version = first_file_system.Stat(path).version
+        first_version = first_compiled_fs._file_system.Stat(path).version

[Jeffrey Yasskin, 2013/10/14 18:52:31]

Hm, accessing a protected field is a yellow flag: does it make sense to make
this a public accessor?

[not at google - send to devlin, 2013/10/14 21:03:34]

I swear I changed this to use version_getter(..) before. Oops. Thanks for
catching.

         for read_future, compiled_fs in reversed(read_futures):
-          if statter(compiled_fs) == version:
+          if version_getter(compiled_fs) == first_version:
             return read_future.Get()
       except FileNotFoundError:
         pass
       # Try an arbitrary operation again to generate a realistic stack trace.
       return read_futures[0][0].Get()
 
     return Future(delegate=Gettable(resolve))