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

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

Index: chrome/common/extensions/docs/server2/server_instance.py
===================================================================
--- chrome/common/extensions/docs/server2/server_instance.py	(revision 196612)
+++ chrome/common/extensions/docs/server2/server_instance.py	(working copy)
@@ -24,8 +24,11 @@
 from local_file_system import LocalFileSystem
 from object_store_creator import ObjectStoreCreator
 from offline_file_system import OfflineFileSystem
+from patched_file_system import PatchedFileSystem
 from path_canonicalizer import PathCanonicalizer
 from reference_resolver import ReferenceResolver
+from rietveld_file_system import RietveldFileSystem
+from rietveld_utility import RietveldUtility
 from samples_data_source import SamplesDataSource
 from sidenav_data_source import SidenavDataSource
 from subversion_file_system import SubversionFileSystem
@@ -46,6 +49,7 @@
   # Lazily create so we don't create github file systems unnecessarily in
   # tests.
   branch_utility = None
+  rietveld_utility = None
   github_file_system = None
 
   @staticmethod
@@ -73,7 +77,7 @@
         ServerInstance._GetOrCreateGithubFileSystem())
 
   @staticmethod
-  def CreateOnline(channel):
+  def CreateOnline(channel, issue=None):
     '''Creates/creates an online server instance, meaning that both local and
     subversion/github resources are queried.
     '''
@@ -93,6 +97,12 @@
     viewvc_url = svn_url.replace(url_constants.SVN_URL,
                                  url_constants.VIEWVC_URL)
 
+    if issue is not None:
+      rietveld_utility = ServerInstance._GetOrCreateRietveldUtility()
+      patchset = rietveld_utility.GetPatchsetFromIssue(issue)
+      if patchset is None:
+        issue is None

[not at google - send to devlin, 2013/04/26 23:53:53]

I don't think "issue is None" means anything as a statement, maybe you mean
"issue = None".

However, this is the kind of data that should happen outside this class so that
the error can be sensibly dealt with. Perhaps bundle up the issue and patchset
(and any other data) into a patch_data class, pass that in here.

+
     object_store_creator_factory = ObjectStoreCreator.Factory(GetAppVersion(),
                                                               branch,
                                                               start_empty=True)
@@ -101,11 +111,37 @@
         SubversionFileSystem(AppEngineUrlFetcher(svn_url),
                              AppEngineUrlFetcher(viewvc_url)),
         object_store_creator_factory)
+    if issue is not None:
+      (patched_files, changes) = rietveld_utility.GetPatchsetDetails(
+          issue, patchset)

[not at google - send to devlin, 2013/04/26 23:53:53]

patched files... changes... everything should be passed into this class rather
than determined by it.

+      patched_object_store_creator_factory = ObjectStoreCreator.Factory(
+          GetAppVersion(),
+          branch,
+          True, # start_empty

[not at google - send to devlin, 2013/04/26 23:53:53]

use start_empty=True

[方觉(Fang Jue), 2013/04/29 12:46:23]

Done.

+          issue,
+          patchset)

[not at google - send to devlin, 2013/04/26 23:53:53]

it's nice to spell out optional arguments explicitly, though depending on how
ObjectStoreCreator turns out, this may be inevitable.

[方觉(Fang Jue), 2013/04/29 12:46:23]

Done.

+      rietveld_file_system = CachingFileSystem(
+          RietveldFileSystem(issue,
+                             patchset,
+                             patched_files,
+                             AppEngineUrlFetcher(
+                                 url_constants.CODEREVIEW_SERVER)),
+          patched_object_store_creator_factory)
+      patched_file_system = PatchedFileSystem(patched_files,
+                                              svn_file_system,
+                                              rietveld_file_system)
+    else:
+      patched_object_store_creator_factory = None
+      patched_file_system = None
+      changes = None
 
     return ServerInstance(channel,
                           object_store_creator_factory,
                           svn_file_system,

[not at google - send to devlin, 2013/04/26 23:53:53]

I hope that it's possible to inject patched_file_system in place of
svn_file_system here and everything will work. I'm surprised there is so much
complication here.

[方觉(Fang Jue), 2013/04/27 00:24:34]

I was trying to reuse existing cached content for trunk. That's why there's
complication here and below.
I'm sure replacing svn_file_system with patched_file_system will work. But we'll
end up having the same data cached in different namespaces and reconstructing
everything (whether changed in the patch or not) for different patches.
What do you think?

[not at google - send to devlin, 2013/04/27 00:45:30]

patched_file_system delegates to svn_file_system, which is a CachingFileSystem,
so you'll still get all the caching, or am I misunderstanding?

[方觉(Fang Jue), 2013/04/27 01:02:48]

SubversionFileSystem will get all the caching but what about CompiledFileSystem?

[not at google - send to devlin, 2013/04/27 01:07:48]

Ah I see what you mean. Eh, re-compiling all that stuff isn't a big deal.

[方觉(Fang Jue), 2013/04/29 12:46:23]

Then complicated and messy changes below are no longer necessary!

-                          ServerInstance._GetOrCreateGithubFileSystem())
+                          ServerInstance._GetOrCreateGithubFileSystem(),
+                          patched_object_store_creator_factory,
+                          patched_file_system,
+                          changes)
 
   @staticmethod
   def CreateForTest(file_system):
@@ -123,6 +159,13 @@
     return ServerInstance.branch_utility
 
   @staticmethod
+  def _GetOrCreateRietveldUtility():
+    if ServerInstance.rietveld_utility is None:
+      ServerInstance.rietveld_utility = RietveldUtility(
+          AppEngineUrlFetcher(url_constants.CODEREVIEW_SERVER))
+    return ServerInstance.rietveld_utility

[not at google - send to devlin, 2013/04/26 23:53:53]

this would probably end up getting owned by PatchServlet?

+
+  @staticmethod
   def _GetOrCreateGithubFileSystem():
     # Initialising github is pointless if samples are disabled, since it's only
     # used for apps samples.
@@ -135,32 +178,67 @@
             AppEngineBlobstore())
     return ServerInstance.github_file_system
 
+  def _GetCompiledFileSystemFactory(self, deps=[]):
+    for dep in deps:
+      if self.changes.get(dep):
+        return self.compiled_fs_factory
+    return self.unpatched_compiled_fs_factory
+
   def __init__(self,

[not at google - send to devlin, 2013/04/26 23:53:53]

As above - I think most of __init__ can stay the same, you just need to inject
that patched file system instead of the svn file system

[方觉(Fang Jue), 2013/04/29 12:46:23]

Done.

                channel,
-               object_store_creator_factory,
+               unpatched_object_store_creator_factory,
                svn_file_system,
-               github_file_system):
-    self.svn_file_system = svn_file_system
+               github_file_system,
+               patched_object_store_creator_factory=None,
+               patched_file_system=None,
+               changes=None):
+    if changes is None:
+      changes = {}
+    self.changes = changes
+    logging.info(changes)
 
     self.github_file_system = github_file_system
 
-    self.compiled_fs_factory = CompiledFileSystem.Factory(
+    self.unpatched_compiled_fs_factory = CompiledFileSystem.Factory(
         svn_file_system,
-        object_store_creator_factory)
+        unpatched_object_store_creator_factory)
+    if patched_object_store_creator_factory is None:
+      patched_object_store_creator_factory = (
+          unpatched_object_store_creator_factory)
+      self.compiled_fs_factory = self.unpatched_compiled_fs_factory
+    else:
+      self.compiled_fs_factory = CompiledFileSystem.Factory(
+          patched_file_system,
+          patched_object_store_creator_factory)
 
     self.api_list_data_source_factory = APIListDataSource.Factory(
-        self.compiled_fs_factory,
+        self._GetCompiledFileSystemFactory([
+            svn_constants.API_PATH,
+            svn_constants.PUBLIC_TEMPLATE_PATH]),
         svn_constants.API_PATH,
         svn_constants.PUBLIC_TEMPLATE_PATH)
 
     self.api_data_source_factory = APIDataSource.Factory(
-        self.compiled_fs_factory,
+        self._GetCompiledFileSystemFactory([svn_constants.API_PATH]),
         svn_constants.API_PATH)
 
-    self.ref_resolver_factory = ReferenceResolver.Factory(
+    self.unpatched_ref_resolver_factory = ReferenceResolver.Factory(
+        APIDataSource.Factory(
+            self.compiled_fs_factory,
+            svn_constants.API_PATH),
+        APIListDataSource.Factory(
+            self.compiled_fs_factory,
+            svn_constants.API_PATH,
+            svn_constants.PUBLIC_TEMPLATE_PATH),
+        unpatched_object_store_creator_factory)
+    if (changes.get(svn_constants.API_PATH) or
+        changes.get(svn_constants.PUBLIC_TEMPLATE_PATH)):
+      self.ref_resolver_factory = ReferenceResolver.Factory(
         self.api_data_source_factory,
         self.api_list_data_source_factory,
-        object_store_creator_factory)
+        patched_object_store_creator_factory)
+    else:
+      self.ref_resolver_factory = self.unpatched_ref_resolver_factory
 
     self.api_data_source_factory.SetReferenceResolverFactory(
         self.ref_resolver_factory)
@@ -171,25 +249,27 @@
     if _IsSamplesDisabled():
       svn_fs_for_samples = EmptyDirFileSystem()
     else:
-      svn_fs_for_samples = self.svn_file_system
+      svn_fs_for_samples = svn_file_system
     self.samples_data_source_factory = SamplesDataSource.Factory(
         channel,
         svn_fs_for_samples,
         self.github_file_system,
-        self.ref_resolver_factory,
-        object_store_creator_factory,
+        self.unpatched_ref_resolver_factory,
+        unpatched_object_store_creator_factory,
         svn_constants.EXAMPLES_PATH)
 
     self.api_data_source_factory.SetSamplesDataSourceFactory(
         self.samples_data_source_factory)
 
     self.intro_data_source_factory = IntroDataSource.Factory(
-        self.compiled_fs_factory,
+        self._GetCompiledFileSystemFactory([
+            svn_constants.INTRO_PATH,
+            svn_constants.ARTICLE_PATH]),
         self.ref_resolver_factory,
         [svn_constants.INTRO_PATH, svn_constants.ARTICLE_PATH])
 
     self.sidenav_data_source_factory = SidenavDataSource.Factory(
-        self.compiled_fs_factory,
+        self._GetCompiledFileSystemFactory([svn_constants.JSON_PATH]),
         svn_constants.JSON_PATH)
 
     self.template_data_source_factory = TemplateDataSource.Factory(
@@ -205,14 +285,20 @@
         svn_constants.PRIVATE_TEMPLATE_PATH)
 
     self.example_zipper = ExampleZipper(
-        self.compiled_fs_factory,
+        self.unpatched_compiled_fs_factory,
         svn_constants.DOCS_PATH)
 
     self.path_canonicalizer = PathCanonicalizer(
         channel,
-        self.compiled_fs_factory)
+        self.unpatched_compiled_fs_factory)
 
-    self.content_cache = self.compiled_fs_factory.GetOrCreateIdentity()
+    self.unpatched_content_cache = (self.unpatched_compiled_fs_factory.
+        GetOrCreateIdentity())
+    if changes.get(svn_constants.STATIC_PATH):
+      self.content_cache = (self.patched_compiled_fs_factory.
+          GetOrCreateIdentity())
+    else:
+      self.content_cache = self.unpatched_content_cache
 
   def _FetchStaticResource(self, path, response):
     """Fetch a resource in the 'static' directory.
@@ -235,7 +321,7 @@
         response.headers['content-type'] = 'application/zip'
       elif path.startswith('extensions/examples/'):
         mimetype = mimetypes.guess_type(path)[0] or 'text/plain'
-        content = self.content_cache.GetFromFile(
+        content = self.unpatched_content_cache.GetFromFile(
             '%s/%s' % (svn_constants.DOCS_PATH, path[len('extensions/'):]),
             binary=_IsBinaryMimetype(mimetype))
         response.headers['content-type'] = 'text/plain'