Docserver: Consolidate features caching and access.

chrome/common/extensions/docs/server2/features_bundle.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 functools import partial
+
+from features import Features
+import features_utility
+import svn_constants
+from third_party.json_schema_compiler.json_parse import Parse
+
+class _FeaturesCache(object):
+  def __init__(self, file_system, compiled_fs_factory, json_paths):

[not at google - send to devlin, 2013/10/01 16:34:34]

this json_paths thing is only necessary to support that features_file_overrides
case, right? so when you delete that you can simplify this code as well.

[Ken Rockot(use gerrit already), 2013/10/01 22:18:16]

Done.

+    self._file_system = file_system
+    self._cache = compiled_fs_factory.Create(self._CreateCache, type(self))
+    if json_paths is None:
+      self._json_path = None
+      self._extra_paths = ()
+    else:
+      self._json_path = json_paths[0]
+      self._extra_paths = json_paths[1:]
+
+  def _CreateCache(self, _, features_json):
+    features = features_utility.Parse(Parse(features_json))
+    for path in self._extra_paths:
+      extra_json = self._file_system.ReadSingle(path)
+      features = features_utility.MergedWith(
+          features_utility.Parse(Parse(extra_json)), features)
+    return features
+
+  def GetFeatures(self):
+    if self._json_path is None:
+      return {}
+    return self._cache.GetFromFile(self._json_path)
+
+class FeaturesBundle(object):
+  '''Provides access to properties of API, Manifest, and Permission features.'''

[not at google - send to devlin, 2013/10/01 16:34:34]

'''Foo
'''

not

'''Foo'''.

[Ken Rockot(use gerrit already), 2013/10/01 22:18:16]

Done.

+  def __init__(self,
+               file_system,
+               compiled_fs_factory,
+               features_files_override=None):
+    if features_files_override is not None:
+      features_files = features_files_override

[not at google - send to devlin, 2013/10/01 16:34:34]

rather than this, create a FakeFeaturesBundle class which stubs out
Get{Permission,Manifest,API}Features.

[Ken Rockot(use gerrit already), 2013/10/01 22:18:16]

Done.

+    else:
+      features_files = {
+        'api': (svn_constants.API_FEATURES_PATH,),
+        'manifest': (svn_constants.MANIFEST_FEATURES_PATH,
+                     svn_constants.MANIFEST_JSON_PATH),
+        'permission': (svn_constants.PERMISSION_FEATURES_PATH,
+                       svn_constants.PERMISSIONS_JSON_PATH)
+      }
+    self._api_cache = _FeaturesCache(
+        file_system,
+        compiled_fs_factory,
+        features_files.get('api'))
+    self._manifest_cache = _FeaturesCache(
+        file_system,
+        compiled_fs_factory,
+        features_files.get('manifest'))
+    self._permission_cache = _FeaturesCache(
+        file_system,
+        compiled_fs_factory,
+        features_files.get('permission'))
+    # Store an intermediate cache of API features since we add annotations.
+    self._api_features = None
+
+  def GetPermissionFeatures(self):
+    return self._permission_cache.GetFeatures()
+
+  def GetManifestFeatures(self):
+    return self._manifest_cache.GetFeatures()
+
+  def GetAPIFeatures(self, force_refresh=False):

[not at google - send to devlin, 2013/10/01 16:34:34]

doesn't look like force_refresh is used.

[Ken Rockot(use gerrit already), 2013/10/01 22:18:16]

Oops. Was going to be used as a Cron hack. Shouldn't be necessary.

+    if self._api_features is not None and not force_refresh:
+      return self._api_features
+    # Manifest and permission features must be loaded and annotated already
+    # for API features to be properly annotated.
+    self.GetManifestFeatures()
+    self.GetPermissionFeatures()
+    self._api_features = self._api_cache.GetFeatures()
+    for feature in self._api_features.itervalues():
+      features_utility.AddPlatformsFromDependencies(feature, self)

[not at google - send to devlin, 2013/10/01 16:34:34]

AddPlatformsFromDependencies is only used in this file. could you put it in this
file util classes are [an unfortunately occasionally useful] evil.

then you might be able to avoid the GetManifestFeatures/GetPermissionFeatures
caching weirdness.

[Ken Rockot(use gerrit already), 2013/10/01 22:18:16]

Done, and on that note features_utility.Filtered is now the only
features_utility function used outside of FeaturesBundle. Might (should) be able
to get rid of it eventually.

+    return self._api_features