Docserver: Consolidate features caching and access.

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

Index: chrome/common/extensions/docs/server2/api_list_data_source.py
diff --git a/chrome/common/extensions/docs/server2/api_list_data_source.py b/chrome/common/extensions/docs/server2/api_list_data_source.py
index 2e35ad631fb6a9b14e94c954b843e25d6fef53a5..cd3c501c41cd2dc7ac0fd5a58b2122ccf0c86198 100644
--- a/chrome/common/extensions/docs/server2/api_list_data_source.py
+++ b/chrome/common/extensions/docs/server2/api_list_data_source.py
@@ -2,84 +2,106 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from operator import itemgetter
 import os
 
+import features_utility as features
+from third_party.json_schema_compiler.json_parse import Parse
 import third_party.json_schema_compiler.model as model
 import docs_server_utils as utils
 
+def _GetAPICategory(api, public_apis):
+  name = api['name']
+  if (name.endswith('Private') or
+      name not in public_apis):
+    return 'private'
+  if name.startswith('experimental.'):
+    return 'experimental'
+  return 'chrome'
+
 class APIListDataSource(object):
   """ This class creates a list of chrome.* APIs and chrome.experimental.* APIs
   for extensions and apps that are used in the api_index.html and
   experimental.html pages.
-  |api_path| is the path to the API schemas.
-  |public_path| is the path to the public HTML templates.
-  An API is considered listable if it's in both |api_path| and |public_path| -
-  the API schemas may contain undocumentable APIs, and the public HTML templates
-  will contain non-API articles.
+
+  An API is considered listable if it is listed in _api_features.json,
+  it has a corresponding HTML file in the public template path, and one of
+  the following conditions is met:
+    - It has no "dependencies" or "extension_types" properties in _api_features
+    - It has an "extension_types" property in _api_features with either/both
+      "extension"/"platform_app" values present.
+    - It has a dependency in _{api,manifest,permission}_features with an
+      "extension_types" property where either/both "extension"/"platform_app"
+      values are present.
   """
   class Factory(object):
-    def __init__(self, compiled_fs_factory, file_system, api_path, public_path):
-      self._compiled_fs = compiled_fs_factory.Create(self._ListAPIs,
-                                                     APIListDataSource)
+    def __init__(self,
+                 compiled_fs_factory,
+                 file_system,
+                 public_template_path,
+                 features_bundle):
       self._file_system = file_system
-      def Normalize(string):
+      def NormalizePath(string):
         return string if string.endswith('/') else (string + '/')
-      self._api_path = Normalize(api_path)
-      self._public_path = Normalize(public_path)
+      self._public_template_path = NormalizePath(public_template_path)
+      self._cache = compiled_fs_factory.Create(self._CollectPublicAPIs,
+                                               APIListDataSource)
+      self._features_bundle = features_bundle
 
-    def _GetAPIsInSubdirectory(self, api_names, doc_type):
-      public_templates = []
-      for root, _, files in self._file_system.Walk(
-          self._public_path + doc_type):
-        public_templates.extend(
-            ('%s/%s' % (root, name)).lstrip('/') for name in files)
-      template_names = set(os.path.splitext(name)[0]
-                           for name in public_templates)
-      experimental_apis = []
-      chrome_apis = []
-      private_apis = []
-      for template_name in sorted(template_names):
-        if model.UnixName(template_name) not in api_names:
-          continue
-        entry = {'name': template_name.replace('_', '.')}
-        if template_name.startswith('experimental'):
-          experimental_apis.append(entry)
-        elif template_name.endswith('Private'):
-          private_apis.append(entry)
-        else:
-          chrome_apis.append(entry)
-      if len(chrome_apis):
-        chrome_apis[-1]['last'] = True
-      if len(experimental_apis):
-        experimental_apis[-1]['last'] = True
-      if len(private_apis):
-        private_apis[-1]['last'] = True
+    def _CollectPublicAPIs(self, base_dir, files):
+      def GetAPIsForPlatform(names, platform):
+        public_templates = []
+        for root, _, files in self._file_system.Walk(
+            self._public_template_path + platform):
+          public_templates.extend(
+              ('%s/%s' % (root, name)).lstrip('/') for name in files)
+        template_names = set(os.path.splitext(name)[0]
+                             for name in public_templates)
+        return sorted(map(lambda name: name.replace('_', '.'), template_names))

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

see comments below (working my way from bottom to top in this file for some
reason) about sorting. shouldn't be necessary.

also: prefer list comprehension to map unless there's a really good reason not
to.

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

Done.

+      api_names = set(utils.SanitizeAPIName(name) for name in files)
       return {
-        'chrome': chrome_apis,
-        'experimental': experimental_apis,
-        'private': private_apis
+        'apps': GetAPIsForPlatform(api_names, 'apps'),
+        'extensions': GetAPIsForPlatform(api_names, 'extensions')
       }
 
-    def _ListAPIs(self, base_dir, apis):
-      api_names = set(utils.SanitizeAPIName(name) for name in apis)
-      return {
-        'apps': self._GetAPIsInSubdirectory(api_names, 'apps'),
-        'extensions': self._GetAPIsInSubdirectory(api_names, 'extensions')
+    def _GenerateAPIDict(self):
+      def MakeEmptyAPIDict():
+        return { 'chrome': [], 'experimental': [], 'private': [] }
+      api_dict = {
+        'apps': MakeEmptyAPIDict(),
+        'extensions': MakeEmptyAPIDict()
       }
+      public_apis = self._cache.GetFromFileListing(self._public_template_path)

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

this "public api" naming confuses me because it look like you're also including
private APIs in that. it's something like "GetDocumentedAPIs" or something?
"GetAPIsWithPublicDocumentation"?

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

Done.

+      api_features = self._features_bundle.GetAPIFeatures()
+      for platform in api_dict.iterkeys():

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

to cut down on a level of nesting here you could write above

api_dict = {
  'apps': MakeDictForPlatform('apps'),
  'extensions': MakeDictForPlatform('extensions'),
}

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

Done.

+        for api in api_features.itervalues():
+          if platform in api['platforms']:

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

platform in api['platforms'] or 'all' in api['platforms']

or

any(key in api['platforms'] for key in (platform, 'all'))

? hey they're exactly the same length, funny.

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

I think you're thinking of extension_types. platforms is always a list over the
set {'apps', 'extensions'}

+            category = _GetAPICategory(api, public_apis[platform])
+            api_dict[platform][category].append(api)
+      for platform in api_dict.iterkeys():

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

maybe I'm tired; but this inner loop is just to sort the lists, right? and
attach the 'last' to it?

I wouldn't worry about sorting it, just show in order that the features file
presents (... works so long as it uses an OrderedDict, which, it should). and
can you do the 'last' thing in the loop beforehand?

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

The sorting is actually enforced by tests. It looks like the order of these
lists determines their order on API index pages.

+        for category in api_dict[platform]:
+          apis = api_dict[platform][category]
+          apis = sorted(apis, key=itemgetter('name'))
+          if len(apis) > 0:
+            apis[-1]['last'] = True

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

bleh this is so common. WDYT about adding a method to doc_server_utils like
"MarkLast" or something.

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

Done.

+          api_dict[platform][category] = apis
+      return api_dict
 
     def Create(self):
-      return APIListDataSource(self._compiled_fs, self._api_path)
+      return APIListDataSource(self)
 
-  def __init__(self, compiled_fs, api_path):
-    self._compiled_fs = compiled_fs
-    self._api_path = api_path
+  def __init__(self, factory):
+    self._factory = factory
+    self._apis = None
 
   def GetAllNames(self):
-    names = []
+    apis = []
     for platform in ['apps', 'extensions']:
       for category in ['chrome', 'experimental', 'private']:
-        names.extend(self.get(platform).get(category))
-    return [api_name['name'] for api_name in names]
+        apis.extend(self.get(platform).get(category))
+    return sorted(api['name'] for api in apis)

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

likewise, don't worry about sorting it.

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

Done.

 
   def get(self, key):
-    return self._compiled_fs.GetFromFileListing(self._api_path)[key]
+    if self._apis is None:
+      self._apis = self._factory._GenerateAPIDict()
+    return self._apis.get(key)

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

it looks like it's ok to be using an ObjectStore to cache this rather than an
ad-hoc in-memory cache. Using an ObjectStore to cache this will let other
instances benefit from this work (ObjectStore also does in-memory caching).

the times when it's not ok to use an ObjectStore: if the data relies on
per-instance data, such as the request parameters. but that's pretty rare.

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

Done.