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

chrome/common/extensions/docs/server2/patch_servlet.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 appengine_url_fetcher import AppEngineUrlFetcher
+from caching_file_system import CachingFileSystem
+from caching_rietveld_patcher import CachingRietveldPatcher
+from chained_compiled_file_system import (CompiledFileSystem,
+                                          ChainedCompiledFileSystem)
+from instance_servlet import InstanceServlet
+from render_servlet import RenderServlet
+from rietveld_patcher import RietveldPatcher, RietveldPatcherError
+from object_store_creator import ObjectStoreCreator
+from patched_file_system import PatchedFileSystem
+from server_instance import ServerInstance
+from servlet import Request, Response, Servlet
+import svn_constants
+import url_constants
+
+class PatchServlet(Servlet):
+  '''Servlet which renders patched docs.
+  '''
+  class Delegate(InstanceServlet.Delegate):

[not at google - send to devlin, 2013/05/11 20:39:53]

see earlier comment about using composition over inheritance

[not at google - send to devlin, 2013/05/11 20:39:53]

Why InstanceServlet rather than RenderServlet? I was trying to make it so that
RenderServlet is the entry point for rendering things, and
Cron/Instance/PatchServlet call out to that.

Anyway, be explicit about what PatchServlet needs, and keep the implementation
of InstanceServlet's (or RenderServlet's?) Delegate separate. See how
CronServlet does it. It's not clear from looking at this what the dependency
graph looks like.

I.e. pull most of this into a separate _RenderServletDelegate class, which takes
the delegate for PatchServlet and whatever it needs (issue I suppose).

+    def __init__(self, issue):
+      self._issue = issue
+
+    # Overriden in test.
+    def _GetChannel(self, channel):

[not at google - send to devlin, 2013/05/11 20:39:53]

methods intended for overriding shouldn't start with a _, they're not private.

I don't understand why this method is even necessary, though. The channel is
passed in, and it's not interesting for patch servlet.

+      return channel
+
+    def CreateServerInstanceForChannel(self, channel):
+      channel = self._GetChannel(channel)
+      base_object_store_creator = ObjectStoreCreator(channel,
+                                                     start_empty=False)
+      object_store_creator = ObjectStoreCreator('trunk@%s' % self._issue,
+                                                start_empty=False)
+      # TODO(fj): Use OfflineFileSystem here once all json/idl files in api/
+      # are pulled into data store by cron jobs.
+      base_file_system = CachingFileSystem(self.CreateHostFileSystemForBranch(
+          channel), base_object_store_creator)
+      rietveld_patcher = CachingRietveldPatcher(
+          RietveldPatcher(svn_constants.EXTENSIONS_PATH,
+                          self._issue,
+                          AppEngineUrlFetcher(url_constants.CODEREVIEW_SERVER)),
+          object_store_creator)
+      patched_file_system = PatchedFileSystem(base_file_system,
+                                              rietveld_patcher)
+      base_compiled_fs_factory = CompiledFileSystem.Factory(
+          base_file_system, base_object_store_creator)
+      compiled_fs_factory = ChainedCompiledFileSystem.Factory(
+          patched_file_system, object_store_creator, base_compiled_fs_factory)
+      return ServerInstance(channel,
+                            object_store_creator,
+                            patched_file_system,
+                            self.CreateAppSamplesFileSystem(
+                                object_store_creator),
+                            '/_patch/%s/static' % self._issue,
+                            compiled_fs_factory)
+
+  def __init__(self, request, delegate=None):
+    self._request = request
+    self._delegate = delegate

[not at google - send to devlin, 2013/05/11 20:39:53]

see below.

self._issue = ...
self._delegate = delegate or _RenderServletDelegate(self._issue)

etc.

+
+  def Get(self):
+    path_with_issue = self._request.path.lstrip('/')
+    if '/' in path_with_issue:
+      issue, real_path = path_with_issue.split('/', 1)
+    else:
+      return Response.NotFound('Malformed URL.')

[not at google - send to devlin, 2013/05/11 20:39:53]

Maybe print what the URL should look like

[方觉(Fang Jue), 2013/05/12 03:01:47]

Done.

+
+    fake_path = '/trunk/%s' % real_path
+
+    if self._delegate is None:
+      self._delegate = PatchServlet.Delegate(issue)

[not at google - send to devlin, 2013/05/11 20:39:53]

everything above here can go in the constructor.

[方觉(Fang Jue), 2013/05/12 03:01:47]

but what about return Response.NotFound('Malformed URL.')?

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

I see. Ok. It's just weird overriding a field on self that was set on
construction.

What CronServlet does it has a delegate_for_test. You could actually call the
field that to be clear. Then here,

delegate = self._delegate_for_test or PathServlet.Delegate(issue)

+
+    try:
+      response = RenderServlet(Request(fake_path,
+                                       self._request.host,
+                                       self._request.headers),
+                               self._delegate).Get()
+      # Disable cache for patched content.
+      response.headers.pop('cache-control', None)
+    except RietveldPatcherError as e:
+      response = Response.NotFound(e.message, {'Content-Type': 'text/plain'})
+
+    if response.IsRedirect():

[not at google - send to devlin, 2013/05/11 20:39:53]

maybe this should actually be GetRedirect().

redirect = response.GetRedirect()
if redirect is not None:
  ...

[方觉(Fang Jue), 2013/05/12 03:01:47]

Done.

+      url = response.headers['Location']
+      if url.startswith('/trunk/'):
+        url = url.split('/trunk', 1)[1]
+      response.headers['Location'] = '/_patch/%s%s' % (issue, url)
+    return response