Docserver manifest follow up (rewrite)

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

Index: chrome/common/extensions/docs/server2/manifest_data_source.py
diff --git a/chrome/common/extensions/docs/server2/manifest_data_source.py b/chrome/common/extensions/docs/server2/manifest_data_source.py
index b01d22655074aafbdc6e242ae61b48fef267d5ce..897b2531c6ca9b23f09038c3f6017531bb1b09a7 100644
--- a/chrome/common/extensions/docs/server2/manifest_data_source.py
+++ b/chrome/common/extensions/docs/server2/manifest_data_source.py
@@ -2,12 +2,95 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-from collections import OrderedDict
 from copy import deepcopy
-from operator import itemgetter
 
+from features_utility import MergeDictionaries, ProcessFeaturesFile
 from third_party.json_schema_compiler.json_parse import Parse
 
+def _InsertSubDocs(properties):
+  '''Properties that contain a '.' (period) should be inserted into a parent
+  dictionary. Ex, background.persistent should be a subdocument of
+  background. Mutates |properties|.
+  '''
+  def add_subdoc(propreties, parent, subname, value):
+    value['name'] = subname
+    if not 'subdocs' in properties[parent]:
+      properties[parent]['subdocs'] = {}
+    properties[parent]['subdocs'][subname] = value
+
+  for name in properties.keys():
+    if '.' in name:
+      value = properties.pop(name)
+      parent, subname = name.split('.', 1)
+      add_subdoc(properties, parent, subname, value)
+
+  for value in properties.values():
+    if 'subdocs' in value:
+      _InsertSubDocs(value['subdocs'])
+
+def _Filter(properties, platform):
+  '''Recursively remove entries that are not relevant to the target
+  |platform|.
+  '''
+  for key in properties.keys():
+    value = properties[key]
+    if not platform in value['platform']:
+      del properties[key]
+    else:
+      if 'subdocs' in value:
+        _Filter(value['subdocs'], platform)
+
+def _ConvertAndSortDocs(properties):
+  '''Convert docs and subdocs into lists and sort them, first by level, then
+  by name.
+  '''
+  levels = {
+    'required': 0,
+    'recommended': 1,
+    'only_one': 2,
+    'optional': 3,
+  }
+
+  keyfunc = lambda item: (levels[item.get('level', 'optional')], item['name'])

[not at google - send to devlin, 2013/07/24 21:45:56]

this code is a bit obfuscated for me to read, but it seems as though you could
make levels a list and use find() to make it a bit simpler.

this is also a bit too long for a lambda.

[jshumway, 2013/07/26 00:36:46]

brilliant idea, using list.index().
Now a named function.

+
+  def convert_and_sort_recur(properties):

[not at google - send to devlin, 2013/07/24 21:45:56]

the "recur" is a bit of an implementation detail..

[jshumway, 2013/07/26 00:36:46]

Done.

+    for key in properties.keys():
+      value = properties[key]
+      if 'subdocs' in value:
+        properties[key]['subdocs'] = convert_and_sort_recur(value['subdocs'])
+    return sorted(properties.values(), key=keyfunc)
+
+  return convert_and_sort_recur(properties)
+
+def _Annotate(properties):
+  '''Add level annotations to level groups in a manifest property list and
+  subdoc lists. Requeries a list of properties sorted by level, with subdocs
+  in sorted lists as well.
+  '''
+  annotations = {
+    'required': 'Required',
+    'recommended': 'Recommended',
+    'only_one': 'Pick one (or none)',
+    'optional': 'Optional'
+  }
+
+  def add_annotation(item, annotation):
+    if not 'annotations' in item:
+      item['annotations'] = []
+    item['annotations'].insert(0, annotation)
+
+  def annotate_recur(parent_level, properties):

[not at google - send to devlin, 2013/07/24 21:45:56]

likewise

[jshumway, 2013/07/26 00:36:46]

Done.

+    current_level = parent_level
+    for item in properties:
+      level = item.get('level', 'optional')
+      if level != current_level:
+        add_annotation(item, annotations[level])
+        current_level = level
+      if 'subdocs' in item:
+        annotate_recur(level, item['subdocs'])
+
+  annotate_recur('required', properties)
+
 class ManifestDataSource(object):
   '''Provides a template with access to manifest properties specific to apps or
   extensions.
@@ -23,68 +106,38 @@ class ManifestDataSource(object):
     self._cache = compiled_fs_factory.Create(
         self._CreateManifestData, ManifestDataSource)
 
-  def _ApplyAppsTransformations(self, manifest):
-    manifest['required'][0]['example'] = 'Application'
-    manifest['optional'][-1]['is_last'] = True
+  def _TransformForTemplates(self, master_dict, platform):
+    properties = deepcopy(master_dict)
+    _Filter(properties, platform)
+    properties = _ConvertAndSortDocs(properties)
+    _Annotate(properties)

[not at google - send to devlin, 2013/07/24 21:45:56]

I'd rather see this as

properties = _Annotate(_ConvertAndSortDocs(_Filter(master_dict, platform)))

you can probably use generators to implement it efficiently.

[jshumway, 2013/07/26 00:36:46]

Done.

+
+    if platform == 'app':
+      for item in properties:
+        if item['name'] == 'name':
+          item['example']['value'] = 'Application'
+          break

[not at google - send to devlin, 2013/07/24 21:45:56]

maybe we could do some slightly less hacky hackery. elsewhere in the templates
we use {{title}} to display the title, which the templates use in various
places. we could implement that hackery by hand here - replace {{title}} with
either 'Extension' or 'App' depending on the value of |platform|. I think that
would be preferable to this.

[jshumway, 2013/07/26 00:36:46]

Done.

+
+    properties[-1]['is_last'] = True
 
-  def _ApplyExtensionsTransformations(self, manifest):
-    manifest['optional'][-1]['is_last'] = True
+    return properties
 
   def _CreateManifestData(self, _, content):
-    '''Take the contents of |_manifest_path| and create apps and extensions
-    versions of a manifest example based on the contents of |_features_path|.
+    '''Combine the contents of |_manifest_path| and |_features_path| into a
+    master dictionary then create filtered lists to be used by templates.
     '''
-    def create_manifest_dict():
-      manifest_dict = OrderedDict()
-      for category in ('required', 'only_one', 'recommended', 'optional'):
-        manifest_dict[category] = []
-      return manifest_dict
+    manifest_json = Parse(content)
+    master = ProcessFeaturesFile(
+        Parse(self._file_system.ReadSingle(self._features_path)))

[not at google - send to devlin, 2013/07/24 21:45:56]

better name that "master"

[jshumway, 2013/07/26 00:36:46]

Done.

 
-    apps = create_manifest_dict()
-    extensions = create_manifest_dict()
+    MergeDictionaries(master, manifest_json)
+    _InsertSubDocs(master)
 
-    manifest_json = Parse(content)
-    features_json = Parse(self._file_system.ReadSingle(
-        self._features_path))
-
-    def add_property(feature, manifest_key, category):
-      '''If |feature|, from features_json, has the correct extension_types, add
-      |manifest_key| to either apps or extensions.
-      '''
-      added = False
-      extension_types = feature['extension_types']
-      if extension_types == 'all' or 'platform_app' in extension_types:
-        apps[category].append(deepcopy(manifest_key))
-        added = True
-      if extension_types == 'all' or 'extension' in extension_types:
-        extensions[category].append(deepcopy(manifest_key))
-        added = True
-      return added
-
-    # Property types are: required, only_one, recommended, and optional.
-    for category in manifest_json:
-      for manifest_key in manifest_json[category]:
-        # If a property is in manifest.json but not _manifest_features, this
-        # will cause an error.
-        feature = features_json[manifest_key['name']]
-        if add_property(feature, manifest_key, category):
-          del features_json[manifest_key['name']]
-
-    # All of the properties left in features_json are assumed to be optional.
-    for feature in features_json.keys():
-      item = features_json[feature]
-      # Handles instances where a features entry is a union with a whitelist.
-      if isinstance(item, list):
-        item = item[0]
-      add_property(item, {'name': feature}, 'optional')
-
-    apps['optional'].sort(key=itemgetter('name'))
-    extensions['optional'].sort(key=itemgetter('name'))
-
-    self._ApplyAppsTransformations(apps)
-    self._ApplyExtensionsTransformations(extensions)
-
-    return {'apps': apps, 'extensions': extensions}
+    return {
+      'apps': self._TransformForTemplates(master, 'app'),
+      'extensions': self._TransformForTemplates(master, 'extension'),
+      'master': master

[not at google - send to devlin, 2013/07/24 21:45:56]

why do you need to cache the master?

[jshumway, 2013/07/26 00:36:46]

I was talking with Evan about adding manifest properties to the into tables.
Exposing the 'master' dictionary (now named 'all_features') is an API for his
code to consume (and any other code that wants access to the manifest features
file).

It is necessary to expose 'all_features' separately because 'apps' and
'extensions' get mutilated pretty badly from their original dictionary form so
they work with the templates.

+    }
 
   def get(self, key):
     return self._cache.GetFromFile(self._manifest_path)[key]