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 d0c2c15208be535c59f6a4a4ce50b089750a6c60..2b9c02e952c9c753a110eb6810ee20419028cde6 100644
--- a/chrome/common/extensions/docs/server2/manifest_data_source.py
+++ b/chrome/common/extensions/docs/server2/manifest_data_source.py
@@ -3,9 +3,76 @@
 # found in the LICENSE file.
 
 from copy import deepcopy
-from operator import itemgetter
+import json
 
-from third_party.json_schema_compiler.json_parse import OrderedDict, Parse
+from features_model import FeaturesModel
+from third_party.json_schema_compiler.json_parse import Parse
+
+def _ConvertAndSortDocs(properties, platform):
+  '''Convert docs and children into lists and sort them, first by level, then
+  by name.
+  '''
+  def sort_key(item):
+    '''Key function to sort items primarily by level (according to index into
+    levels) then subsort by name.
+    '''
+    levels = ('required', 'recommended', 'only_one', 'optional')
+
+    return (levels.index(item.get('level', 'optional')), item['name'])
+
+  def convert_and_sort(properties):
+    for key, value in properties.items():
+      if 'example' in value:
+        value['has_example'] = True
+        example = json.dumps(value['example'])
+        if example == '{}':
+          value['example'] = '{...}'
+        elif example == '[]':
+          value['example'] = '[...]'
+        else:
+          value['example'] = example
+
+      if 'children' in value:
+        properties[key]['children'] = convert_and_sort(value['children'])
+
+    return sorted(properties.values(), key=sort_key)
+
+  name = properties['name']
+  name['example'] = name['example'].replace('{{title}}', platform.capitalize())
+
+  return convert_and_sort(properties)
+
+def _Annotate(properties):
+  '''Add level annotations to level groups in a manifest property list and
+  child lists. Requeries a list of properties sorted by level, with children
+  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(parent_level, properties):
+    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 'children' in item:
+        annotate(level, item['children'])
+    if properties:
+      properties[-1]['is_last'] = True
+
+  annotate('required', properties)
+  return properties
 
 class ManifestDataSource(object):
   '''Provides a template with access to manifest properties specific to apps or
@@ -22,68 +89,29 @@ 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 _ApplyExtensionsTransformations(self, manifest):
-    manifest['optional'][-1]['is_last'] = True
-
   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)
+    manifest_features = FeaturesModel().LoadFeaturesJson(
+        Parse(self._file_system.ReadSingle(self._features_path)))
+    manifest_features = manifest_features.MergeWith(manifest_json)
 
-    apps = create_manifest_dict()
-    extensions = create_manifest_dict()
+    apps_features = (
+        manifest_features.Filter(platform='app').RestructureChildren())
+    apps_features = _Annotate(_ConvertAndSortDocs(apps_features, 'app'))
 
-    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}
+    extensions_features = (
+        manifest_features.Filter(platform='extension').RestructureChildren())
+    extensions_features = _Annotate(
+        _ConvertAndSortDocs(extensions_features, 'extension'))

[not at google - send to devlin, 2013/07/31 00:36:39]

So we should probably be doing the RestructureChildren thing in manifest data
source, not in the features model.

The data that you pass to the templates generally is not the pure model. You
need to mutate the model in some way (e.g. adding the has_example things). This
child-nesting thing is a specific requirement of manifest_data_source, so it
belongs here, not as part of the pure model.

It's as simple as changing this to

apps_features =
_Annotate(_ConvertAndSortDocs(_WithNestedChildren(features.Filter(platform='app'))))

then again, I'd probably do something similar to what api_data_source does and
have that be a separate object (JSCModel) so that you can cache it nicely. then
do it all in one go.

(you can still test that method individually, though, from
manifest_data_source_test)

[jshumway, 2013/07/31 18:10:28]

Done.

+
+    return {
+      'apps': apps_features,
+      'extensions': extensions_features,
+      'all_features': manifest_features

[not at google - send to devlin, 2013/07/31 00:36:39]

similar to comment above, I don't think this all features thing belongs here,
it's different - apps/extensions are objects that have been mutated for passing
to templates, whereas all_features is the pure model.

I'd make the all_features thing a separate method on this object, like
ManifestDataSource.GetManifestFeatures(), then this method return the
apps/extensions thing like you have.

[jshumway, 2013/07/31 18:10:28]

Done.

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