Docserver: Add template support for object level availability

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

Index: chrome/common/extensions/docs/server2/api_data_source.py
diff --git a/chrome/common/extensions/docs/server2/api_data_source.py b/chrome/common/extensions/docs/server2/api_data_source.py
index f81dbf2eee63e7b7100b0d15605bed44cc994e22..79935f364f0ce01be35970d58c348e1371fc0f44 100644
--- a/chrome/common/extensions/docs/server2/api_data_source.py
+++ b/chrome/common/extensions/docs/server2/api_data_source.py
@@ -72,12 +72,14 @@ class _JSCModel(object):
 
   def __init__(self,
                namespace,
-               availability,
+               availability_finder,
                json_cache,
                template_cache,
                features_bundle,
                event_byname_future):
-    self._availability = availability
+    self._availability = availability_finder.GetAPIAvailability(namespace.name)
+    self._node_availabilities = availability_finder.GetAPINodeAvailability(
+        namespace.name)
     self._api_availabilities = json_cache.GetFromFile(
         posixpath.join(JSON_TEMPLATES, 'api_availabilities.json'))
     self._intro_tables = json_cache.GetFromFile(
@@ -95,6 +97,7 @@ class _JSCModel(object):
     if self._namespace is None:
       return {}
     chrome_dot_name = 'chrome.%s' % self._namespace.name
+    lookup_path = [self._namespace.name]

[ahernandez, 2014/06/26 23:53:31]

On the previous patch you mentioned an issue with inlined docs. Could you
explain that in a little more detail? I didn't quite understand what you were
talking about.

[not at google - send to devlin, 2014/06/27 00:22:54]

The issues is that when you put the "inline_doc" annotation on nodes we inline
those nodes way down in the stack. For example,

{ "id": "MyType", "type": "string", "inline_doc": true }
{ "name": "myFunction", "params": {"$ref": "MyType"}}

will actually end up like

{ "name": "myFunction", "params": {"type": "string"}}

because MyType has "inline_doc" on it. This throws off the availability finder.
If Chrome 30 *removes* the "inline_doc" from MyType then the node availability
will say that the type was added in Chrome 30, when it was really added in
Chrome ... whatever. Let's say 25.

So that's a problem, and it's made much worse because we automatically apply
inline_doc to types that are only used once. In the above example, even if
"inline_doc" *wasn't* on MyType it would be inlined:

{ "id": "MyType", "type": "string"}
{ "name": "myFunction", "params": {"$ref": "MyType"}}

-->

{ "name": "myFunction", "params": {"type": "string"}}

however if we were then to add another function which refers to MyType, it won't
be inlined:

{ "id": "MyType", "type": "string"}
{ "name": "myFunction", "params": {"$ref": "MyType"}}
{ "name": "myFunction2", "params": {"$ref": "MyType"}}

-->

{ "id": "MyType", "type": "string"}
{ "name": "myFunction", "params": {"$ref": "MyType"}}
{ "name": "myFunction2", "params": {"$ref": "MyType"}}

so the effect on adding "myFunction2" makes it appear as though MyType was added
in Chrome 30, even though it wasn't.

Does that make sense?

I'm not sure the best way to solve it, but I would start by looking at ignoring
all doc inlining at the availability finder stage, trying to add those
annotations before the inlining takes place? sorry it's so complicated :(

     as_dict = {
       'name': self._namespace.name,
       'namespace': self._namespace.documentation_options.get('namespace',
@@ -102,7 +105,7 @@ class _JSCModel(object):
       'title': self._namespace.documentation_options.get('title',
                                                          chrome_dot_name),
       'documentationOptions': self._namespace.documentation_options,
-      'types': self._GenerateTypes(self._namespace.types.values()),
+      'types': self._GenerateTypes(self._namespace.types.values(), lookup_path),

[ahernandez, 2014/06/26 23:53:31]

I'm only generating object level availability for types in this upload to keep
things simple. I'll add in the rest once these additions are good to go.

[not at google - send to devlin, 2014/06/27 00:22:54]

thanks!

I wonder if there's a way to avoid passing lookup_path around everywhere, it's
an eyesore. not sure if putting it on |self| would be any better.

       'functions': self._GenerateFunctions(self._namespace.functions),
       'events': self._GenerateEvents(self._namespace.events),
       'domEvents': self._GenerateDomEvents(self._namespace.events),
@@ -126,17 +129,89 @@ class _JSCModel(object):
   def _IsExperimental(self):
     return self._namespace.name.startswith('experimental')
 
-  def _GenerateTypes(self, types):
-    return [self._GenerateType(t) for t in types]
+  def _LookupNodeAvailability(self, lookup_path):
+    '''Returns the ChannelInfo object for the node represented by
+    |lookup_path|.
+    '''
+    return self._node_availabilities.Lookup(*lookup_path).annotation

[not at google - send to devlin, 2014/06/27 00:22:54]

I think we should make |lookup_path| a little class. there are a lot of comments
on index accesses explaining subtle things, these could actually be methods.
likewise a Push/Pop type interface would be nice.

the class can just be declared inside this file, and it doesn't need to be very
complex.

+
+  def _CheckNamespacePrefix(self, lookup_path):
+    '''API schemas may prepend the namespace name to top-level types
+    (e.g. declarativeWebRequest > types > declarativeWebRequest.IgnoreRules),
+    but just the base name (here, 'IgnoreRules') will be in the |lookup_path|.
+    Try creating an alternate |lookup_path| by adding the namespace name.
+    '''
+    # lookup_path[0] is always the API namespace, and
+    # lookup_path[1] is always the node category (e.g. types, functions, etc.).
+    # Thus, lookup_path[2] is always the top-level node name.
+    base_name = lookup_path[2]
+    lookup_path[2] = '%s.%s' % (self._namespace.name, base_name)
+    node_availability = self._LookupNodeAvailability(lookup_path)
+    if node_availability is not None:
+      return node_availability
+    # We want to maintain a working lookup_path, so only restore it
+    # if modifying the lookup_path did not work.
+    lookup_path[2] = base_name
+    return None
 
-  def _GenerateType(self, type_):

[ahernandez, 2014/06/26 23:53:31]

Looks like Rietveld got confused. Is there a way to generate a better diff?

[not at google - send to devlin, 2014/06/27 00:22:54]

why? what happened?

+  def _CheckEventCallback(self, lookup_path):
+    '''Within API schemas, an event has a list of 'properties' that the event's
+    callback expects. The callback itself is not explicitly represented in the
+    schema. However, when creating an event node in _JSCModel, a callback node
+    is generated and acts as the parent for the event's properties.
+    Modify |lookup_path| to check the original schema format.
+    '''
+    if 'events' in lookup_path and 'callback' in lookup_path:
+      lookup_path_copy = copy(lookup_path)
+      lookup_path_copy.remove('callback')
+      return self._LookupNodeAvailability(lookup_path_copy)
+    return None
+
+  def _GetNodeAvailability(self, lookup_path):
+    '''Uses an APISchemaGraph instance to find availability information for
+    a node pointed to by |lookup_path|.
+    '''
+    if lookup_path[0] != self._namespace.name:

[ahernandez, 2014/06/26 23:53:31]

There are many instances where a blank lookup path is given (see later comment),
so lookup_path[0] is not always equal to namespace.name.

[ahernandez, 2014/06/27 03:00:01]

This comment is more for me to remember, but this check is no longer needed if
we keep this current design.

+      # |lookup_path| won't be lookup up if it doesn't start with the API name.
+      return None
+
+    assert len(lookup_path) > 2, \
+            'Tried to look up parent availability for the top-level node.'
+
+    for lookup in (self._LookupNodeAvailability,
+                   self._CheckEventCallback,
+                   self._CheckNamespacePrefix):
+      node_availability = lookup(lookup_path)
+      if node_availability is not None:
+        break
+
+    if node_availability is None:
+      logging.warning('No availability found for: %s' % ' > '.join(lookup_path))
+      return None
+
+    # lookup_path[-1] is the node we found availability for.
+    # lookup_path[-2] is the node category (e.g. types, events, etc.).
+    # Thus, the parent node is described by lookup_path[:-2].
+    if node_availability == self._LookupNodeAvailability(lookup_path[:-2]):
+      # Only render this node's availability if it differs from the parent
+      # node's availability.
+      return None
+    return self._GetAvailabilityTemplate(node_availability.channel,
+                                         node_availability.version)
+
+  def _GenerateTypes(self, types, lookup_path):
+    return [self._GenerateType(t, lookup_path + ['types']) for t in types]
+
+  def _GenerateType(self, type_, lookup_path):
+    next_lookup_path = lookup_path + [type_.simple_name]
     type_dict = {
       'name': type_.simple_name,
       'description': type_.description,
       'properties': self._GenerateProperties(type_.properties),
       'functions': self._GenerateFunctions(type_.functions),
       'events': self._GenerateEvents(type_.events),
-      'id': _CreateId(type_, 'type')
+      'id': _CreateId(type_, 'type'),
+      'availability': self._GetNodeAvailability(next_lookup_path)
     }
     self._RenderTypeInformation(type_, type_dict)
     return type_dict
@@ -155,7 +230,7 @@ class _JSCModel(object):
     }
     self._AddCommonProperties(function_dict, function)
     if function.returns:
-      function_dict['returns'] = self._GenerateType(function.returns)
+      function_dict['returns'] = self._GenerateType(function.returns, [])

[ahernandez, 2014/06/26 23:53:31]

Here is an example of the blank lookup path. I think all the TODOs Evan wrote
about "blank path" were related to this, but I don't know if he intended to pass
in a different path, or if he wrote them as a reminder to pass a blank path and
never deleted the TODOs. It seems that there's no way to get any availability if
a blank path is passed in, but as far as I could tell Evan's patch was indeed
displaying availability for all nodes. 

tl;dr I'm not sure what the deal is with the blank path TODOs.

[not at google - send to devlin, 2014/06/27 00:22:54]

ok :)

     for param in function.params:
       function_dict['parameters'].append(self._GenerateProperty(param))
     if function.callback is not None:
@@ -266,7 +341,7 @@ class _JSCModel(object):
       for param in function.params:
         property_dict['parameters'].append(self._GenerateProperty(param))
       if function.returns:
-        property_dict['returns'] = self._GenerateType(function.returns)
+        property_dict['returns'] = self._GenerateType(function.returns, [])
 
     value = property_.value
     if value is not None:
@@ -296,7 +371,7 @@ class _JSCModel(object):
   def _RenderTypeInformation(self, type_, dst_dict):
     dst_dict['is_object'] = type_.property_type == model.PropertyType.OBJECT
     if type_.property_type == model.PropertyType.CHOICES:
-      dst_dict['choices'] = self._GenerateTypes(type_.choices)
+      dst_dict['choices'] = self._GenerateTypes(type_.choices, [])
       # We keep track of which == last for knowing when to add "or" between
       # choices in templates.
       if len(dst_dict['choices']) > 0:
@@ -304,7 +379,7 @@ class _JSCModel(object):
     elif type_.property_type == model.PropertyType.REF:
       dst_dict['link'] = self._GetLink(type_.ref_type)
     elif type_.property_type == model.PropertyType.ARRAY:
-      dst_dict['array'] = self._GenerateType(type_.item_type)
+      dst_dict['array'] = self._GenerateType(type_.item_type, [])
     elif type_.property_type == model.PropertyType.ENUM:
       dst_dict['enum_values'] = [
           {'name': value.name, 'description': value.description}
@@ -336,6 +411,17 @@ class _JSCModel(object):
 
     return intro_rows
 
+  def _GetAvailabilityTemplate(self, status, version=None, scheduled=None):
+    '''Returns an object that the templates use to display availability
+    information.
+    '''
+    return {
+        'partial': self._template_cache.GetFromFile(
+          '%sintro_tables/%s_message.html' % (PRIVATE_TEMPLATES, status)).Get(),
+        'version': version,
+        'scheduled': scheduled
+    }
+
   def _GetIntroDescriptionRow(self):
     ''' Generates the 'Description' row data for an API intro table.
     '''
@@ -359,14 +445,11 @@ class _JSCModel(object):
       scheduled = self._availability.scheduled
     return {
       'title': 'Availability',
-      'content': [{
-        'partial': self._template_cache.GetFromFile(
-          posixpath.join(PRIVATE_TEMPLATES,
-                         'intro_tables',
-                         '%s_message.html' % status)).Get(),
-        'version': version,
-        'scheduled': scheduled
-      }]
+      'content': [
+        self._GetAvailabilityTemplate(status,
+                                      version=version,
+                                      scheduled=scheduled)
+      ]
     }
 
   def _GetIntroDependencyRows(self):
@@ -506,8 +589,7 @@ class APIDataSource(DataSource):
       if jsc_model is None:
         jsc_model = _JSCModel(
             model_future.Get(),
-            self._platform_bundle.GetAvailabilityFinder(
-                platform).GetAPIAvailability(api_name),
+            self._platform_bundle.GetAvailabilityFinder(platform),
             self._json_cache,
             self._template_cache,
             self._platform_bundle.GetFeaturesBundle(platform),