Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
354073004:
Docserver: Add template support for object level availability (Closed)
|
Created:
6 years, 6 months ago by ahernandez Modified:
6 years, 5 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDocserver: Add template support for object level availability
BUG=233982
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281082
Patch Set 1 #
Total comments: 14
Patch Set 2 : Make abstractions #
Total comments: 3
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 17
Patch Set 5 : Cleanup #
Total comments: 6
Patch Set 6 : Display availability for properties #Patch Set 7 : Simpler CSS #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 31 (0 generated)
I added in some support for displaying object level availability in the templates. PTAL. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/api_data_source.py (left): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:132: def _GenerateType(self, type_): Looks like Rietveld got confused. Is there a way to generate a better diff? https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:100: lookup_path = [self._namespace.name] 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. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:108: 'types': self._GenerateTypes(self._namespace.types.values(), lookup_path), 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. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:174: if lookup_path[0] != self._namespace.name: 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. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:233: function_dict['returns'] = self._GenerateType(function.returns, []) 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. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source_test.py:1: #!/usr/bin/env python I reorganized a lot of the tests in this file because the setUp function was getting to be ridiculous (and a bit confusing). https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source_test.py:339: # Test nodes that have the same availability as their parent. There are only two tests for this because I'm only generating object level availability for types currently. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py (right): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/test_data/object_level_availability/tabs.py:30: 'type': 'any', I added a bunch of these 'type' keys so json_schema_compiler.model.Namespace would accept this json.
I didn't have time to properly review this, added a couple of high level comments and answered your question. I'll have a better look tomorrow and try it out. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/api_data_source.py (left): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:132: def _GenerateType(self, type_): On 2014/06/26 23:53:31, ahernandez.miralles wrote: > Looks like Rietveld got confused. Is there a way to generate a better diff? why? what happened? https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:100: lookup_path = [self._namespace.name] On 2014/06/26 23:53:31, ahernandez.miralles wrote: > 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. 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 :( https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:108: 'types': self._GenerateTypes(self._namespace.types.values(), lookup_path), On 2014/06/26 23:53:31, ahernandez.miralles wrote: > 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. 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. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:136: return self._node_availabilities.Lookup(*lookup_path).annotation 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. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:233: function_dict['returns'] = self._GenerateType(function.returns, []) On 2014/06/26 23:53:31, ahernandez.miralles wrote: > 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. ok :)
I have abstracted lookup_path away into a class. I think it's a bit more complex than you originally envisioned, but I think (read: hope) you'll like it. https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/server2/api_data_source.py:174: if lookup_path[0] != self._namespace.name: On 2014/06/26 23:53:31, ahernandez.miralles wrote: > 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. This comment is more for me to remember, but this check is no longer needed if we keep this current design. https://codereview.chromium.org/354073004/diff/20001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:146: self._lookup_path.remove('callback') I'm not sure if this should be restored or not. https://codereview.chromium.org/354073004/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:264: self._current_node.Ascend() I'm not sure if the whole purpose of the APINodeCursor Ascend and DescendTo methods are obvious, so I'll explain the idea I have in mind here. Each time a Generate?() method is called, the function will call current_node.DescendTo() to get availability data for that node if necessary. Once the function is done with that node, it calls current_node.Ascend() to return back to where it was at previously. By doing this, I think it is possible to eliminate having to pass around some sort of lookup_path object everywhere. https://codereview.chromium.org/354073004/diff/20001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:283: no_gen=True) I think I now understand what the deal with the blank paths is. Namespace objects contain a lot of values that don't need availability information generated, such as where I left this comment. By passing in a blank path, the check we discussed earlier prevented availability data from being generated. However, that was clearly confusing, so I added this no_gen keyword arg that just indicates whether or not availability data should be generated.
did you have a chance to look at the type inlining problem? it's probably better that you don't start with types, but instead just functions, until we can resolve that.
On 2014/06/27 18:28:42, kalman wrote: > did you have a chance to look at the type inlining problem? > > it's probably better that you don't start with types, but instead just > functions, until we can resolve that. I haven't looked at the inlining problem yet. Did you want me to reupload with functions? Or did you want me to wait until inlining is resolved to upload again?
On 2014/06/27 18:31:47, ahernandez.miralles wrote: > On 2014/06/27 18:28:42, kalman wrote: > > did you have a chance to look at the type inlining problem? > > > > it's probably better that you don't start with types, but instead just > > functions, until we can resolve that. > > I haven't looked at the inlining problem yet. Did you want me to reupload with > functions? Or did you want me to wait until inlining is resolved to upload > again? let's do functions in this patch, so yes, reupload with functions not types.
ping this CL when you've updated it to show availability for functions
Ping. PTAL.
sorry I didn't have time to properly look at this. I have a suggestion which will hopefully clean up the code a bunch. I tested this with some APIs that I *thought* had new functions and didn't see anything. I'll have a look again next week and notice it :) does this work with the availability of functions on types? https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:68: class _APINodeCursor(object): use of this class would be nicer if you used the with..as syntax. For example: class _APINodeCursor(object): def __init__(self): self._lookup_path = [] def Descend(self, val): class scope(object): def __enter__(self2): self._lookup_path.append(val) def __exit__(self2, _, __, ___): self._lookup_path.pop() return scope() def Print(self): print(self._lookup_path) then use it like: def _GenerateFunctions(self): with self._current_node.Descend('foo'): return [self._GenerateFunction(f) for f in functions.values()] and that's it.
On 2014/06/28 00:21:02, kalman wrote: > sorry I didn't have time to properly look at this. I have a suggestion which > will hopefully clean up the code a bunch. > > I tested this with some APIs that I *thought* had new functions and didn't see > anything. I'll have a look again next week and notice it :) > > does this work with the availability of functions on types? > > https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... > File chrome/common/extensions/docs/server2/api_data_source.py (right): > > https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... > chrome/common/extensions/docs/server2/api_data_source.py:68: class > _APINodeCursor(object): > use of this class would be nicer if you used the with..as syntax. For example: > > > > class _APINodeCursor(object): > def __init__(self): > self._lookup_path = [] > > def Descend(self, val): > class scope(object): > def __enter__(self2): > self._lookup_path.append(val) > def __exit__(self2, _, __, ___): > self._lookup_path.pop() > return scope() > > def Print(self): > print(self._lookup_path) > > > > then use it like: > > > def _GenerateFunctions(self): > with self._current_node.Descend('foo'): > return [self._GenerateFunction(f) for f in functions.values()] > > > and that's it. 1 correction: class _APINodeCursor(object): def __init__(self): self._lookup_path = [] def Descend(self, val): self._lookup_path.append(val) class scope(object): def __enter__(self2): pass def __exit__(self2, _, __, ___): self._lookup_path.pop() return scope() def Print(self): print(self._lookup_path)
On 2014/06/30 18:56:27, kalman wrote: > On 2014/06/28 00:21:02, kalman wrote: > > sorry I didn't have time to properly look at this. I have a suggestion which > > will hopefully clean up the code a bunch. > > > > I tested this with some APIs that I *thought* had new functions and didn't see > > anything. I'll have a look again next week and notice it :) > > > > does this work with the availability of functions on types? > > > > > https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... > > File chrome/common/extensions/docs/server2/api_data_source.py (right): > > > > > https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... > > chrome/common/extensions/docs/server2/api_data_source.py:68: class > > _APINodeCursor(object): > > use of this class would be nicer if you used the with..as syntax. For example: > > > > > > > > class _APINodeCursor(object): > > def __init__(self): > > self._lookup_path = [] > > > > def Descend(self, val): > > class scope(object): > > def __enter__(self2): > > self._lookup_path.append(val) > > def __exit__(self2, _, __, ___): > > self._lookup_path.pop() > > return scope() > > > > def Print(self): > > print(self._lookup_path) > > > > > > > > then use it like: > > > > > > def _GenerateFunctions(self): > > with self._current_node.Descend('foo'): > > return [self._GenerateFunction(f) for f in functions.values()] > > > > > > and that's it. > > 1 correction: > > > > class _APINodeCursor(object): > def __init__(self): > self._lookup_path = [] > > def Descend(self, val): > self._lookup_path.append(val) > class scope(object): > def __enter__(self2): > pass > def __exit__(self2, _, __, ___): > self._lookup_path.pop() > return scope() > > def Print(self): > print(self._lookup_path) It worked with your initial version. Or is there another reason for moving that line out?
Here's the upload with the added context manager. I thought I had already uploaded this, but I guess I forgot.
On 2014/06/30 18:59:29, ahernandez.miralles wrote: > On 2014/06/30 18:56:27, kalman wrote: > > On 2014/06/28 00:21:02, kalman wrote: > > > sorry I didn't have time to properly look at this. I have a suggestion which > > > will hopefully clean up the code a bunch. > > > > > > I tested this with some APIs that I *thought* had new functions and didn't > see > > > anything. I'll have a look again next week and notice it :) > > > > > > does this work with the availability of functions on types? > > > > > > > > > https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... > > > File chrome/common/extensions/docs/server2/api_data_source.py (right): > > > > > > > > > https://codereview.chromium.org/354073004/diff/40001/chrome/common/extensions... > > > chrome/common/extensions/docs/server2/api_data_source.py:68: class > > > _APINodeCursor(object): > > > use of this class would be nicer if you used the with..as syntax. For > example: > > > > > > > > > > > > class _APINodeCursor(object): > > > def __init__(self): > > > self._lookup_path = [] > > > > > > def Descend(self, val): > > > class scope(object): > > > def __enter__(self2): > > > self._lookup_path.append(val) > > > def __exit__(self2, _, __, ___): > > > self._lookup_path.pop() > > > return scope() > > > > > > def Print(self): > > > print(self._lookup_path) > > > > > > > > > > > > then use it like: > > > > > > > > > def _GenerateFunctions(self): > > > with self._current_node.Descend('foo'): > > > return [self._GenerateFunction(f) for f in functions.values()] > > > > > > > > > and that's it. > > > > 1 correction: > > > > > > > > class _APINodeCursor(object): > > def __init__(self): > > self._lookup_path = [] > > > > def Descend(self, val): > > self._lookup_path.append(val) > > class scope(object): > > def __enter__(self2): > > pass > > def __exit__(self2, _, __, ___): > > self._lookup_path.pop() > > return scope() > > > > def Print(self): > > print(self._lookup_path) > > It worked with your initial version. Or is there another reason for moving that > line out? only that one might expect to be able to call Descend() outside a "with" scope and have it actually do something.
I believe I have a solution to the doc inlining problem. I can upload the new patch when you are ready to see it.
https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:90: ''' nit: blank line here https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:93: self._lookup_path = [namespace_name] rather than doing this could you include |namespace_name| in the Lookup calls like self._node_availabilities.Lookup(self._namespace_name, *self._lookup_path) it would save a few comments to do with magical access of self._lookup_path[0]. https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:105: # lookup_path[-2] is this node's category (e.g. types, events, etc.). could you define the set of acceptable categories somewhere? then assert that this holds true on Descend, and here also include an "assert lookup_path[-2] in <whatever>" https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:126: # lookup_path[1] is always the node category (e.g. types, functions, etc.). ditto https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:133: # We want to maintain a working lookup_path, so only restore it I'd rather you always restored this, and in a try..finally so that exceptions don't accidentally cause it not to be restored. https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:145: if 'events' in self._lookup_path and 'callback' in self._lookup_path: so if there's an 'events' there *must* be a 'callback', right? you should assert that, then. https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:149: self._lookup_path = lookup_path_copy you should be able to do this without copy'ing; use lookup_path.index('callback') then pop() that index and insert() it back afterwards (again, in a try-finally). https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:179: self._lookup_path.extend(path) actually this is fine https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:181: self._lookup_path = self._lookup_path[:-len(path)] nit: slightly better to be consistent and mutate |lookup_path| rather than re-assigning it, like self._lookup_path[:] = ... https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:248: with self._current_node.Descend('types'): nice! https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:476: 'partial': self._template_cache.GetFromFile( nit: indent -=2 on these https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:477: '%sintro_tables/%s_message.html' % (PRIVATE_TEMPLATES, status)).Get(), nit: indent += 2 (relative to the previous line) here https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_data_source.py:479: 'scheduled': scheduled nit: I'm going to try making these objects alphabetically ordered from now on, so, we should here https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/templates/private/event.html (right): https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/templates/private/event.html:7: <span class="availability" style="color:magenta; font-family:'arial';">{{+availability.partial content:availability/}}</span> ok... take out the style property and make it a "class" and entry in the CSS file :) then we can talk about it not being pink. https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/templates/private/function_details.html (right): https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/templates/private/function_details.html:9: <span class="availability" style="color:magenta; font-family:'arial';">{{+availability.partial content:availability/}}</span> ditto https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/templates/private/property.html (right): https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/templates/private/property.html:11: <span class="availability" style="color: magenta; font-family: 'arial';">{{+availability.partial content:availability/}}</span> ditto https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... File chrome/common/extensions/docs/templates/private/type.html (right): https://codereview.chromium.org/354073004/diff/80001/chrome/common/extensions... chrome/common/extensions/docs/templates/private/type.html:7: <span class="availability" style="color: magenta; font-family: 'arial';">{{+availability.partial content:availability/}}</span> ditto
I added in the fix for the inlining issue as well since it wasn't a big change. PTAL. https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/static/sass/_api.scss (right): https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... chrome/common/extensions/docs/static/sass/_api.scss:52: color: magenta; I left this magenta because it stands out well for me when I'm testing. I'll revert this to something more proper when it's time to commit this.
https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/static/sass/_api.scss (right): https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... chrome/common/extensions/docs/static/sass/_api.scss:52: color: magenta; On 2014/07/01 20:27:46, ahernandez.miralles wrote: > I left this magenta because it stands out well for me when I'm testing. I'll > revert this to something more proper when it's time to commit this. no problem. I actually want to play with the style as well. I'm wondering if something similar to the deprecated style would work: https://developer.chrome.com/apps/app_window#property-CreateWindowOptions-min... maybe a bit obvious.
https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/static/sass/_api.scss (right): https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... chrome/common/extensions/docs/static/sass/_api.scss:52: color: magenta; On 2014/07/01 20:29:53, kalman wrote: > On 2014/07/01 20:27:46, ahernandez.miralles wrote: > > I left this magenta because it stands out well for me when I'm testing. I'll > > revert this to something more proper when it's time to commit this. > > no problem. I actually want to play with the style as well. I'm wondering if > something similar to the deprecated style would work: > > https://developer.chrome.com/apps/app_window#property-CreateWindowOptions-min... > > maybe a bit obvious. I just tried out a style similar to the deprecated style. I think it looks good; it stands out without having to be a really bright color. Not sure if it will be too much when we start displaying availability for all the nodes however.
code lgtm. when you say "like deprecated", I played with setting the class to "note" and it looks ok. perhaps start there and iterate. https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/api_data_source.py:120: # node name, thus lookup_path[-3] must be a node category. maybe I'm misunderstanding, but if this is an event callback then shouldn't lookup_path[-3] be 'event'? https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/api_data_source.py:170: callback_index = self._lookup_path.index('callback') nit: do this outside of the try block
I also played with the note class. After being tweaked it looks good for the function nodes, but I tried it with property nodes (these show up mostly in the table elements) and it doesn't look right. I tried changing it a bit more to look good in both cases, but I couldn't come up with anything completely satisfactory. Did you want me to upload the tweaked note class? https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... chrome/common/extensions/docs/server2/api_data_source.py:120: # node name, thus lookup_path[-3] must be a node category. On 2014/07/01 20:58:50, kalman wrote: > maybe I'm misunderstanding, but if this is an event callback then shouldn't > lookup_path[-3] be 'event'? Yes it should. Will change.
On 2014/07/01 21:04:34, ahernandez.miralles wrote: > I also played with the note class. After being tweaked it looks good for the > function nodes, but I tried it with property nodes (these show up mostly in the > table elements) and it doesn't look right. I tried changing it a bit more to > look good in both cases, but I couldn't come up with anything completely > satisfactory. Did you want me to upload the tweaked note class? Yeah, you might as well upload this with the availability being rendered for the properties too. Perhaps you could have a "note" for the functions and a "small-note" for the properties? Come to think of it, there are deprecated properties. Does it look equally weird? > > https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... > File chrome/common/extensions/docs/server2/api_data_source.py (right): > > https://codereview.chromium.org/354073004/diff/120001/chrome/common/extension... > chrome/common/extensions/docs/server2/api_data_source.py:120: # node name, thus > lookup_path[-3] must be a node category. > On 2014/07/01 20:58:50, kalman wrote: > > maybe I'm misunderstanding, but if this is an event callback then shouldn't > > lookup_path[-3] be 'event'? > > Yes it should. Will change.
I think it makes the whole thing look weirder. This patchset doesn't generate the correct availability for properties, because that will take a bit of work. I just made it so that something shows up for the availability, that way you can see how the pseudo-note class looks in table elements.
let's just submit something simple that looks decent and play with the display later.
wait - I'm running the cron and saw this:
ERROR 2014-07-01 22:08:57,373 cron_servlet.py:47] cron: APIDataSource: error
Traceback (most recent call last):
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/cron_servlet.py",
line 177, in resolve
future.Get()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 51, in Get
self._Raise()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 47, in Get
self._value = self._callback()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 19, in resolve
resolved.append(f.Get())
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 51, in Get
self._Raise()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 47, in Get
self._value = self._callback()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 687, in resolve
handlebar_dict = handlebar_dict_future.Get()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 51, in Get
self._Raise()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py",
line 47, in Get
self._value = self._callback()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 679, in resolve
self._LoadEventByName(platform)).ToDict()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 252, in ToDict
'properties': self._GenerateProperties(self._namespace.properties),
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 387, in _GenerateProperties
return [self._GenerateProperty(v) for v in properties.values()]
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 407, in _GenerateProperty
'properties': self._GenerateProperties(type_.properties),
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 387, in _GenerateProperties
return [self._GenerateProperty(v) for v in properties.values()]
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 412, in _GenerateProperty
'availability': self._GetAvailabilityTemplate()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 496, in _GetAvailabilityTemplate
availability_info = self._current_node.GetAvailability()
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 195, in GetAvailability
if node_availability == self._LookupParentNodeAvailability():
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 138, in _LookupParentNodeAvailability
*self._GetParentPath()).annotation
File
"/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py",
line 111, in _GetParentPath
'Tried to look up parent for the top-level node.'
AssertionError: Tried to look up parent for the top-level node.
On 2014/07/01 22:16:13, kalman wrote: > wait - I'm running the cron and saw this: > > ERROR 2014-07-01 22:08:57,373 cron_servlet.py:47] cron: APIDataSource: error > Traceback (most recent call last): > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/cron_servlet.py", > line 177, in resolve > future.Get() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 51, in Get > self._Raise() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 47, in Get > self._value = self._callback() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 19, in resolve > resolved.append(f.Get()) > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 51, in Get > self._Raise() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 47, in Get > self._value = self._callback() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 687, in resolve > handlebar_dict = handlebar_dict_future.Get() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 51, in Get > self._Raise() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/future.py", > line 47, in Get > self._value = self._callback() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 679, in resolve > self._LoadEventByName(platform)).ToDict() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 252, in ToDict > 'properties': self._GenerateProperties(self._namespace.properties), > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 387, in _GenerateProperties > return [self._GenerateProperty(v) for v in properties.values()] > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 407, in _GenerateProperty > 'properties': self._GenerateProperties(type_.properties), > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 387, in _GenerateProperties > return [self._GenerateProperty(v) for v in properties.values()] > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 412, in _GenerateProperty > 'availability': self._GetAvailabilityTemplate() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 496, in _GetAvailabilityTemplate > availability_info = self._current_node.GetAvailability() > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 195, in GetAvailability > if node_availability == self._LookupParentNodeAvailability(): > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 138, in _LookupParentNodeAvailability > *self._GetParentPath()).annotation > File > "/usr/local/google/home/kalman/local/gclient_chromium/src/chrome/common/extensions/docs/server2/api_data_source.py", > line 111, in _GetParentPath > 'Tried to look up parent for the top-level node.' > AssertionError: Tried to look up parent for the top-level node. That's because I didn't actually generate the property availability correctly. Do you still want me to include property availability, or shall I just upload with types/simple availability style?
> That's because I didn't actually generate the property availability correctly. > Do you still want me to include property availability, or shall I just upload > with types/simple availability style? I don't mind you implementing it now, but you mentioned it was hard? If it's not trivial then better to submit what you have I think.
Adding property availability is a bit tricky, so I reverted back to just function availability, as well as making the availability CSS simpler. I discovered a few subtle issues with some existing code, so I made those changes in this patch. PTAL.
lgtm https://codereview.chromium.org/354073004/diff/110016/chrome/common/extension... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/354073004/diff/110016/chrome/common/extension... chrome/common/extensions/docs/server2/api_data_source.py:204: self._lookup_path = current_path Again, please do this in a try..finally. Though I'd actually prefer passing the path around rather than changing self._lookup_path. That's a larger refactor, and probably goes for the other cases of try..finally. In the interest of not having continual diffs, let's do this for now. https://codereview.chromium.org/354073004/diff/110016/chrome/common/extension... File chrome/common/extensions/docs/templates/private/property.html (right): https://codereview.chromium.org/354073004/diff/110016/chrome/common/extension... chrome/common/extensions/docs/templates/private/property.html:12: {{/availability}} should you revert the changes to this file given you're not rendering properties yet?
The CQ bit was checked by ahernandez.miralles@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/35407300...
Message was sent while issue was closed.
Change committed as 281082 |
