Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
48263002:
list apis by channel info, e.g. dev, stable (Closed)
|
Created:
7 years, 1 month ago by hukun Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionlist apis by channel info, e.g. dev, stable
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235124
Patch Set 1 #
Total comments: 24
Patch Set 2 : use api models #
Total comments: 48
Patch Set 3 : gerenate one new template, e.g. api_table #
Total comments: 22
Patch Set 4 : add private_apis html #Patch Set 5 : #
Total comments: 22
Patch Set 6 : polish test data in api list tests #
Total comments: 12
Patch Set 7 : polish api list test data #
Total comments: 4
Patch Set 8 : fix last issue #
Total comments: 2
Patch Set 9 : change api array expression #Patch Set 10 : done #Messages
Total messages: 31 (0 generated)
List all apis by channel, e.g. stable, dev, and beta
Sorry it took me so long to review. It's a bit vague in places but hopefully cleaning up the templates will lead to APIListDataSource being better-structured. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:42: api_data_source_factory, I don't want to introduce a dependency on api_data_source_factory here. However, I've put https://codereview.chromium.org/46243003/ through the CQ and hopefully it'll be submitted soon (by the time you read this?). You should be able to pass in an api_models class and use that: get the model, then read its description. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:57: self._cachhed_api_features = None the places you get these from do their own caching, you don't need to cache it here too (probably means you don't need those other methods). https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:109: api_data_source): this should still [be able to be arranged to] have access to availability_finder and api_data_source via self, they don't need to be passed in. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:127: I'm finding this sa bit hard to follow, can you break it up somehow? https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:135: 'apps': MakeDictForChannel(MakeDictForPlatform('apps'), MakeDictForPlatform('apps') here was right. MakeDictForPlatform should be responsible for separating that into channels. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:8: {{:is_apps}} with respect to the comment below: just delete the second case here and always show chrome.runtime and chrome.alarms (no is_apps check ever). https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:23: Stable APIs h2 not p https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:26: <tr><td>API Name</td><td>API Description</td><td>Chrome Version</td></tr> use th here. Also I'd just go with "Name" and "Description", the "API" is implicit. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:28: {{#api_list.apps.chrome.stable}} cleanup: change templates/public/apps/api_index.html and templates/public/extensions/api_index.html to inject variables "stable_apis", "beta_apis", and "dev_apis" rather than "is_apps". Then use those here. For example, converting public/apps/api_index: {{+partials.standard_apps_article article:intros.api_index is_apps:true}} would change to {{+partials.standard_apps_article article:intros.api_index stable_apis:api_list.apps.stable beta_apis:api_list.apps.beta dev_apis:api_list.apps.dev}} then here {{stable_apis}} <tr>..</tr> {{/stable_apis}} https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:99: </p> hrm maybe we should move the trunk and experimental lists into this file too. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/articles/experimental.html (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/experimental.html:14: <p> I wouldn't worry about changing this file. It's wrong at the moment anyway. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/private/extensions_footer.html (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/private/extensions_footer.html:8: {{*api_list.extensions.experimental.dev}}) maybe the api list data source just needs a way to return all APIs rather than this concat() stuff. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/private/samples.html (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/private/samples.html:31: Dev APIs:{{#filter_list.dev}}{{+partials.filter_item}}{{/}} I'm not sure we want to do this?
I'm adding my initial feedback in this message rather than directly in the files. Hope that is OK. ** Can we put each table in a section? This means making "Stable APIs" a <h3> instead of <p>, same for Dev and Beta. ** We should replace the sentence at the top of the page, "Here are the supported..." with a sentence at the beginning of each of the new sections. ** Stable APIs intro sentence: "Here are the supported chrome.* APIs stable since the Chrome version." ** Beta APIs intro sentence: "Here are the supported chrome.* APIs available in Chrome Beta only." ** Dev APIs intro sentence: "Here are the supported chrome.* APIs available in Chrome Dev only." ** Bold content in table headers. ** Chrome version seems way off-- Ben might be able to identify why. ** If a table has no data, can we choose not to display it? For example, Beta APIs table is empty. ** +kalman: do we want to display experimental APIs in a table?
https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:42: api_data_source_factory, Done. Remove dependency of api_data_source_factory, and use api_models. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:57: self._cachhed_api_features = None Done.remove the cache. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:109: api_data_source): Done. Use self' point directly. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:127: Done. Remove my func. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_list_data_source.py:135: 'apps': MakeDictForChannel(MakeDictForPlatform('apps'), Done. Still use MakeDictForPlatform https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:8: {{:is_apps}} Done https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:23: Stable APIs Done https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:26: <tr><td>API Name</td><td>API Description</td><td>Chrome Version</td></tr> Done https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:28: {{#api_list.apps.chrome.stable}} Done https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/articles/api_index.html:99: </p> I will listen to your decision to go ahead. https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/private/extensions_footer.html (right): https://codereview.chromium.org/48263002/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/private/extensions_footer.html:8: {{*api_list.extensions.experimental.dev}}) Done
On Thu, Oct 31, 2013 at 8:11 AM, <mkearney@chromium.org> wrote: > I'm adding my initial feedback in this message rather than directly in the > files. Hope that is OK. > > ** Can we put each table in a section? This means making "Stable APIs" a > <h3> > instead of <p>, same for Dev and Beta. > > Done, use h2 according to ben' comment > ** We should replace the sentence at the top of the page, "Here are the > supported..." with a sentence at the beginning of each of the new sections. > > Done > ** Stable APIs intro sentence: "Here are the supported chrome.* APIs stable > since the Chrome version." > > ** Beta APIs intro sentence: "Here are the supported chrome.* APIs > available in > Chrome Beta only." > > ** Dev APIs intro sentence: "Here are the supported chrome.* APIs > available in > Chrome Dev only." > > ** Bold content in table headers. > > Done > ** Chrome version seems way off-- Ben might be able to identify why. > > ** If a table has no data, can we choose not to display it? For example, > Beta > APIs table is empty. > > Done > ** +kalman: do we want to display experimental APIs in a table? > > https://codereview.chromium.org/48263002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looking almost done, Kun. The Chrome Version is showing up as 5 for all APIs. Is this correct? https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:11: Here are the supported chrome.* APIs stable since the Chrome version. Move this <p> after <h3>. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:31: Here are the supported chrome.* APIs available in Chrome Beta only. Move this <p> after <h3>. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:34: <h3 id="beta_apis"> Can we have a sentence here: "Here are the supported chrome.* APIs available in Chrome Beta only." And have it only appear if the table exists? https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:51: </p> Move this <p> to come after <h3>
Nevermind the top comment-- I was running ./preview.py instead of proper start_dev_server.py. Versioning looks great! On Fri, Nov 1, 2013 at 10:15 AM, <mkearney@chromium.org> wrote: > Looking almost done, Kun. > > The Chrome Version is showing up as 5 for all APIs. Is this correct? > > > https://codereview.chromium.**org/48263002/diff/110001/** > chrome/common/extensions/docs/**templates/articles/api_index.**html<https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions/docs/templates/articles/api_index.html> > File chrome/common/extensions/docs/**templates/articles/api_index.**html > (right): > > https://codereview.chromium.**org/48263002/diff/110001/** > chrome/common/extensions/docs/**templates/articles/api_index.** > html#newcode11<https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions/docs/templates/articles/api_index.html#newcode11> > chrome/common/extensions/docs/**templates/articles/api_index.**html:11: > Here > > are the supported chrome.* APIs stable since the Chrome version. > Move this <p> after <h3>. > > https://codereview.chromium.**org/48263002/diff/110001/** > chrome/common/extensions/docs/**templates/articles/api_index.** > html#newcode31<https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions/docs/templates/articles/api_index.html#newcode31> > chrome/common/extensions/docs/**templates/articles/api_index.**html:31: > Here > > are the supported chrome.* APIs available in Chrome Beta only. > Move this <p> after <h3>. > > https://codereview.chromium.**org/48263002/diff/110001/** > chrome/common/extensions/docs/**templates/articles/api_index.** > html#newcode34<https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions/docs/templates/articles/api_index.html#newcode34> > chrome/common/extensions/docs/**templates/articles/api_index.**html:34: > <h3 > id="beta_apis"> > Can we have a sentence here: "Here are the supported chrome.* APIs > available in Chrome Beta only." And have it only appear if the table > exists? > > https://codereview.chromium.**org/48263002/diff/110001/** > chrome/common/extensions/docs/**templates/articles/api_index.** > html#newcode51<https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions/docs/templates/articles/api_index.html#newcode51> > chrome/common/extensions/docs/**templates/articles/api_index.**html:51: > </p> > Move this <p> to come after <h3> > > https://codereview.chromium.**org/48263002/<https://codereview.chromium.org/4... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
the demo is awesome! https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:79: def GetAllNames(self): don't worry about these GetAllNames changes, I'm going to delete them in https://codereview.chromium.org/56083005/. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:88: return (api for api in api_features.itervalues() I think you could even just inline the whole of api_features in here. return (api for api in self._features_bundle.GetAPIFeatures().itervalues() if platform in api['platforms']) https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:98: 'dev': [],'trunk': [] }} Hm, so, how about we change the way this works. - do away with 'chrome' and 'experimental'. just have 'stable', 'beta', 'dev', 'trunk', and 'experimental' at the top level. - factor templates accordingly. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:104: if category == 'chrome' or category == 'experimental': if category in ('chrome', 'experimental'): though I might just implement this in an else: clause. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:109: api['description'] = model.Get().description when is there not a model? there should always be a model... https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:114: channel = channel_info.channel inline this https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:120: extra blank line https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:126: continue why do you need this? an empty list would be simpler I think. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:131: all_apis = sorted(all_apis, key=itemgetter('name')) extra space https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:132: platform_dict['all_apis'] = all_apis you could do just all_apis.sort(key=itemgetter('name')). however, I'd just inline it: platform_dict.update({ 'all_apis': sorted(all_apis, key=itemgetter('name')), 'private': sorted(all_apis, key=itemgetter('name')), }) https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:150: def GetAllNames(self): ditto. I'm going to delete this. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:18: <tr><th>Name</th><th>Description</th><th>Chrome Version</th></tr> I think just "Since" looks better than "Chrome Version". https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:65: {{/}} what about trunk? perhaps pull this the table into its own template file, passing in the API list, like: === api_table.html === <table> ... {{#apis}} ... {{/apis}} </table> then use here like {{+api_table apis:beta_apis}} {{+api_table apis:dev_apis}} {{+api_table apis:trunk_apis}} {{+api_table apis:experimental_apis}} or similar. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/experimental.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/experimental.html:71: just include the experimental section in here. see previous comment about the separate template to use here. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/private/samples.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/private/samples.html:31: {{?filter_list.dev}}Dev APIs:{{#filter_list.dev}}{{+partials.filter_item}}{{/}}{{/}} I think it's ok having these unfiltered? Just lump them all in together, at least for now. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/apps/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/apps/api_index.html:1: {{+partials.standard_apps_article article:intros.api_index stable_apis:api_list.apps.chrome.stable beta_apis:api_list.apps.chrome.beta dev_apis:api_list.apps.chrome.dev}} break these up on their own lines, or it's a bit unweildy to read. {{+partials.standard_apps_article article:intros.api_index stable_apis=... }} https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/extensions/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/extensions/api_index.html:1: {{+partials.standard_extensions_article article:intros.api_index stable_apis:api_list.extensions.chrome.stable beta_apis:api_list.extensions.chrome.beta dev_apis:api_list.extensions.chrome.dev}} likewise https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/extensions/experimental.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/extensions/experimental.html:1: {{+partials.standard_extensions_article article:intros.experimental is_apps:false stable_apis:api_list.extensions.experimental.stable beta_apis:api_list.extensions.experimental.beta dev_apis:api_list.extensions.experimental.dev}} just pass in the experimental APIs
add one new template, e.g. api_table https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:79: def GetAllNames(self): OK https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:88: return (api for api in api_features.itervalues() Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:98: 'dev': [],'trunk': [] }} This is a little big change, I guess it will influence some other pages. Could I use another CL to make this change? https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:104: if category == 'chrome' or category == 'experimental': Do you mean put "private apis" here, then put "chrome,experiment" in else clause? https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:109: api['description'] = model.Get().description For some private api, e.g. extension.sendNativeMessage. There is no model. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:114: channel = channel_info.channel Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:120: Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:126: continue I used this to check if there is some data in template language. According to handlebar,null is never considered a value. Is there someway to check the length of list in handlebar template? I did not find out it, so I just use null to stand for "false". https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:131: all_apis = sorted(all_apis, key=itemgetter('name')) Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:132: platform_dict['all_apis'] = all_apis Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:150: def GetAllNames(self): OK https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:11: Here are the supported chrome.* APIs stable since the Chrome version. Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:18: <tr><th>Name</th><th>Description</th><th>Chrome Version</th></tr> Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:31: Here are the supported chrome.* APIs available in Chrome Beta only. Done. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:34: <h3 id="beta_apis"> Yes, it should be. I worked well on my local machine. Do you see this sentence appear when beta table is not shown? https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:51: </p> Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:65: {{/}} Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/experimental.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/experimental.html:71: Done, use template api_table https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/private/samples.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/private/samples.html:31: {{?filter_list.dev}}Dev APIs:{{#filter_list.dev}}{{+partials.filter_item}}{{/}}{{/}} Done. Revert to old version. https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/apps/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/apps/api_index.html:1: {{+partials.standard_apps_article article:intros.api_index stable_apis:api_list.apps.chrome.stable beta_apis:api_list.apps.chrome.beta dev_apis:api_list.apps.chrome.dev}} Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/extensions/api_index.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/extensions/api_index.html:1: {{+partials.standard_extensions_article article:intros.api_index stable_apis:api_list.extensions.chrome.stable beta_apis:api_list.extensions.chrome.beta dev_apis:api_list.extensions.chrome.dev}} Done https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... File chrome/common/extensions/docs/templates/public/extensions/experimental.html (right): https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions... chrome/common/extensions/docs/templates/public/extensions/experimental.html:1: {{+partials.standard_extensions_article article:intros.experimental is_apps:false stable_apis:api_list.extensions.experimental.stable beta_apis:api_list.extensions.experimental.beta dev_apis:api_list.extensions.experimental.dev}} Done
Replying to your comments before I look at the CL again.
Incidentally, patching in your CL complains about whitespace errors:
<stdin>:537: trailing whitespace.
{{#filter_list.all_apis}}{{+partials.filter_item}}{{/}}<br>
<stdin>:548: trailing whitespace.
{{+partials.standard_apps_article
<stdin>:549: trailing whitespace.
article:intros.api_index
<stdin>:551: trailing whitespace.
stable_apis:api_list.apps.chrome.stable
<stdin>:552: trailing whitespace.
beta_apis:api_list.apps.chrome.beta
warning: squelched 5 whitespace errors
warning: 10 lines add whitespace errors.
https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):
https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:98: 'dev':
[],'trunk': [] }}
On 2013/11/04 11:07:26, hukun wrote:
> This is a little big change, I guess it will influence some other pages. Could
I
> use another CL to make this change?
Sure.
https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:104: if category
== 'chrome' or category == 'experimental':
On 2013/11/04 11:07:26, hukun wrote:
> Do you mean put "private apis" here, then put "chrome,experiment" in else
> clause?
Yes
https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:109:
api['description'] = model.Get().description
On 2013/11/04 11:07:26, hukun wrote:
> For some private api, e.g. extension.sendNativeMessage. There is no model.
Ah right. Yes, I just fixed a bug for that in APIModels. You should be able to
list models with APIModels.GetNames() which will filter out things like
"extension.sendNativeMethod"; and then GetModel should never return None.
https://codereview.chromium.org/48263002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:126: continue
On 2013/11/04 11:07:26, hukun wrote:
> I used this to check if there is some data in template language. According to
> handlebar,null is never considered a value. Is there someway to check the
length
> of list in handlebar template? I did not find out it, so I just use null to
> stand for "false".
{{?empty_list}} should be false (same as None/False).
See here:
https://github.com/kalman/templates/blob/master/python/handlebar.py#L571
The documentation itself may be a little out of date; the source code is a
better reference (even better is the source code that's actually checked into
Chromium; the version there is a little newer).
Those changes have been submitted now - you should rebase. The only real work left here is the tests. It looks great. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:58: return self._cache.GetFromFileListing(self._public_template_path).Get() there is an extra space after the "return" here. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:97: return model.Get().description likewise https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:103: 'dev': [],'trunk': [] }} I would prefer this be formatted like: platform_dict = { 'chrome': {'stable': [], 'beta': [], 'dev': [], 'trunk': [] } https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:116: api['version'] = GetChannelInfo(api_name).version hold onto a reference to GetChannelInfo(api_name) rather than GetChannelInfo(api_name).channel here. channel_info = GetChannelInfo(api_name) if channel_info.channel == 'stable': api['version'] = channel_info.version platform_dict[category][channel_info.channel].append(api) https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:119: api['description'] = GetApiDescription(api_name) you might as well just always assign api['description'] (i.e. right after "for api in FilterAPIs(platform)"). Having the redundant description for private APIs is fine, and it's simpler that way. In fact, we should be updating private_apis.html in this change as well, which would require setting the description every iteration. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:125: for category, apis_by_channel in platform_dict.iteritems(): the only key in platform_dict at this point is 'chrome'. also, as I pointed out, empty-list is falsifiable in handlebar. so, you should be able to write this loop something like for chrome_apis_by_channel in platform_dict['chrome'].itervalues(): sort(chrome_apis_by_channel, key=itemgetter('name')) utils.MarkLast(chrome_apis_by_channel) https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:135: 'all_apis': sorted(all_apis, key=itemgetter('name')), just 'all'. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:143: utils.MarkLast(platform_dict['experimental']) And come to think of it, you should be able to write this list something like: for key, apis in (('all', all_apis), ('private', private_apis), ('experimental', experimental_apis)): sort(apis, key=itemgetter('name')) utils.MarkLast(apis) platform_dict[key] = apis https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right): https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:115: FakeAvailabilityFinder()) Ok, so prompted by this I've rewritten this test a bit to make it clearer what's going on. Basically you need to set up the _TEST_DATA such that correct schema are returned; see the way that api_models_test works. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:124: 'description': u'<code>alarms</code>' Modify the test data (_api_features.json file) so that you also get some APIs in dev, beta, and trunk. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:10: <h3 id="stable_apis"> Let's kill the "Supported APIs" header and make each of the "Stable", "Beta", and "Dev" APIs a <h2> rather than a <h3>
@Ben: How can I see these warning and error messages? I did not see them
when I run "git cl presubmit"?
*<stdin>:537: trailing whitespace.
{{#filter_list.all_apis}}{{+partials.filter_item}}{{/}}<br>
<stdin>:548: trailing whitespace.
{{+partials.standard_apps_article
<stdin>:549: trailing whitespace.
article:intros.api_index
<stdin>:551: trailing whitespace.
stable_apis:api_list.apps.chrome.stable
<stdin>:552: trailing whitespace.
beta_apis:api_list.apps.chrome.beta
warning: squelched 5 whitespace errors
warning: 10 lines add whitespace errors.*
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2013/11/05 02:31:52, hukun_google.com wrote: > @Ben: How can I see these warning and error messages? I did not see them > when I run "git cl presubmit"? > > > *<stdin>:537: trailing whitespace. > {{#filter_list.all_apis}}{{+partials.filter_item}}{{/}}<br> > > <stdin>:548: trailing whitespace. > {{+partials.standard_apps_article > <stdin>:549: trailing whitespace. > article:intros.api_index > <stdin>:551: trailing whitespace. > stable_apis:api_list.apps.chrome.stable > <stdin>:552: trailing whitespace. > beta_apis:api_list.apps.chrome.beta > warning: squelched 5 whitespace errors > warning: 10 lines add whitespace errors.* > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I don't know :( I only see it when I patch it in. It's probably not a big deal. But if it's trailing whitespace, you could grep the templates for ' $'. Note that I think some of the trailing whitespace may actually be deliberate, somewhere.
https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:58: return self._cache.GetFromFileListing(self._public_template_path).Get() Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:97: return model.Get().description Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:103: 'dev': [],'trunk': [] }} Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:116: api['version'] = GetChannelInfo(api_name).version Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:119: api['description'] = GetApiDescription(api_name) E.g. musicManagerPrivate will be None returned by self._api_models.GetModel(api_name).Get(). https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:125: for category, apis_by_channel in platform_dict.iteritems(): Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:135: 'all_apis': sorted(all_apis, key=itemgetter('name')), Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:143: utils.MarkLast(platform_dict['experimental']) Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right): https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:115: FakeAvailabilityFinder()) OK, have merged. https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:124: 'description': u'<code>alarms</code>' Done https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/api_index.html (right): https://codereview.chromium.org/48263002/diff/290001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/api_index.html:10: <h3 id="stable_apis"> Done
On Tue, Nov 5, 2013 at 2:30 AM, <kalman@chromium.org> wrote: > Replying to your comments before I look at the CL again. > > Incidentally, patching in your CL complains about whitespace errors: > > <stdin>:537: trailing whitespace. > {{#filter_list.all_apis}}{{+ > partials.filter_item}}{{/}}<br> > > <stdin>:548: trailing whitespace. > {{+partials.standard_apps_article > <stdin>:549: trailing whitespace. > article:intros.api_index > <stdin>:551: trailing whitespace. > stable_apis:api_list.apps.chrome.stable > <stdin>:552: trailing whitespace. > beta_apis:api_list.apps.chrome.beta > warning: squelched 5 whitespace errors > warning: 10 lines add whitespace errors. > > Found it in the rebase, fixed it. > > > > https://codereview.chromium.org/48263002/diff/110001/ > chrome/common/extensions/docs/server2/api_list_data_source.py > File chrome/common/extensions/docs/server2/api_list_data_source.py > (right): > > https://codereview.chromium.org/48263002/diff/110001/ > chrome/common/extensions/docs/server2/api_list_data_source.py#newcode98 > chrome/common/extensions/docs/server2/api_list_data_source.py:98: 'dev': > [],'trunk': [] }} > On 2013/11/04 11:07:26, hukun wrote: > >> This is a little big change, I guess it will influence some other >> > pages. Could I > >> use another CL to make this change? >> > > Sure. > > OK, got it. > > https://codereview.chromium.org/48263002/diff/110001/ > chrome/common/extensions/docs/server2/api_list_data_source.py#newcode104 > chrome/common/extensions/docs/server2/api_list_data_source.py:104: if > category == 'chrome' or category == 'experimental': > On 2013/11/04 11:07:26, hukun wrote: > >> Do you mean put "private apis" here, then put "chrome,experiment" in >> > else > >> clause? >> > > Yes > > > > https://codereview.chromium.org/48263002/diff/110001/ > chrome/common/extensions/docs/server2/api_list_data_source.py#newcode109 > chrome/common/extensions/docs/server2/api_list_data_source.py:109: > api['description'] = model.Get().description > On 2013/11/04 11:07:26, hukun wrote: > >> For some private api, e.g. extension.sendNativeMessage. There is no >> > model. > > Ah right. Yes, I just fixed a bug for that in APIModels. You should be > able to list models with APIModels.GetNames() which will filter out > things like "extension.sendNativeMethod"; and then GetModel should never > return None. > > Done. > > https://codereview.chromium.org/48263002/diff/110001/ > chrome/common/extensions/docs/server2/api_list_data_source.py#newcode126 > chrome/common/extensions/docs/server2/api_list_data_source.py:126: > continue > On 2013/11/04 11:07:26, hukun wrote: > >> I used this to check if there is some data in template language. >> > According to > >> handlebar,null is never considered a value. Is there someway to check >> > the length > >> of list in handlebar template? I did not find out it, so I just use >> > null to > >> stand for "false". >> > > {{?empty_list}} should be false (same as None/False). > > Done. > See here: > https://github.com/kalman/templates/blob/master/python/handlebar.py#L571 > > The documentation itself may be a little out of date; the source code is > a better reference (even better is the source code that's actually > checked into Chromium; the version there is a little newer). > > https://codereview.chromium.org/48263002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry to keep on doing rounds here, I really don't want to discourage you, since the patch looks great. It's a pity about the time difference, or this would iterate faster I think - speaking of which, I won't take so long getting back to you from now on! I was sheriff the last 2 days and found it hard to focus on much else. https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:95: return '' you can also write this more concisely: model = self._api_models.GetModel(api_name).Get() return model.description if model else '' *however* I do see the problem here now. This code should be using self._api_models.GetNames(), not the features bundle at all. That would really simplify this. So in the loop below, instead of having for api in FilterAPIs(platform): ... complicated logic you would have api_features = self._features_bundle.GetAPIFeatures() for api_name in self._api_models.GetName(): if platform not in api_features[api_name]['platforms']: continue api_model = self._api_models.GetModel(api_name) ... api_model.description and so on https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:101: 'chrome': {'stable': [],'beta': [], 'dev': [],'trunk': []} some of these commas need spaces after them https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:45: _TEST_API_Data = { DATA not Data https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:65: } each of these constants need 2 newlines between them; and the closing braces need to be unindented by 2 spaces https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:71: '_permission_features.json': '{}', include the API schema in here. you could write a utility which reads in the real API schema from a file, then modifies its 'description' field to something predictable? like def _ReadSchemaWithDescription(filename, description): json_dict = Parse(ReadFile(filename)) json_dict['description'] = description return json.dumps(json_dict) 'api': { 'alarms.idl': _ReadSchemaWithDescription('alarms.idl', 'alarms API') 'app_window.idl': _ReadSchemaWithDescription(...) } https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:138: FakeAPIModels(), can you use server_instance.api_models here? https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:139: FakeAvailabilityFinder()) and server_instance.availability_finder here? you will probably get version 5 for the stable channels, but that's fine. the important part is getting the dev stuff etc. https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:170: }], 'beta': [], 'trunk': []}, api_list.get('apps').get('chrome')) nice to still ahve the 'beta' and 'trunk' lists on their own line, likewise the api_list.get(..) part. https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_models.py (left): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_models.py:24: Ppython style is still is actually to have 2 blank lines between top-level classes/functions, not 1. I didn't realise this for a while which is why a lot of existing code doesn't follow it. https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_models.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_models.py:22: if not schema: return None When does this happen? If the schema doesn't parse or something? Is there any chance we can not make this change? I don't like guarding against buggy behaviour elsewhere in the system.
https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:95: return '' I did filter the api by _api_models.GetName() before getting description. You could see the codes at line 110. But, this functioin "_CreateAPIModel" in api_models.py will get schema as "None", so that I add one line "if not schema: return None" in api_models.py. One example is "musicManagerPrivate". 1) ProcessSchema will return None, so scheme is None. But, use schema['namespace'] will throw exceptio due to None ref 2) Inside ProcessSchema, RemoveNoDocs is called. By design, RemoveNoDocs will return true due to "[nodoc]" element. So, I am not sure if it is by design or buggy. I would like to help fix this issue if need. Listen to your opinion to go ahead. https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:101: 'chrome': {'stable': [],'beta': [], 'dev': [],'trunk': []} Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:45: _TEST_API_Data = { Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:65: } Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:71: '_permission_features.json': '{}', Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:138: FakeAPIModels(), Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:139: FakeAvailabilityFinder()) Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:170: }], 'beta': [], 'trunk': []}, api_list.get('apps').get('chrome')) Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_models.py (left): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_models.py:24: Done https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_models.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_models.py:22: if not schema: return None One example is "musicManagerPrivate". 1) ProcessSchema will return None, so scheme is None. But, use schema['namespace'] will throw exceptio due to None ref 2) Inside ProcessSchema, RemoveNoDocs is called. By design, RemoveNoDocs will return true due to "[nodoc]" element. So, I am not sure if it is by design or buggy. I would like to help fix this issue if need. Listen to your opinion to go ahead.
lgtm. The remaining comments should be pretty straightforward, I think. https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:95: return '' On 2013/11/08 06:23:39, hukun wrote: > I did filter the api by _api_models.GetName() before getting description. You > could see the codes at line 110. But, this functioin "_CreateAPIModel" in > api_models.py will get schema as "None", so that I add one line "if not schema: > return None" in api_models.py. > > One example is "musicManagerPrivate". > > 1) ProcessSchema will return None, so scheme is None. But, use > schema['namespace'] will throw exceptio due to None ref > > 2) Inside ProcessSchema, RemoveNoDocs is called. By design, RemoveNoDocs will > return true due to "[nodoc]" element. > > So, I am not sure if it is by design or buggy. I would like to help fix this > issue if need. > > Listen to your opinion to go ahead. Oh because they have [nodoc]. Right. That is a problem that I'd like to fix, but, we can do it later. Nice catch, thanks. Could you add a comment and reference crbug.com/317197 here? https://codereview.chromium.org/48263002/diff/550001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:107: for api in FilterAPIs(platform): See comment above. Just loop over self._api_models.GetNames(), this will be simpler. You shouldn't need FilterAPIs etc. https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:12: from branch_utility import ChannelInfo sorting: branch_utility should go above svn_constants https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:14: def _GetAPICategory(api, documented_apis): (for comment below, change this to take api_name directly rather than api) https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:105: if api_name in self._api_models.GetNames(): Instead of the above 4 lines, you can have for api_name in self._api_models.GetNames(): category = GetAPICategory(api_name, documented_apis[platform]) then this all becomes quite a bit cleaner. https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right): https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:73: 'namespace': 'sockets.udp'}] I think you could do something similar to _ToTestFeatures here, like _ToTestAPIs which takes a list of API names and generates those APIs for them (the description + namespace). https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:86: json.dumps(_TEST_API_DATA['experimental.bluetooth']), _ToTestAPIs can also do the json.dumps for each API schema to save you from doing it here. https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_models.py (right): https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_models.py:22: if not schema: return None Ok. Please reference crbug.com/317197 here.
https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:12: from branch_utility import ChannelInfo Done https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:14: def _GetAPICategory(api, documented_apis): Done https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:105: if api_name in self._api_models.GetNames(): Need some other properties, e.g. platform, dependencies. I am not sure if these are necessary, but unittest needs it. So, I kept them. https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right): https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:73: 'namespace': 'sockets.udp'}] Done https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source_test.py:86: json.dumps(_TEST_API_DATA['experimental.bluetooth']), Done https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_models.py (right): https://codereview.chromium.org/48263002/diff/690001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_models.py:22: if not schema: return None Done
I don't know where the .gitmodules error is coming from since it doesn't seem like your patch is modifying .gitmodules. It shouldn't. Make sure it hasn't changed; perhaps actually revert the changes to it anyway? If that doesn't work then I'd try syncing to head including gclient sync, though I've never had to do anything other than just "git pull" to work on the docserver (the docserver doesn't pull in any gclient dependencies). https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:104: api['description'] = GetApiDescription(api_name) This and below is modifying the live data returned by GetAPIFeatures(), which is bad; a side-effect of calling this method. You should construct a new object to pass to the model with just the specific data that you need. api_data = { 'name': api['name'], ... } ... all_apis.append(api_data) Sorry for not noticing this earlier. It's also why I didn't notice that you were using the platform etc data straight from the feature object. Still, please loop over the result from api_list.GetNames() not the features. It's idiomatically correct. If you need feature data like the platform then grab the feature separately: for api_name in self._api_models.GetNames(): api_model = self._api_models.GetModel(api_name) api_feature = self._features_bundle.GetAPIFeatures()[api_name] ... You might actually want to add an APIModels.IterModels() which yields (name, model) pairs so that you can write for api_name, api_model in self._api_models.IterModels(): ...
> You might actually want to add an APIModels.IterModels() which yields (name, > model) pairs so that you can write > > for api_name, api_model in self._api_models.IterModels(): > ... Actually that's silly; the api model already has the name on it (I think?). So IterModels() can just return the model not tuples.
https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:104: api['description'] = GetApiDescription(api_name) Got it. If you do not mind, I fix it in next CL.
https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:104: api['description'] = GetApiDescription(api_name) On 2013/11/13 15:36:44, hukun wrote: > Got it. If you do not mind, I fix it in next CL. Please fix it now, it's a pretty small change. It's one thing to follow up with a fix for unrelated code (that's fine) but don't submit code that you're just going to change again right away.
OK. I am struggling with dcommit, and do not submit code now. Let me fix at first. On Wed, Nov 13, 2013 at 11:49 PM, <kalman@chromium.org> wrote: > > https://codereview.chromium.org/48263002/diff/840001/ > chrome/common/extensions/docs/server2/api_list_data_source.py > File chrome/common/extensions/docs/server2/api_list_data_source.py > (right): > > https://codereview.chromium.org/48263002/diff/840001/ > chrome/common/extensions/docs/server2/api_list_data_source.py#newcode104 > chrome/common/extensions/docs/server2/api_list_data_source.py:104: > api['description'] = GetApiDescription(api_name) > On 2013/11/13 15:36:44, hukun wrote: > >> Got it. If you do not mind, I fix it in next CL. >> > > Please fix it now, it's a pretty small change. It's one thing to follow > up with a fix for unrelated code (that's fine) but don't submit code > that you're just going to change again right away. > > https://codereview.chromium.org/48263002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I don't think non-committers can dcommit, so it could be that. Also it looks like that last patchset that you uploaded didn't quite finish, and codereview often has a bug where it means any subsequent patches you upload also don't finish - you'll probably need to delete that last patchset and re-upload it.
@Ben: I have a little confusion. What is meaning about "I don't think non-committers can dcommit"? Does it mean I could not commit the codes by myself? By the way, I am going to fix that bug, and upload the patch again. On Thu, Nov 14, 2013 at 12:16 AM, <kalman@chromium.org> wrote: > I don't think non-committers can dcommit, so it could be that. > > Also it looks like that last patchset that you uploaded didn't quite > finish, and > codereview often has a bug where it means any subsequent patches you > upload also > don't finish - you'll probably need to delete that last patchset and > re-upload > it. > > https://codereview.chromium.org/48263002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/840001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:104: api['description'] = GetApiDescription(api_name) Done. Fix it by using api_models.
lgtm https://codereview.chromium.org/48263002/diff/980001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/980001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:101: api['platforms'] = GetApiPlatform(api_name) you can write this slightly nicer like api = { 'name': api_name, 'description': GetApiDescription(api_model), 'platforms': GetApiPlatform(api_model), }
https://codereview.chromium.org/48263002/diff/980001/chrome/common/extensions... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://codereview.chromium.org/48263002/diff/980001/chrome/common/extensions... chrome/common/extensions/docs/server2/api_list_data_source.py:101: api['platforms'] = GetApiPlatform(api_name) Done
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hukun@chromium.org/48263002/1190001
Message was sent while issue was closed.
Change committed as 235124 |
