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

chrome/common/extensions/docs/server2/caching_rietveld_patcher.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 datetime import datetime, timedelta
+from file_system import FileNotFoundError, ToUnicode
+from future import Future
+from patcher import Patcher
+
+_VERSION_CACHE_MAXAGE = timedelta(seconds=5)
+
+''' Append @version for keys to distinguish between different patchsets of
+an issue.
+'''
+def _MakeKey(path_or_paths, version):
+  if isinstance(path_or_paths, list) or isinstance(path_or_paths, set):

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

isinstance(path_or_paths, (tuple, list, set))

that said, it's nicer for functions to accept a single type and either
- this method can only take a sequence, and _ToObjectStoreValue does
_MakeKey([key])[0] - in which case call this MakeKeys.
- this takes a single value and where it's used below does a list comprehension

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

Done.

+    return ['%s@%s' % (p, version) for p in path_or_paths]
+  return _MakeKey([path_or_paths], version)[0]
+
+def _ToObjectStoreValue(raw_value, version):
+  return {_MakeKey(key, version): raw_value[key] for key in raw_value}
+
+def _FromObjectStoreValue(raw_value, binary):
+  return {key[0:key.find('@')]: _HandleBinary(raw_value[key], binary)

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

What happens if a patchset contains both binary and non-binary data? Do we need
to handle that at all? Can it happen?

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

For this reason, CachingRietveldPatcher always read from RietveldPatcher with
binary=True and cache raw data. When returning the data, it is converted to
unicode if binary=False.

It can't happen for now. But if in the future, Rietveld can handle binary files,
this should also work.

+          for key in raw_value}

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

btw @ is a legitimate character to appear in a file path, should be rfind('@').

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

Maybe use a character that's not legitimate in file path here? (but I believe no
one will ever commit a file with '@' in its name.)

+
+def _HandleBinary(data, binary):
+  return data if binary else ToUnicode(data)
+
+class _AsyncUncachedFuture(object):
+  def __init__(self,
+               version,
+               paths,
+               binary,
+               cached_value,
+               missing_paths,
+               fetch_delegate,
+               object_store):
+    self._version = version
+    self._paths = paths
+    self._binary = binary
+    self._cached_value = cached_value
+    self._missing_paths = missing_paths
+    self._fetch_delegate = fetch_delegate
+    self._object_store = object_store
+
+  def Get(self):
+    uncached_raw_value = self._fetch_delegate.Get()
+    self._object_store.SetMulti(_ToObjectStoreValue(uncached_raw_value,
+                                                    self._version))
+
+    for path in self._missing_paths:
+      if uncached_raw_value.get(path) is None:
+        raise FileNotFoundError('File %s was not found in the patch.' % path)
+      self._cached_value[path] = _HandleBinary(uncached_raw_value[path],
+                                               self._binary)
+
+    return self._cached_value
+
+class CachingRietveldPatcher(Patcher):
+  ''' CachingRietveldPatcher implements a caching layer on top of |patcher|.
+  In theory, it can be used with any class that implements Patcher. But this
+  class assumes that applying to all patched files one at a time is more

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

s/one at a time/at once/

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

Done.

+  efficient than applying to individual files.
+  '''
+  def __init__(self,
+               rietveld_patcher,
+               object_store_creator,
+               test_datetime=datetime):
+    self._patcher = rietveld_patcher
+    self._list_object_store = object_store_creator.Create(
+        CachingRietveldPatcher, category='list')
+    self._file_object_store = object_store_creator.Create(
+        CachingRietveldPatcher, category='file')
+    self._datetime = test_datetime
+
+  def GetVersion(self):
+    key = 'version'
+    value = self._list_object_store.Get(key).Get()

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

Re-using list_object_store here and in GetPatchedFiles is a bit confusing.
Having an object store where you only ever access a single key is fine,
version_object_store?

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

Done.

+    if value is not None:
+      version, time = value
+      if self._datetime.now() - time < _VERSION_CACHE_MAXAGE:
+        return version
+
+    version = self._patcher.GetVersion()
+    self._list_object_store.Set(key,
+                                (version, self._datetime.now()))
+    return version
+
+  def GetPatchedFiles(self, version = None):

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

version=None

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

Done.

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

Done.

+    if version is None:
+      version = self.GetVersion()
+    value = self._list_object_store.Get(version).Get()
+    if value is not None:
+      return value
+
+    value = self._patcher.GetPatchedFiles(version)

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

more descriptive name than "value" pls

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

Done.

+    self._list_object_store.Set(version, value)
+    return value
+
+  def Apply(self, paths, file_system, binary=False, version=None):
+    if version is None:
+      version = self.GetVersion()
+    added, deleted, modified = self.GetPatchedFiles(version)
+    cached_value = _FromObjectStoreValue(self._file_object_store.
+        GetMulti(_MakeKey(paths, version)).Get(), binary)
+    missing_paths = list(set(paths) - set(cached_value.keys()))
+    if len(missing_paths) == 0:
+      return Future(value=cached_value)
+
+    return _AsyncUncachedFuture(version,
+                                paths,
+                                binary,
+                                cached_value,
+                                missing_paths,
+                                self._patcher.Apply(set(added) | set(modified),
+                                                    None,
+                                                    True,
+                                                    version),

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

why is binary explicitly True?

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

See the comment above. Maybe I should add a comment here.

+                                self._file_object_store)