Doc server manifest page generation

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

+# Copyright (c) 2013 The Chromium Authors. All rights reserved.

[not at google - send to devlin, 2013/05/10 04:14:14]

no (c)

[jshumway, 2013/05/11 02:37:08]

Done.

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+from collections import defaultdict
+import json

[not at google - send to devlin, 2013/05/10 04:14:14]

pls use third_party.json_schema_compiler.json_parse

also means you won't need the Nom'ing, much as it's a cool name.

[jshumway, 2013/05/11 02:37:08]

It is a cool function name, changed.

+
+from third_party.json_schema_compiler.json_comment_eater import Nom
+
+class ManifestDataSource(object):
+  """ Provides a template with access to manifest properties specific to apps or
+  extensions.
+  """
+  def __init__(self, compiled_fs_factory, manifest_path, features_path, channel):

[not at google - send to devlin, 2013/05/10 04:14:14]

line is too long

[jshumway, 2013/05/11 02:37:08]

Done.

+    # Path to manifest.json
+    self._manifest_path = manifest_path
+    # Path to _manifest_features.json
+    self._features_path = features_path
+    # Used to get raw json files.
+    self._compiled_fs = compiled_fs_factory.Create(
+      lambda _, contents: json.loads(Nom(contents)), ManifestDataSource)
+    # Used to cache the final apps and extensions manifest json objects.

[not at google - send to devlin, 2013/05/10 04:14:14]

None of these comments add much IMO.

Three comments with the cached file system:
(1) unless they have category= they'll share the same namespace, which would be
bad, so they need a category=.
(2) perhaps json_cache rather than compiled_fs.
(3) 4 space indent when wrapping lines, not 2.

[jshumway, 2013/05/11 02:37:08]

Done.

+    self._cache = compiled_fs_factory.Create(
+      self._CreateManifestData, ManifestDataSource)
+

[not at google - send to devlin, 2013/05/10 04:14:14]

no \n

[jshumway, 2013/05/11 02:37:08]

Done.

+    self._channel = channel
+
+  def _ApplyAppsTransformations(self, manifest):
+    manifest['required'][0]['example'] = 'My Application'

[not at google - send to devlin, 2013/05/10 04:14:14]

just App

[jshumway, 2013/05/11 02:37:08]

Done.

+    manifest['optional'][-1]['is_last'] = True
+
+  def _ApplyExtensionsTransformations(self, manifest):
+    manifest['optional'][-1]['is_last'] = True
+
+  def _LoadManifestData(self, content):
+    """Helper function to make testing easier.
+    """

[not at google - send to devlin, 2013/05/10 04:14:14]

how does this make testing easier? what does it have to do with manifest
specifically?

in any case... please use json_parse from json_schema_compiler here. It gets the
OrderedDict treatment, which may become important.

This method probably isn't much value then.

[jshumway, 2013/05/11 02:37:08]

Removed it.

+    return json.loads(Nom(content))
+
+  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|.
+    """
+    manifest_json = self._LoadManifestData(content)
+    features_json = self._compiled_fs.GetFromFile(self._features_path)

[not at google - send to devlin, 2013/05/10 04:14:14]

... that said about the caches, I don't think this extra cache is actually doing
anything, since this method will only ever be called once and the result will be
cached.

I.e. reading from the file system would be just as good, and put less strain on
memcache.

[jshumway, 2013/05/11 02:37:08]

Done.

+    apps = defaultdict(list)
+    extensions = defaultdict(list)
+
+    channels = {"stable": 0, "beta": 1, "dev": 2, "trunk": 3}

[not at google - send to devlin, 2013/05/10 04:14:14]

try to use ' not " for strings. Less RSI.

[jshumway, 2013/05/11 02:37:08]

My mistake

+    app_types = frozenset(['packaged_app', 'hosted_app', 'platform_app'])

[not at google - send to devlin, 2013/05/10 04:14:14]

Just platform_app.

I guess this logic will become simpler.

Generally though, if you're creating an immutable list then use tuples not lists
i.e. frozenset((..., ..., ...))

[jshumway, 2013/05/11 02:37:08]

Changed.

+
+    def add_property(feature, mproperty, type_='optional'):
+      """If |feature|, from features_json, has the correct extension_types, add
+      |mproperty| to either apps or extensions.
+      """
+      added = False
+
+      if feature['extension_types'] == 'all':
+        apps[type_].append(mproperty)
+        extensions[type_].append(mproperty)
+        return True
+      # The following two conditions are not mutally exclusive.
+      if app_types.intersection(feature['extension_types']):
+        apps[type_].append(mproperty)
+        added = True
+      if 'extension' in feature['extension_types']:
+        extensions[type_].append(mproperty)
+        added = True
+      return added

[not at google - send to devlin, 2013/05/10 04:14:14]

Also as I mentioned in IRC - we will need to group nested features, e.g. render
app.background as though it's inside app (you can imagine how that should look).

But do it in a follow-up. There will be some trickier things like what to do
about it in terms of required vs not-required. Maybe it should all be done
inside manifest.json rather than parsing the features file?

Let's decide later.

+
+    # Property types are: required, only_one, recommended, and optional.
+    for property_type in manifest_json:
+      for mproperty in manifest_json[property_type]:

[not at google - send to devlin, 2013/05/10 04:14:14]

mproperty -> manifest_key?

[jshumway, 2013/05/11 02:37:08]

Done.

+        # If a property is in manifest.json but not _manifest_features, this
+        # will cause an error.
+        feature = features_json[mproperty['name']]
+        # Only add properties that are available in a certain channel.
+        if channels[feature['channel']] <= channels[self._channel]:

[not at google - send to devlin, 2013/05/10 04:14:14]

interesting. This is something we'll need to think about how to surface when we
only show 'trunk'. For now, I'm not sure whether it's worth the extra code,
it'll probably change when Evan's availability stuff is finished.

So I reckon just show them no matter what. In a follow-up we can think about
generating a comment like "dev only" and/or showing a warning in the manifest
overview pages.

[jshumway, 2013/05/11 02:37:08]

Done.

+          if add_property(feature, mproperty, property_type):
+            del features_json[mproperty['name']]

[not at google - send to devlin, 2013/05/10 04:14:14]

makes me nervous, better to take a copy of it before you mutate objects that are
owned by other things - but since this will come from a file system later,
that's fine.

[jshumway, 2013/05/11 02:37:08]

Done.

+
+    # All of the properties left in features_json are assumed to be optional.
+    for feature in sorted(features_json.keys()):

[not at google - send to devlin, 2013/05/10 04:14:14]

as I mentioned - it looks a bit odd the way it is with half of the optional
things sorted then the other half sorted. Just combine them into one.

[jshumway, 2013/05/11 02:37:08]

Done.

+      item = features_json[feature]
+      # In two cases, a features_json entry is a list for no obvious reason.

[not at google - send to devlin, 2013/05/10 04:14:14]

it represents union :) one is usually for whitelisting. You can ignore that one.

[jshumway, 2013/05/11 02:37:08]

I changed the comment.

+      if isinstance(item, list):
+        item = item[0]
+      if channels[item['channel']] <= channels[self._channel]:
+        add_property(item, {'name': feature})
+
+    self._ApplyAppsTransformations(apps)
+    self._ApplyExtensionsTransformations(extensions)
+
+    return {'apps': apps, 'extensions': extensions}
+
+  def get(self, key):
+    return self._cache.GetFromFile(self._manifest_path)[key]