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

chrome/common/extensions/docs/server2/rietveld_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.
+
+import tarfile
+from StringIO import StringIO
+
+from file_system import FileSystem, FileNotFoundError, StatInfo, ToUnicode
+from future import Future
+from svn_constants import EXTENSIONS_PATH
+
+RIETVELD_ISSUE_TARBALL = 'tarball/%s/%s'

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

constants with hanging substitutions are odd... they're not really constants.
Just inline that.

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

Done.

+RIETVELD_TARBALL_PATCHED_FILE = 'b/' + EXTENSIONS_PATH + '/%s'

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

ditto. and string concatentation is slow, except perhaps in the case of only
catt-ing two strings. Anyway, use %s.

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

Done.

+
+class _AsyncFetchFuture(object):
+  def __init__(self, paths, binary, tarball):
+    self._paths = paths
+    self._binary = binary
+    self._tarball = tarball
+
+  def Get(self):
+    tarball_result = self._tarball.Get()
+    if tarball_result.status_code != 200:
+      raise FileNotFoundError('Failed to download tarball.')

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

this means we'll be unpacking the tarball every time something from the
filesystem is read. Instead we should cache the result as soon as it's read and
unpacked, then always use that (cache on RietveldFileSystem obviously).

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

There's cache on RietveldFileSystem (memcache, see server_instance.py), but no
cache for the tarball (and probably multiple requests are made for the same
tarball). Maybe also cache tarball in memcache (what if it's too large (although
it should occasionally happen))?

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

There is a cache, though it's still the case that two different files will be
cached separately, so requesting "extensions/foo.html" and later
"extensions/bar.html" will do another fetch of the tarball.

TBH it's quite unlikely that these will be large, so I'd check them all into an
ObjectStore (the individual files). Using a CachingFileSystem around it becomes
unnecessary then, but that's fine, a lot of the smart caching stuff it does is
moot here anyway since the data doesn't change.

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

Done.
Now RietveldFileSystem has built-in cache. However, I have to get values from
cache synchronously in Read in order to determine whether fetching the tarball
is needed.
When a tarball is fetched, it will extract all files that are not yet cached and
then cache them.

+
+    try:
+      tar = tarfile.open(fileobj=StringIO(tarball_result.content))
+    except tarfile.TarError as e:
+      raise FileNotFoundError('Invalid tarball.')
+
+    value = {}
+    for path in self._paths:
+      try:
+        patched_file = tar.extractfile(RIETVELD_TARBALL_PATCHED_FILE % path)
+      except tarfile.TarError as e:
+        raise FileNotFoundError('File %s was deleted in the patchset.' % p)
+
+      value[path] = patched_file.read()
+      patched_file.close()
+      if not self._binary:
+        value[path] = ToUnicode(value[path])
+
+    return value

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

.. this is the value to cache. In fact, most of the time Read would actually
return a Future(value=...) with the value that is being created here.

+
+class RietveldFileSystem(FileSystem):
+  """ Class to fetch resources from a patchset in Rietveld.
+  """

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

Use ' not ".

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

Done.

+  def __init__(self, issue, patchset, patched_files, fetcher):

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

pass through that PatchData object that you'll write rather than
issue/patchset/patched_files.

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

Done.

+    self._tarball = fetcher.FetchAsync(
+        RIETVELD_ISSUE_TARBALL % (issue, patchset))
+    self._patched_files = patched_files
+
+  def Read(self, paths, binary=False):
+    return Future(delegate=_AsyncFetchFuture(
+        paths,
+        binary,
+        self._tarball))
+
+  def Stat(self, path):
+    child_versions = {}
+    for f in self._patched_files:
+      if f.startswith(path):
+        child = f.split(path, 1)[1]
+        if '/' in child:
+          child = child.strip('/', 1)[0] + '/'
+        child_versions[child] = '0'
+    return StatInfo('0', child_versions)

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

I can't actually really read this, besides, the Stat for these files is probably
the patchset.

Implement something like:

if path.endswith('/'):
  return StatInfo(
      patchset,
      child_versions=dict((child, patchset) for child in patched_files
                          if child.startswith(path))
return StatInfo(patchset)

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

Different patchsets in an issue get different namespaces for memcache so they
won't conflict with each other. A patchset, once created, should not change.
That's why I return '0' as version for all files here.

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

Ok, that makes sense. Let's see how it turns out eventually. This approach *is*
simpler but it's also akin to mirroring subversion at every version - it's
potentially wasteful. The number of patchsets uploaded to these things is
hopefully fairly low, but it could number in the 10s and that's 10x more data
than we need.

What I'm saying is that maybe we *will* end up implementing these on a per-issue
basis rather than a per-issue per-patchset basis. In fact, what do you think;
how much extra work would that be?

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

But these are memcached (and are never intended to be persisted). Cached data
for older patchsets will be automatically purged eventually.
Implementing RietveldFileSystem on a per-issue basis means that it has to make
(extra? synchronous) requests to get patchset number in order to construct
tarball URL.
But maybe it makes more sense. Different patchsets in an issue are similar to
different revisions in svn. If we decide not to reuse CompiledFileSystem caches,
this should be the right thing to do.

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

Yeah, exactly.

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

Now, RietveldFileSystem is no longer wrapped by CachingFileSystem. It's only
wrapped by PatchedFileSystem, which never calls Stat on it. So I removed Stat
here.