Dynamically generate a heading for Extension Docs API pages

Dynamically generate a heading for Extension Docs API pages

UPDATE: This has been committed in several different pieces.
Extension Docs API pages now have a dynamically generated heading. Closing this [massive] issue. It's been fun.

This replaces the currently hardcoded HTML heading.
This patch finds the API's description and the earliest version of chrome in which is was available, and uses json information to temporarily find the permissions data.

BUG=163418, 233968

[cduvall, 2013-03-21 18:43:53]

First round!

Also, make the description of the issue a little more concise.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/tts.json (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/tts.json:9: "availability": "14",
Why are some of these "Google Chrome X", and some are "X"?

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:7: import os
Add the newline back in after this (separates python stdlib includes from your
files)

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:88: #availability found
using filesystem
Space after # in all comments.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:92: availability = None
No need to set to None, we already know it is

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:93:
availability_generated = None
Should be False not None

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:97:
availability_generated = None
Just set availability and availability_generated at the beginning of this
function:

availability = self._namespace.availability
availability_generated = False
if availability is None:
  ...

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:506: def tryGet(self,
key, tryNonUnix=False):
parameter names are unix_hacker_style
method names should be CamelCase with first letter capital.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_list_data_source.py (left):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:5: import logging
NICE

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:30: version =
int(self._version_number)
dont reuse version as an int. Call it something else

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:31: while
version >= 18: #extension api's are not present before version 18
Put comment above while

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:33: "%d" %
version)
single quotes

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:36: version -=
1
instead of doing version -= 1 in both places, use a finally block

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source_test.py:10:
import json
separate python stdlib includes from your own

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source_test.py:66: def
_LoadJsonAPI(self, api, disable_refs):
Move these out of the class

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/branch_utility.py:46: return {'name' :
channel_name,
format like:
return {
  'name': channel_name,
  ...
}

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/branch_utility.py:59: data =
self._object_store.Get(channel_name + '.' +
Use '%s.%s.%s'

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/chrome_version_data_source.py
(right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:30:
logging.error("Could not fetch data at Omaha Proxy.\n%s" % (e))
better return None here

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system.py:31: DEFAULT_BRANCH
           = '000'
Separate this from the namespace constants.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system.py:72:
self._branch_number = branch_number
Doesn't the file system already have a branch number that it fetches from? I
think this having a branch number is unnecessary.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system.py:115:
logging.error("%s" % e)
Give a better message than this. And single quotes.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/example_zipper_test.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/example_zipper_test.py:24:
CompiledFileSystem.Factory(self._file_system,
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:66: GITHUB_COMPILED_FILE_SYSTEM
= CompiledFileSystem.Factory(
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:81: 
Remove newline

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:107:
APPS_MEMCACHE).Create(_SplitFilenameUnix,
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:115:
EXTENSIONS_MEMCACHE).Create(_SplitFilenameUnix,
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:126: branchAndVersion =
BRANCH_UTILITY.GetBranchAndVersionForChannelName(
use unix_hacker style names.

but this can just be branch, version = BRANCH_UTILITY...

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:141: cache_factory =
CompiledFileSystem.Factory(
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:233:
CHROME_VERSION_DATA_SOURCE.SetCreateMemcacheForBranch(
Can you just pass this into the ChromeVersionDataSource constructor?

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:337: factory =
CompiledFileSystem.Factory(file_system,
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/server_instance.py (left):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/server_instance.py:46: 
Give back my newline!

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:20: 'channels' :
[{'name': channel['name'].upper(),
reformat to:

'channels' : [
  {
    'name': ...
  } for blah...
]

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:44: { 'name':
'a', 'branch': '11', 'version': '1' },
reformat to:
self._fake_channel_data = [
  { ... },
  { ... }
]

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:87:
cache_factory = CompiledFileSystem.Factory(
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:104:
cache_factory = CompiledFileSystem.Factory(
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:117:
cache_factory = CompiledFileSystem.Factory(
revert

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/articles/whats_new.html (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/whats_new.html:496: <!--  
<li>The $ref:cookies.onChanged
Fix this, and uncomment.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/private/intro_table.html (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:1: <table
class="intro">
You need to delete this table from intros that already have it (e.g. alarms).

[cduvall, 2013-03-21 18:45:19]

Also, for some reason the generated availability for alarms doesn't match the
one in the intro...

[cduvall, 2013-03-21 18:47:43]

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/private/intro_table.html (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:1: <table
class="intro">
On 2013/03/21 18:43:53, cduvall wrote:
> You need to delete this table from intros that already have it (e.g. alarms).

But only delete the description and availability parts.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:36:
<strong>Permissions:</strong>
Take out permissions, we don't generate this yet.

[epeterson, 2013-03-25 19:35:11]

This should cover most of the requests you've asked for except for fixing the
intro table html.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/page_capture.json (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/page_capture.json:8: "description": "The
pagecapture API allows you to save a tab as MHTML.",
Should be pageCapture --done

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/tts.json (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/tts.json:9: "availability": "14",
Should be Google Chrome 14

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/tts.json:9: "availability": "14",
On 2013/03/21 18:43:53, cduvall wrote:
> Why are some of these "Google Chrome X", and some are "X"?

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/webstore.json:9: "availability": "15",
Should be Google Chrome 15. --done

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:7: import os
On 2013/03/21 18:43:53, cduvall wrote:
> Add the newline back in after this (separates python stdlib includes from your
> files)

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:88: #availability found
using filesystem
On 2013/03/21 18:43:53, cduvall wrote:
> Space after # in all comments.

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:92: availability = None
On 2013/03/21 18:43:53, cduvall wrote:
> No need to set to None, we already know it is

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:93:
availability_generated = None
On 2013/03/21 18:43:53, cduvall wrote:
> Should be False not None

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:97:
availability_generated = None
On 2013/03/21 18:43:53, cduvall wrote:
> Just set availability and availability_generated at the beginning of this
> function:
> 
> availability = self._namespace.availability
> availability_generated = False
> if availability is None:
>   ...

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:506: def tryGet(self,
key, tryNonUnix=False):
On 2013/03/21 18:43:53, cduvall wrote:
> parameter names are unix_hacker_style
> method names should be CamelCase with first letter capital.

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_list_data_source.py (left):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:5: import logging
On 2013/03/21 18:43:53, cduvall wrote:
> NICE

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:30: version =
int(self._version_number)
On 2013/03/21 18:43:53, cduvall wrote:
> dont reuse version as an int. Call it something else

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:31: while
version >= 18: #extension api's are not present before version 18
On 2013/03/21 18:43:53, cduvall wrote:
> Put comment above while

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:33: "%d" %
version)
On 2013/03/21 18:43:53, cduvall wrote:
> single quotes

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source.py:36: version -=
1
On 2013/03/21 18:43:53, cduvall wrote:
> instead of doing version -= 1 in both places, use a finally block

I didn't want that piece of code to always execute (the version shouldn't be
decremented if the file name isn't found in either case); just putting the line
at the end of the loop was the solution.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source_test.py:10:
import json
On 2013/03/21 18:43:53, cduvall wrote:
> separate python stdlib includes from your own

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_data_source_test.py:66: def
_LoadJsonAPI(self, api, disable_refs):
On 2013/03/21 18:43:53, cduvall wrote:
> Move these out of the class

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/branch_utility.py:46: return {'name' :
channel_name,
On 2013/03/21 18:43:53, cduvall wrote:
> format like:
> return {
>   'name': channel_name,
>   ...
> }

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/branch_utility.py:59: data =
self._object_store.Get(channel_name + '.' +
On 2013/03/21 18:43:53, cduvall wrote:
> Use '%s.%s.%s'

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/branch_utility.py:76: return ''
return '0' -- done

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/chrome_version_data_source.py
(right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:30:
logging.error("Could not fetch data at Omaha Proxy.\n%s" % (e))
On 2013/03/21 18:43:53, cduvall wrote:
> better return None here

Done? Is there a better way to handle this case so that it doesn't result in bad
things happening?

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system.py:31: DEFAULT_BRANCH
           = '000'
On 2013/03/21 18:43:53, cduvall wrote:
> Separate this from the namespace constants.

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system.py:72:
self._branch_number = branch_number
On 2013/03/21 18:43:53, cduvall wrote:
> Doesn't the file system already have a branch number that it fetches from? I
> think this having a branch number is unnecessary.

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system.py:115:
logging.error("%s" % e)
On 2013/03/21 18:43:53, cduvall wrote:
> Give a better message than this. And single quotes.

Done? Does this work for you?

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/example_zipper_test.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/example_zipper_test.py:24:
CompiledFileSystem.Factory(self._file_system,
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:66: GITHUB_COMPILED_FILE_SYSTEM
= CompiledFileSystem.Factory(
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:81: 
On 2013/03/21 18:43:53, cduvall wrote:
> Remove newline

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:107:
APPS_MEMCACHE).Create(_SplitFilenameUnix,
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:115:
EXTENSIONS_MEMCACHE).Create(_SplitFilenameUnix,
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:126: branchAndVersion =
BRANCH_UTILITY.GetBranchAndVersionForChannelName(
On 2013/03/21 18:43:53, cduvall wrote:
> use unix_hacker style names.
> 
> but this can just be branch, version = BRANCH_UTILITY...

I reworked this. branchAndVersion actually contains the name of the channel it
was generated for, also. The name has been changed to reflect this.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:141: cache_factory =
CompiledFileSystem.Factory(
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:233:
CHROME_VERSION_DATA_SOURCE.SetCreateMemcacheForBranch(
On 2013/03/21 18:43:53, cduvall wrote:
> Can you just pass this into the ChromeVersionDataSource constructor?

Done. Yep.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handler.py:337: factory =
CompiledFileSystem.Factory(file_system,
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/server_instance.py (left):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/server_instance.py:46: 
On 2013/03/21 18:43:53, cduvall wrote:
> Give back my newline!

Done. Sorry!

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:20: 'channels' :
[{'name': channel['name'].upper(),
On 2013/03/21 18:43:53, cduvall wrote:
> reformat to:
> 
> 'channels' : [
>   {
>     'name': ...
>   } for blah...
> ]

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:44: { 'name':
'a', 'branch': '11', 'version': '1' },
On 2013/03/21 18:43:53, cduvall wrote:
> reformat to:
> self._fake_channel_data = [
>   { ... },
>   { ... }
> ]

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:87:
cache_factory = CompiledFileSystem.Factory(
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:104:
cache_factory = CompiledFileSystem.Factory(
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:117:
cache_factory = CompiledFileSystem.Factory(
On 2013/03/21 18:43:53, cduvall wrote:
> revert

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/articles/whats_new.html (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/whats_new.html:496: <!--  
<li>The $ref:cookies.onChanged
On 2013/03/21 18:43:53, cduvall wrote:
> Fix this, and uncomment.

Done.

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/private/intro_table.html (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:36:
<strong>Permissions:</strong>
On 2013/03/21 18:47:43, cduvall wrote:
> Take out permissions, we don't generate this yet.

Done.

[epeterson, 2013-03-25 19:48:02]

Added some notes in handler.py

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/12996003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/browser_action.json:9: "dependencies": [ "tabs" ],
This is from a patch that caused conflicts; someone removed dependencies from
some of the json files. I went along with it.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:93: def
_CreateMemcacheForBranch(branch_number):
  This is now a global method, which is passed to the global
chrome_version_data_source. 
  This method can't really be used for both _GetInstanceForBranch and in
chrome_version_data_source, as the former requires more objects that depend on
the ones generated here, and the latter doesn't need access to those extra
objects at all.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:114: 'trunk',
  This is really the only part that prevents this method from standing alone
from the _GetInstanceForbranch method.
  Previously, the channel_name was grabbed from the branch instance, but the
samples_data_source isn't really an integral part of finding availability.
  So I stuck 'trunk' in there.

[cduvall, 2013-03-25 22:59:43]

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/api/commands.json (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/commands.json:9: "availability": "Google Chrome
25",
Like I said on IRC, I would make this just "availability": 25

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:91: # availability
found using filesystem
Make this a complete sentence with capital first letter

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:94: # we don't know
anything about this api's availability
same

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:95:
availability_generated = False
no need to set this again, its already False

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:516: #Some api's in
older versions trip up the JSC (i.e. v.18 tabs.json lacks
Space after #

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:8: class
AvailabilityDataSource:
Class level comment about what this does.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:31: #
extension api's are not present before version 18
Full sentence with proper punctuation for all comments

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source_test.py:25: '''
These will prevent an api_data_source from actually loading an api,
Use # comments here

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/branch_utility.py:99:
self._object_store.Set(channel_name + '.' + self._base_path +
Use %s style string

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/chrome_version_data_source.py
(right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:14:
self._CreateMemcacheForBranch = CreateMemcacheForBranch
Name unix_hacker_style

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:31:
logging.error("Could not fetch data at Omaha Proxy.\n%s" % (e))
do '...Proxy: %s'

also, single quotes

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/compiled_file_system.py:107: except
ValueError:
When will this ValueError happen?

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/compiled_file_system.py:109:
logging.error('Caught a ValueError while trying to get the contents of',
This logging will never happen because its after the return

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:93: def
_CreateMemcacheForBranch(branch_number):
On 2013/03/25 19:48:02, epeterson wrote:
>   This is now a global method, which is passed to the global
> chrome_version_data_source. 
>   This method can't really be used for both _GetInstanceForBranch and in
> chrome_version_data_source, as the former requires more objects that depend on
> the ones generated here, and the latter doesn't need access to those extra
> objects at all.

Which objects does it depend on? Can you just pass them in as extra parameters
to this function?

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:114: 'trunk',
On 2013/03/25 19:48:02, epeterson wrote:
>   This is really the only part that prevents this method from standing alone
> from the _GetInstanceForbranch method.
>   Previously, the channel_name was grabbed from the branch instance, but the
> samples_data_source isn't really an integral part of finding availability.
>   So I stuck 'trunk' in there. 

Can you just pass the channel name in as a function parameter?

Also you can do something like:
lambda x: _CreateMemcacheForBranch(x, arg1, arg2,...)

if you don't want to have to supply all the extra args in
ChromeVersionDataSource

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:205: JSON_PATH)
revert

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:86: #branch_info =
self._branch_info.copy()
take out?

[epeterson, 2013-03-27 22:36:09]

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/api/commands.json (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/commands.json:9: "availability": "Google Chrome
25",
On 2013/03/25 22:59:43, cduvall wrote:
> Like I said on IRC, I would make this just "availability": 25

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:91: # availability
found using filesystem
On 2013/03/25 22:59:43, cduvall wrote:
> Make this a complete sentence with capital first letter

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:94: # we don't know
anything about this api's availability
On 2013/03/25 22:59:43, cduvall wrote:
> same

I just got rid of this one,

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:95:
availability_generated = False
On 2013/03/25 22:59:43, cduvall wrote:
> no need to set this again, its already False

and this one.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:516: #Some api's in
older versions trip up the JSC (i.e. v.18 tabs.json lacks
On 2013/03/25 22:59:43, cduvall wrote:
> Space after #

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:8: class
AvailabilityDataSource:
On 2013/03/25 22:59:43, cduvall wrote:
> Class level comment about what this does.

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:31: #
extension api's are not present before version 18
On 2013/03/25 22:59:43, cduvall wrote:
> Full sentence with proper punctuation for all comments

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source_test.py:25: '''
These will prevent an api_data_source from actually loading an api,
On 2013/03/25 22:59:43, cduvall wrote:
> Use # comments here

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/branch_utility.py:99:
self._object_store.Set(channel_name + '.' + self._base_path +
On 2013/03/25 22:59:43, cduvall wrote:
> Use %s style string

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/chrome_version_data_source.py
(right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:14:
self._CreateMemcacheForBranch = CreateMemcacheForBranch
On 2013/03/25 22:59:43, cduvall wrote:
> Name unix_hacker_style

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:31:
logging.error("Could not fetch data at Omaha Proxy.\n%s" % (e))
On 2013/03/25 22:59:43, cduvall wrote:
> do '...Proxy: %s'
> 
> also, single quotes

Done.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/compiled_file_system.py:107: except
ValueError:
On 2013/03/25 22:59:43, cduvall wrote:
> When will this ValueError happen?

This ValueError is raised in ApiListDataSource's .get() method, which calls
CompiledFileSystem's GetFromFileListing method, when GetFromFileListing fails to
get data for a file.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/compiled_file_system.py:109:
logging.error('Caught a ValueError while trying to get the contents of',
On 2013/03/25 22:59:43, cduvall wrote:
> This logging will never happen because its after the return

Done. Oops..thought I fixed that.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:205: JSON_PATH)
On 2013/03/25 22:59:43, cduvall wrote:
> revert

Done, in the new global method.

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://codereview.chromium.org/12996003/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:86: #branch_info =
self._branch_info.copy()
On 2013/03/25 22:59:43, cduvall wrote:
> take out?

Done.

[cduvall, 2013-03-27 23:01:10]

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:83: 
delete extra newline

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:100: that this api was
available in.
line this up with the """:
"""...
that this..

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:108: the availability
depending on its original format.
same

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:513: def tryGet(self,
key, try_non_unix=False):
Rename to TryGet

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:527:
logging.error("Caught Parse Exception")
Make this error say something like:

'Caught parse exception for %s: %s' % (unix_name, e)

Also, single quotes.

Also, make it a logging.warn

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source_test.py:52: self.maxDiff =
None
take out

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:23: '%s' %
version)
just version? or str(version)?

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:47: if
self._CheckExistence(api_name, 'trunk') is False:
if not self._CheckExistence(api_name, 'trunk'):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:49: return
None
Cache this so we don't try to keep looking.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:59: if
self._CheckExistence(api_name, '%s' % version_int) is False:
if not ...

str(version_int)

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:65: # the api
was not found in the current version.
capitalize API

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:70:
availability = '%d' % version_int
str(version_int)

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:72:
availability = '%d' % version_int
str(version_int)

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/chrome_version_data_source.py
(right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:6: import
logging
nit: newline between logging and object_store

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:66: for
entry in version_json['events']:
why 'events' here?

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/compiled_file_system.py:108:
logging.error('Could not get data for the file at %s' % path)
..file at %s: %s' % (path, e)

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:92: def
_CreateObjectsForBranch(branch_number, channel_info=None):
I think this would be more clear if you had two more helpers:

_CreateAPIDataSourceForBranch(branch_number):
  return _CreateObjectsForBranch(branch_number, None)

_CreateInstanceForBranch(branch_number, channel_info)
  return _CreateObjectsForBranch(branch_number, channel_info)

[epeterson, 2013-03-28 00:53:50]

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:83: 
On 2013/03/27 23:01:10, cduvall wrote:
> delete extra newline

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:100: that this api was
available in.
On 2013/03/27 23:01:10, cduvall wrote:
> line this up with the """:
> """...
> that this..

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:108: the availability
depending on its original format.
On 2013/03/27 23:01:10, cduvall wrote:
> same

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:513: def tryGet(self,
key, try_non_unix=False):
On 2013/03/27 23:01:10, cduvall wrote:
> Rename to TryGet

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:527:
logging.error("Caught Parse Exception")
On 2013/03/27 23:01:10, cduvall wrote:
> Make this error say something like:
> 
> 'Caught parse exception for %s: %s' % (unix_name, e)
> 
> Also, single quotes.
> 
> Also, make it a logging.warn

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source_test.py:52: self.maxDiff =
None
On 2013/03/27 23:01:10, cduvall wrote:
> take out

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:23: '%s' %
version)
On 2013/03/27 23:01:10, cduvall wrote:
> just version? or str(version)?

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:47: if
self._CheckExistence(api_name, 'trunk') is False:
On 2013/03/27 23:01:10, cduvall wrote:
> if not self._CheckExistence(api_name, 'trunk'):

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:49: return
None
On 2013/03/27 23:01:10, cduvall wrote:
> Cache this so we don't try to keep looking.

If I cache a None value, it willfail the above check:

if availability is not None:
  return availability

So I worked in a boolean

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:59: if
self._CheckExistence(api_name, '%s' % version_int) is False:
On 2013/03/27 23:01:10, cduvall wrote:
> if not ...
> 
> str(version_int)

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:65: # the api
was not found in the current version.
On 2013/03/27 23:01:10, cduvall wrote:
> capitalize API

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:70:
availability = '%d' % version_int
On 2013/03/27 23:01:10, cduvall wrote:
> str(version_int)

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:72:
availability = '%d' % version_int
On 2013/03/27 23:01:10, cduvall wrote:
> str(version_int)

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/chrome_version_data_source.py
(right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:6: import
logging
On 2013/03/27 23:01:10, cduvall wrote:
> nit: newline between logging and object_store

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chrome_version_data_source.py:66: for
entry in version_json['events']:
On 2013/03/27 23:01:10, cduvall wrote:
> why 'events' here?

events is the JSON key for a list of all the separate version/branch numbers

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/compiled_file_system.py:108:
logging.error('Could not get data for the file at %s' % path)
On 2013/03/27 23:01:10, cduvall wrote:
> ..file at %s: %s' % (path, e)

Done.

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/43001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:92: def
_CreateObjectsForBranch(branch_number, channel_info=None):
On 2013/03/27 23:01:10, cduvall wrote:
> I think this would be more clear if you had two more helpers:
> 
> _CreateAPIDataSourceForBranch(branch_number):
>   return _CreateObjectsForBranch(branch_number, None)
> 
> _CreateInstanceForBranch(branch_number, channel_info)
>   return _CreateObjectsForBranch(branch_number, channel_info)

Done.

https://codereview.chromium.org/12996003/diff/52001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/standard_apps_api.html
(right):

https://codereview.chromium.org/12996003/diff/52001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/standard_apps_api.html:29:
{{+partials.intro_table}}
oops.

[epeterson, 2013-03-28 21:44:45]

The merge conflicts from earlier just had to do with some stuff being removed
from a couple .json files and an html intro.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:483: return '%s/%s' %
(parts[0], name)
This is what I came up with for experimental_APIs like experimental_record and
experimental_identity

[cduvall, 2013-03-28 22:18:35]

lgtm with comments

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:5: // Use the
<code>chrome.downloads</code> API to programmatically initiate,
Remove trailing whitespace

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:15: from
third_party.json_schema_compiler.model import ParseException
Take out this import and use model.ParseException

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:117:
availability_string = ('Google Chrome %s' % availability)
No need for parens

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:1: # Copyright
(c) 2012 The Chromium Authors. All rights reserved.
2013 for all files

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:77: elif
version_int <= int(self._version):
can this just be an else?

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source_test.py:19:
_FILE_SYSTEMS = json.load(
Instead of making these json objets global, use _ReadFile like in some of the
other tests, and load them when you need them.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:197:
CHROME_VERSION_DATA_SOURCE_MEMCACHE,
Just construct this inline

[epeterson, 2013-03-28 23:50:36]

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/api/downloads.idl (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/api/downloads.idl:5: // Use the
<code>chrome.downloads</code> API to programmatically initiate,
On 2013/03/28 22:18:35, cduvall wrote:
> Remove trailing whitespace

Done.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:15: from
third_party.json_schema_compiler.model import ParseException
On 2013/03/28 22:18:35, cduvall wrote:
> Take out this import and use model.ParseException

Done.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:117:
availability_string = ('Google Chrome %s' % availability)
On 2013/03/28 22:18:35, cduvall wrote:
> No need for parens

Done.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:1: # Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/03/28 22:18:35, cduvall wrote:
> 2013 for all files

Done.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source.py:77: elif
version_int <= int(self._version):
On 2013/03/28 22:18:35, cduvall wrote:
> can this just be an else?

An else block would run if the generated version was greater than the current
version of chrome. This could result in availability being stored as a version
greater than the current version.

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_data_source_test.py:19:
_FILE_SYSTEMS = json.load(
On 2013/03/28 22:18:35, cduvall wrote:
> Instead of making these json objets global, use _ReadFile like in some of the
> other tests, and load them when you need them.

Done? - Is this an adequate solution?

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/67001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:197:
CHROME_VERSION_DATA_SOURCE_MEMCACHE,
On 2013/03/28 22:18:35, cduvall wrote:
> Just construct this inline

Done. And why didn't everything just have a 4-space indent?

[epeterson, 2013-03-29 01:01:25]

Hey Kalman,

Welcome back.

Clark and I have been going back and forth on preparing this patch, and I think
it's finally ready for your official review.

[not at google - send to devlin, 2013-04-02 02:58:45]

Awesome. Adding mkearney to look over the description changes while I look over
the code changes.

[not at google - send to devlin, 2013-04-02 10:29:39]

I have a small number of comments so far, but a couple might take a bit of time
to work through.

I would also like to point out a change I've been working on for a long time but
keep on having things that are more urgent:
https://codereview.chromium.org/13470005 (half finished) but will conflict with
this in a pretty major way (it will basically conflict with everything). However
- the goal of that is to make it so that we can push changes to the server and
docs simultaneously. This may be necessary to submit this patch in the end?
Well, it'd certainly make it easier in any case.

A couple more general comments I had:
- we should colour-code the warnings. Note the newest design of the server: the
drop-down has colour codes for the various channels. We should use those
colours.
- The availability strings are quite verbose; "Google Chrome 20" can just be
"Chrome 20"... some of the others we can tweak more, but I think one of my
file-level comments addresses this (and the colouring stuff).

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/api/alarms.idl:9: [availability=22]
We should be able to determine this automatically right? The alarms API was
added post-extension_api.json split (in fact it's IDL so by definition it was
post-JSON).

And yet when I remove it it says availability is 18. Hm.

(Same comment applies to all IDL files, and probably some of the JSON ones too.)

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:85: availability =
self._HandleAvailability()
inline?

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:100: if
self._availability_data_source is not None:
when is there no availability data source?

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:120: return
availability_string
All of this string logic needs to be embedded in the templates, it's not really
a concern of the server (or more pragmatically, we need to be able to tweak
wording and layout without needing a server push).

Perhaps we need to return an object with {'version': <the version number>, ...
}. The data to include there is interesting - what would also be cool is if we
could show like "has been stable for 3 years" or whatever there. So like

Availability: trunk <whatever wording we decide to use here>
Availability: Google Chrome 26 - dev channel
Availability: Google Chrome 25 - beta and dev channel
Availability: Google Chrome 24 - stable since 2013/03/01 (2+ weeks)

So that might involve like, to borrow IDL syntax,

dictionary Availability {
  int version;
  bool? beta;
  bool? dev;
  bool? trunk;
  StableAvailability? stable;
}

dictionary StableAvailability {
  string duration;  // e.g. "3+ years", "2+ months"
  string since;  // e.g. "2013/03/01"
}

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:514: def TryGet(self,
key, try_non_unix=False):
Note to future self: unclear what this should be returning, perhaps solidifying
this as a HasApi method and returning a bool would be clearer.

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:171: return
api_data_source_factory.Create(None)
divergent return types is super confusing, took me a while to parse this. why
does channel info need to be none in these cases? Can the logic for
_CreateAPIDataSourceForBranch be to grab the server instance then take the api
data source from that?

[not at google - send to devlin, 2013-04-02 21:58:16]

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:120: return
availability_string
On 2013/04/02 10:29:39, kalman wrote:
> All of this string logic needs to be embedded in the templates, it's not
really
> a concern of the server (or more pragmatically, we need to be able to tweak
> wording and layout without needing a server push).
> 
> Perhaps we need to return an object with {'version': <the version number>, ...
> }. The data to include there is interesting - what would also be cool is if we
> could show like "has been stable for 3 years" or whatever there. So like
> 
> Availability: trunk <whatever wording we decide to use here>
> Availability: Google Chrome 26 - dev channel
> Availability: Google Chrome 25 - beta and dev channel
> Availability: Google Chrome 24 - stable since 2013/03/01 (2+ weeks)
> 
> So that might involve like, to borrow IDL syntax,
> 
> dictionary Availability {
>   int version;
>   bool? beta;
>   bool? dev;
>   bool? trunk;
>   StableAvailability? stable;
> }
> 
> dictionary StableAvailability {
>   string duration;  // e.g. "3+ years", "2+ months"
>   string since;  // e.g. "2013/03/01"
> }

Ok - so I think it's fine to submit the server changes once the object model is
right, without needing to fine-tune what the templates look like. We can do that
separately.

[mkearney1, 2013-04-02 22:07:37]

This is awesome. Any chance you've got a version of the server staged that I can
look at?

On 2013/04/02 02:58:45, kalman wrote:
> Awesome. Adding mkearney to look over the description changes while I look
over
> the code changes.

[mkearney1, 2013-04-02 22:07:44]

This is awesome. Any chance you've got a version of the server staged that I can
look at?

On 2013/04/02 02:58:45, kalman wrote:
> Awesome. Adding mkearney to look over the description changes while I look
over
> the code changes.

[epeterson, 2013-04-03 00:36:35]

Just a quick note:

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/api/alarms.idl:9: [availability=22]
On 2013/04/02 10:29:39, kalman wrote:
> We should be able to determine this automatically right? The alarms API was
> added post-extension_api.json split (in fact it's IDL so by definition it was
> post-JSON).
> 
> And yet when I remove it it says availability is 18. Hm.
> 
> (Same comment applies to all IDL files, and probably some of the JSON ones
too.)

If you're using preview.py, then this patch is only interacting with old, "fake"
data, which leads to the generated availability being deemed as 18 for just
about everything (since the data must exist in the fake filesystem at version 18
and the hardcoded cutoff is at 18.

When using the appengine (along with logging messages), the generated
availability should be more accurate.

[not at google - send to devlin, 2013-04-03 00:41:14]

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/api/alarms.idl:9: [availability=22]
On 2013/04/03 00:36:35, epeterson wrote:
> On 2013/04/02 10:29:39, kalman wrote:
> > We should be able to determine this automatically right? The alarms API was
> > added post-extension_api.json split (in fact it's IDL so by definition it
was
> > post-JSON).
> > 
> > And yet when I remove it it says availability is 18. Hm.
> > 
> > (Same comment applies to all IDL files, and probably some of the JSON ones
> too.)
> 
> If you're using preview.py, then this patch is only interacting with old,
"fake"
> data, which leads to the generated availability being deemed as 18 for just
> about everything (since the data must exist in the fake filesystem at version
18
> and the hardcoded cutoff is at 18.
> 
> When using the appengine (along with logging messages), the generated
> availability should be more accurate. 

Ah - makes sense, sorry about that. I was actually thinking the other day that
we need a flag like --use-live-data to preview.py for testing exactly this. But
let's do this after I get my refactor in.

[epeterson, 2013-04-03 01:21:43]

One more note:

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:85: availability =
self._HandleAvailability()
On 2013/04/02 10:29:39, kalman wrote:
> inline?

Done.

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:100: if
self._availability_data_source is not None:
On 2013/04/02 10:29:39, kalman wrote:
> when is there no availability data source?

Each channel instance has an APIDataSourceFactory containing an availability
data source. 

The global chrome version data source is passed a method upon construction that
allows it to create APIDataSources for the purpose of checking for the existence
of APIs using TryGet(). These APIDataSources do not need an availability data
source.

[epeterson, 2013-04-03 03:59:56]

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:514: def TryGet(self,
key, try_non_unix=False):
On 2013/04/02 10:29:39, kalman wrote:
> Note to future self: unclear what this should be returning, perhaps
solidifying
> this as a HasApi method and returning a bool would be clearer.

You're right, it doesn't make sense to return an unused object here, I was a
little wary of this before.

I tried to implement your idea, but the exception handling looks messy.

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:171: return
api_data_source_factory.Create(None)
On 2013/04/02 10:29:39, kalman wrote:
> divergent return types is super confusing, took me a while to parse this. why
> does channel info need to be none in these cases? Can the logic for
> _CreateAPIDataSourceForBranch be to grab the server instance then take the api
> data source from that?

The ChromeVersionDataSource needed a way to create APIDataSources, which
involves a good amount of setup.

Creating server instances requires the same amount of setup for the
APIDataSource, with the addition of creating an AvailabilityDataSource, a
TemplateDataSourceFactory, and an ExampleZipper.

Since so much code was repeated with two separate methods (one for creating
APIDataSources and one for server instances[which rely on channel_info]), I
wanted to combine the two methods to save space.

https://codereview.chromium.org/12996003/diff/93001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/handler.py:201: def
_GetInstanceForBranch(channel_name, local_path):
This local_path doesn't seem to be used anywhere (so I removed it). Please let
me know if this was a mistake.

[not at google - send to devlin, 2013-04-04 04:49:33]

On 2013/04/02 22:07:44, mkearney1 wrote:
> This is awesome. Any chance you've got a version of the server staged that I
can
> look at?
> 
> On 2013/04/02 02:58:45, kalman wrote:
> > Awesome. Adding mkearney to look over the description changes while I look
> over
> > the code changes.

Meggin - I usually just patch changes in locally and explore that way ("git cl
patch 12996003" in this case).

[not at google - send to devlin, 2013-04-04 05:19:23]

Sorry I ran out of time to review this properly today, but there was a comment I
needed to make (... as you can see).

For some of the dependency issues you're having (e.g. that divergent return type
thing), https://codereview.chromium.org/13470005/ (which is now actually ready
for review) should fix that up.

As for the APIDataSource and AvailabilityDataSource thing, I think this is due
to my confusion as to what the role of ChromeVersionDataSource is. It doesn't
actually appear to fit into the "data source" idiom since it's not feeding any
data into templates. Rather, it's a ServerInstance-type thing which bundles up
minimal state per version?

I think the problem here may be the dependency on APIDataSource; it's too large
a dependency for AvailabilityDataSource to have particularly since it's
circular. Perhaps AvailabilityDataSource needs a more targeted interface to
query (and also; at a glance it seems like it should be responsible for figuring
out what branch an API is in, but I don't see that logic).

https://codereview.chromium.org/12996003/diff/109001/tools/json_schema_compil...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/12996003/diff/109001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema.py:311: internal=False, description=None,
availability=None):
Sorry to bring this up at this late point, but I don't think we should have the
availability attached to the API definitions. We don't want to encourage people
to put them on JSON - we only need them for old APIs.

How about putting them in a separate description in
chrome/common/extensions/docs/templates/json/api_availability.json?

https://codereview.chromium.org/12996003/diff/109001/tools/json_schema_compil...
File tools/json_schema_compiler/model.py (right):

https://codereview.chromium.org/12996003/diff/109001/tools/json_schema_compil...
tools/json_schema_compiler/model.py:61: self.availability =
json.get('availability', None)
see comment in idl_schema about this.

[not at google - send to devlin, 2013-04-04 05:20:47]

> For some of the dependency issues you're having (e.g. that divergent return
type
> thing), https://codereview.chromium.org/13470005/ (which is now actually ready
> for review) should fix that up.
> 

Actually don't think this is true, but hopefully the other things I said are.

[epeterson, 2013-04-17 03:38:50]

Alright, I've made some updates to this patch. Feel free to take a look when you
have time. 

Also, a note:
This is what the intro tables currently look like:

http://i.imgur.com/eJhin7S.png

There are two separate tables, one dynamically generated, and one static. There
doesn't appear to be an easy way to fix this using HTML/templates/CSS.

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:16: 
Removed the [huge] dependency on api_data_source for generating availability.
availability_data_source takes care of it now.

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:32: 
This hasn't been tested on the appengine, because the new .json file is only
available locally so far. It definitely works while running preview.py, though

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:99: names =
names_cache.GetFromFileListing(svn_constants.API_PATH)
Instead of trying to load every file and check for a FileNotFoundError, we now
just check the file names present in the api directory (and cache the names,
too). This is faster.

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:22: }
Here's the central .json file for existing availabilities

[not at google - send to devlin, 2013-04-17 18:32:30]

Cool, will do this asap - but a quick comment in the meantime -

> Alright, I've made some updates to this patch. Feel free to take a look when
you
> have time. 
> 
> Also, a note:
> This is what the intro tables currently look like:
> 
> http://i.imgur.com/eJhin7S.png
> 
> There are two separate tables, one dynamically generated, and one static.
There
> doesn't appear to be an easy way to fix this using HTML/templates/CSS.

Yeah... bummer. That's not ideal (though, awesome to get this stuff working,
really happy with how it's turning out, this is super critical).

Anyway - here's an idea. Yet more of a yak shave but such it is. Could we pull
out the existing intro table content into JSON? It would look something like:

=== intro_tables.json

{
  "storage": [
    { "Permissions": "<code>storage</code>" },
    { "Learn more": "<a href>...</a><br><a>... }
  ],
  ... other APIs
}

then we can fully generate the tables, and later we can remove the "Permissions"
when that gets implemented.

[not at google - send to devlin, 2013-04-17 18:32:44]

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:22: }
On 2013/04/17 03:38:50, epeterson wrote:
> Here's the central .json file for existing availabilities

hm, without reviewing the code, I would have expected to only need to specify
these for versions less than... 22 is it? Can't remember. And why the need for
specifying trunk/dev in those places?

[mkearney1, 2013-04-17 20:10:14]

This looks really cool (took on the patch and staged locally).

Presumably you are still working out what to do when something is available in
multiple channels, but not stable. Ben, is there ever the possibility that
something is available in Beta channel, but not Dev and Trunk? Maybe we could
have a message the indicates the latest channels, without worrying about earlier
channels?

You're also working on experimental. I could have sworn I drafted something for
experimental.identity API availability that might have helped but it seems to be
gone. 

What about a generic message, something like 'Experimental only (see <a
href="experimental.html#using">How to use experimental APIs</a>).

[epeterson, 2013-04-18 00:17:56]

On 2013/04/17 18:32:44, kalman wrote:
>
https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
> File chrome/common/extensions/docs/templates/json/api_availabilities.json
> (right):
> 
>
https://codereview.chromium.org/12996003/diff/190001/chrome/common/extensions...
> chrome/common/extensions/docs/templates/json/api_availabilities.json:22: }
> On 2013/04/17 03:38:50, epeterson wrote:
> > Here's the central .json file for existing availabilities
> 
> hm, without reviewing the code, I would have expected to only need to specify
> these for versions less than... 22 is it? Can't remember. And why the need for
> specifying trunk/dev in those places?

Right, the api directory didn't exist until version 18, so availability can't be
generated dynamically for those.

For the availabilities greater than version 18, I pulled the versions for these
APIs directly from the HTML intro pages. The problem is that the availability
found in the intro tables often didn't match up with the version where the API
first appeared in the file system.

Here's a list of cases where that's true:
  <HTML intro availability>      <generated availability>
  "storage" : "20",              /*19*/
  "declarativeContent" : "trunk",/*26*/
  "runtime" : "22",              /*21*/
  "syncFileSystem" : "27",       /*24*/
  "usb" : "26",                  /*24*/
  "appWindow" : "23",            /*21*/
  "alarms" : "22",               /*21*/
  "notifications" : "dev",       /*27*/
  "serial" : "23",               /*22*/
  "socket" : "24",               /*22*/
  "bluetooth" : "dev",           /*24*/
  "fileSystem" : "23"            /*21*/

So, having these listed in api_availabilities.json honors the manually -chosen
availability as being the version where the API was first truly available.

In regard to the dev and trunk availabilities, these APIs currently have a
special disclaimer about only being available on these channels, so it seemed
right to handle these in a special way.

Links:
<trunk> http://developer.chrome.com/extensions/declarativeContent.html
<dev>   http://developer.chrome.com/apps/bluetooth.html

[epeterson, 2013-04-18 00:24:35]

On 2013/04/17 20:10:14, mkearney1 wrote:
> This looks really cool (took on the patch and staged locally).
> 
> Presumably you are still working out what to do when something is available in
> multiple channels, but not stable. Ben, is there ever the possibility that
> something is available in Beta channel, but not Dev and Trunk? Maybe we could
> have a message the indicates the latest channels, without worrying about
earlier
> channels?
> 
> You're also working on experimental. I could have sworn I drafted something
for
> experimental.identity API availability that might have helped but it seems to
be
> gone. 
> 
> What about a generic message, something like 'Experimental only (see <a
> href="experimental.html#using">How to use experimental APIs</a>).

I hadn't seen any examples of APIs specified as being available on multiple
channels, other than stable, in the HTML intro tables. Generally, just one
channel or version is used to show availability. But, I don't think this would
be hard to implement using the api_availabilities.json file, if it needs to be
done.

We can also have a special message shown for Experimental APIs, if they should
not be displaying a generated availability (or, we could have both the message
and availability?).

[not at google - send to devlin, 2013-04-18 03:05:49]

Hm, so, there are two things going on here. For some API, there will be a
version X where the schema (idl/json) for it first appears, and a later version
X + N being the first version it appears in the stable channel (specified in
permission_features). N may be 0.

What we should be showing here is (X+N), which is probably what the manual
versions show? But, we should be able to figure that out rather than needing to
specify it manually.

That's just for stable APIs though. The channel stuff is slightly more
complicated though the algorithm should be basically be,

def GetAvailability():
  available_channel = filter(IsAvailable, ['stable', 'beta', 'dev', 'trunk'])[0]
  if available_channel == 'stable':
    return str(GetEarliestStableVersion())  # X + N
  return available_channel

If you know what I mean... probably wouldn't actually be written that way
obviously...

Apologies if this is sort of the thing you've written, I haven't had a chance to
get to it today. But I did get the feeling there was a bit of confusion going
on.

P.S. yeah let's not try to handle the case where something actually become
unavailable (the example that Meggin had). assume the release managers have this
under control.

P.P.S. is there a BUG for this or do we need to file one?

[not at google - send to devlin, 2013-04-18 03:55:32]

> === intro_tables.json
> 
> {
>   "storage": [
>     { "Permissions": "<code>storage</code>" },
>     { "Learn more": "<a href>...</a><br><a>... }
>   ],
>   ... other APIs
> }
> 

(or you might have more luck feeding { "title": "Permissions", "content":
"<...>" } into the templates.)

[epeterson, 2013-04-18 23:43:15]

On 2013/04/18 03:05:49, kalman wrote:
> Hm, so, there are two things going on here. For some API, there will be a
> version X where the schema (idl/json) for it first appears, and a later
version
> X + N being the first version it appears in the stable channel (specified in
> permission_features). N may be 0.
> 
> What we should be showing here is (X+N), which is probably what the manual
> versions show? But, we should be able to figure that out rather than needing
to
> specify it manually.
> 
> That's just for stable APIs though. The channel stuff is slightly more
> complicated though the algorithm should be basically be,
> 
> def GetAvailability():
>   available_channel = filter(IsAvailable, ['stable', 'beta', 'dev',
'trunk'])[0]
>   if available_channel == 'stable':
>     return str(GetEarliestStableVersion())  # X + N
>   return available_channel
> 
> If you know what I mean... probably wouldn't actually be written that way
> obviously...
> 
> Apologies if this is sort of the thing you've written, I haven't had a chance
to
> get to it today. But I did get the feeling there was a bit of confusion going
> on.
> 
> P.S. yeah let's not try to handle the case where something actually become
> unavailable (the example that Meggin had). assume the release managers have
this
> under control.
> 
> P.P.S. is there a BUG for this or do we need to file one?

I don't believe there's been a BUG filed for this yet. 
I'll get to work on finding the 'stable' availability for APIs that have been
released on stable, as well as combining the separate HTML tables.

[epeterson, 2013-04-25 00:51:57]

On 2013/04/18 23:43:15, epeterson wrote:
> On 2013/04/18 03:05:49, kalman wrote:
> > Hm, so, there are two things going on here. For some API, there will be a
> > version X where the schema (idl/json) for it first appears, and a later
> version
> > X + N being the first version it appears in the stable channel (specified in
> > permission_features). N may be 0.
> > 
> > What we should be showing here is (X+N), which is probably what the manual
> > versions show? But, we should be able to figure that out rather than needing
> to
> > specify it manually.
> > 
> > That's just for stable APIs though. The channel stuff is slightly more
> > complicated though the algorithm should be basically be,
> > 
> > def GetAvailability():
> >   available_channel = filter(IsAvailable, ['stable', 'beta', 'dev',
> 'trunk'])[0]
> >   if available_channel == 'stable':
> >     return str(GetEarliestStableVersion())  # X + N
> >   return available_channel
> > 
> > If you know what I mean... probably wouldn't actually be written that way
> > obviously...
> > 
> > Apologies if this is sort of the thing you've written, I haven't had a
chance
> to
> > get to it today. But I did get the feeling there was a bit of confusion
going
> > on.
> > 
> > P.S. yeah let's not try to handle the case where something actually become
> > unavailable (the example that Meggin had). assume the release managers have
> this
> > under control.
> > 
> > P.P.S. is there a BUG for this or do we need to file one?
> 
> I don't believe there's been a BUG filed for this yet. 
> I'll get to work on finding the 'stable' availability for APIs that have been
> released on stable, as well as combining the separate HTML tables.

Ok, just an update:

I saw that you wanted to review this patch/have it ready by the end of the week,
so I'm going to at least get an updated CL to you for review before then.

It looks like I'm successfully finding the earliest 'stable' availability for
APIs (if one is available). 
If an API isn't represented in _permission_features, then we fall back to to
_manifest_features. 
Some APIs, like windows or events, don't appear in either of those files. In
that case, what I'm currently doing is just checking for their existence in the
filesystem (these two APIs, at least, have been stable for a while)

At this point, I am working on updating the unittests before uploading a new CL.

[epeterson, 2013-04-26 04:17:27]

On 2013/04/25 00:51:57, epeterson wrote:
> On 2013/04/18 23:43:15, epeterson wrote:
> > On 2013/04/18 03:05:49, kalman wrote:
> > > Hm, so, there are two things going on here. For some API, there will be a
> > > version X where the schema (idl/json) for it first appears, and a later
> > version
> > > X + N being the first version it appears in the stable channel (specified
in
> > > permission_features). N may be 0.
> > > 
> > > What we should be showing here is (X+N), which is probably what the manual
> > > versions show? But, we should be able to figure that out rather than
needing
> > to
> > > specify it manually.
> > > 
> > > That's just for stable APIs though. The channel stuff is slightly more
> > > complicated though the algorithm should be basically be,
> > > 
> > > def GetAvailability():
> > >   available_channel = filter(IsAvailable, ['stable', 'beta', 'dev',
> > 'trunk'])[0]
> > >   if available_channel == 'stable':
> > >     return str(GetEarliestStableVersion())  # X + N
> > >   return available_channel
> > > 
> > > If you know what I mean... probably wouldn't actually be written that way
> > > obviously...
> > > 
> > > Apologies if this is sort of the thing you've written, I haven't had a
> chance
> > to
> > > get to it today. But I did get the feeling there was a bit of confusion
> going
> > > on.
> > > 
> > > P.S. yeah let's not try to handle the case where something actually become
> > > unavailable (the example that Meggin had). assume the release managers
have
> > this
> > > under control.
> > > 
> > > P.P.S. is there a BUG for this or do we need to file one?
> > 
> > I don't believe there's been a BUG filed for this yet. 
> > I'll get to work on finding the 'stable' availability for APIs that have
been
> > released on stable, as well as combining the separate HTML tables.
> 
> Ok, just an update:
> 
> I saw that you wanted to review this patch/have it ready by the end of the
week,
> so I'm going to at least get an updated CL to you for review before then.
> 
> It looks like I'm successfully finding the earliest 'stable' availability for
> APIs (if one is available). 
> If an API isn't represented in _permission_features, then we fall back to to
> _manifest_features. 
> Some APIs, like windows or events, don't appear in either of those files. In
> that case, what I'm currently doing is just checking for their existence in
the
> filesystem (these two APIs, at least, have been stable for a while)
> 
> At this point, I am working on updating the unittests before uploading a new
CL.

Alright, the most recent patchset that I've just put up is ready for review.

I realize there are a lot of files to look through; please let me know if any
clarification is needed.

This patch set changes availability_data_source to use _permission_features.json
to find an API's earliest stable availability, or whatever channel it is
currently available on. Failing that, availability_data_source uses
_manifest_features.json, and in the event that an API isn't in either of these
files, a simple check for existence in the file system is used.

Currently, this isn't working for the three devtools schemas:
devtools.inspectedWindow,
devtools.panels,
devtools.network,
since these are nested in a subdirectory. 
input.ime is also giving this patch some troubles.

I wanted to at least give you something to review in the meantime, however, and,
if necessary, manually-found availabilities can be added to the
api-availabilities.json file for these APIs.

[not at google - send to devlin, 2013-04-27 01:48:34]

I shouldn't have left this for friday, it's a bit much. I'll need to look at
AvailabilityDataSource more closely next week - but it'd be great if you could
rebase so I can patch it in and play with it.

But it's looking great.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:23: FILESYS =
'filesys'
this is what the category= parameter to ObjectStoreCreator.Create is for, it
does namespacing automatically.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:90:
self._object_store_creator_factory)
so this is obviously the same as server instance's version, and we should re-use
the code.

in fact we need to re-use the code, since part of the contract of all this stuff
now is that appengine instances aren't allowed to touch the network, only object
store - so this class needs to know to construct offline subversion file systems
in that case, and online file systems for cron jobs.

you'll probably need to inject a factory-style method/class into here that can
construct SubversionFileSystems from a branch - or at least, one that's either
wrapped in a cache or an offline version.

would it work for those methods to actually just create a whole ServerInstance
then access the svn_file_sytsem from it? Like

--- server_instance.py

@staticmethod
def CreateOnline(channel):
  @memoize
  def svn_file_system_creator(branch):
    return CreateOnline(branch).svn_file_system   <-- oh crap it's a branch not
a channel
  return ServerInstance(..., svn_file_system_creator)

@staticmethod
@memoize
def CreateOffline(channel):
  def svn_file_system_creator(branch):
    return CreateOffline(branch).svn_file_system  <-- same oh crap
  return ServerInstance(..., svn_file_system_creator)

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:56: }
can you make this return an actual struct so that field are accessed like
info.branch not info['branch']?

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/compiled_file_system.py:5: import logging
unnecessary

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:6: }
nice! I'm curious, what's preventing these APIs from being detected,
particularly contextMenus?

(and ... how are you determining the old ones?)

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/intro_tables.json (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:2: "commands": [
can you sort the top-level dict by API name?

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/intro_table.html (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:14: No
description is currently available.
it'd be nice to make sure this never happens...

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:29: No
availability is currently available.
we need to make sure this never happens.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:52:
{{{api.learn_more}}}
learn more are always links right? we could encode that in the JSON.

because using {{{ }}} is dangerous.

[epeterson, 2013-04-27 23:17:23]

Ok, I've made a few changes, and at the time of this writing the patch should be
up-to-date with master.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:56: }
On 2013/04/27 01:48:34, kalman wrote:
> can you make this return an actual struct so that field are accessed like
> info.branch not info['branch']?

Done(?) Is this what you had in mind?

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/compiled_file_system.py:5: import logging
On 2013/04/27 01:48:34, kalman wrote:
> unnecessary

Done.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:6: }
On 2013/04/27 01:48:34, kalman wrote:
> nice! I'm curious, what's preventing these APIs from being detected,
> particularly contextMenus?
> 
> (and ... how are you determining the old ones?)

Definitely take a look at how I'm using the _permission_features.json and
_manifest_features.json files in availability_data_source when you get the
chance.

These features files are only present until version 19 (although
_permission_features lacks the "channel" key for APIs, so I'm only going to
version 20).

Version 18 doesn't have either of these files, only individual API schemas.

From version 17 down to version 5, the extension_api.json file contains
namespaces for each API. I was looking at possibly using this to generate
availability for older APIs. It would work for idle, contextMenus, tts (webstore
is weird, though.. there's only webstorePrivate in the pre-18 versions), as well
as changing availability for APIs like tabs or windows from the current "20" to
a more accurate "5."

So, in summary, 
versions 20+ is what this patch currently handles
version 19 is a special case
version 18 is also a special case
versions 17-5 have extension_api.json files which could be used to find
availability for older APIs

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/intro_tables.json (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:2: "commands": [
On 2013/04/27 01:48:34, kalman wrote:
> can you sort the top-level dict by API name?

Done.

[not at google - send to devlin, 2013-04-30 17:20:29]

Replying to your comments before I go through this again.

Also adding that after https://codereview.chromium.org/14273041/ goes in we'll
want to show a similar thing for manifest files. As I said there, it can be in a
follow-up (and you can decide amongst yourselves who implements it), but keep it
in mind.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:56: }
On 2013/04/27 23:17:23, epeterson wrote:
> On 2013/04/27 01:48:34, kalman wrote:
> > can you make this return an actual struct so that field are accessed like
> > info.branch not info['branch']?
> 
> Done(?) Is this what you had in mind?

Yep, thanks.

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/203001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:6: }
On 2013/04/27 23:17:23, epeterson wrote:
> On 2013/04/27 01:48:34, kalman wrote:
> > nice! I'm curious, what's preventing these APIs from being detected,
> > particularly contextMenus?
> > 
> > (and ... how are you determining the old ones?)
> 
> Definitely take a look at how I'm using the _permission_features.json and
> _manifest_features.json files in availability_data_source when you get the
> chance.
> 
> These features files are only present until version 19 (although
> _permission_features lacks the "channel" key for APIs, so I'm only going to
> version 20).
> 
> Version 18 doesn't have either of these files, only individual API schemas.
> 
> From version 17 down to version 5, the extension_api.json file contains
> namespaces for each API. I was looking at possibly using this to generate
> availability for older APIs. It would work for idle, contextMenus, tts
(webstore
> is weird, though.. there's only webstorePrivate in the pre-18 versions), as
well
> as changing availability for APIs like tabs or windows from the current "20"
to
> a more accurate "5."
> 
> So, in summary, 
> versions 20+ is what this patch currently handles
> version 19 is a special case
> version 18 is also a special case
> versions 17-5 have extension_api.json files which could be used to find
> availability for older APIs

My comment was more that contextMenus seems no more special-casey than say tabs
or windows. I'm wondering what makes it necessary to specify this.

Ah - and now I see, I presume it's because contextMenus.html contains version 6
manually.

Yep. I think, unfortunately, it will be tricky to push this patch since tabs
will show up as version 20. If it's not too much trouble, going to back to when
the APIs were added to extension_api.json would be nice... then again, it's (a)
never going to change and (b) will introduce some extra work (... if one-off)
for the server and (c) is probably going to be slower than just going back and
figuring out what changes between each version.

Actually... (c) I'm not so sure. I'll let you be the judge.

[not at google - send to devlin, 2013-04-30 18:34:18]

This is looking great.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/api/alarms.idl:4: // TODO(mpcomplete): We need
documentation before we can release this.
erm, this comment seems wrong, given we released it...

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/api/alarms.idl:8: 
no extra newline. maybe the other IDL files have that, you should delete them.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/api/browser_action.json:8: "description": "Use browser
actions to put icons in the main Google Chrome toolbar, to the right of the
address bar. In addition to its <a href='#icon'>icon</a>, a browser action can
also have a <a href='#tooltip'>tooltip</a>, a <a href='#badge'>badge</a>, and a
<a href = '#popups'>popup</a>.",
Do these links actually go somewhere?

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:94: json_string =
svn_file_system.ReadSingle('%s/intro_tables.json'
nicer line break would be
svn_file_system.ReadSingle(
    '%s/intro_tables.json' % svn_constants.JSON_PATH)

though, you might as well just pass in the intro table directly and hold onto
that everywhere rather than the svn file system (rather than
self._svn_file_system = ... have self._intro_table =
svn_file_system.ReadSingle(...))

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:157: return None
pls don't use exceptions for control flow. Also assuming that the permissions
come first is asking for a confused engineer at some point.

Something like:

permission_rows = [row for row in tab_info if 'Permissions' in row]
return permission_rows[0] if permission_rows else None

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:169: return None
ditto regarding the filtering, not assuming it's there, not excepting, not using
[1].

but like I said, maybe this should actually return a list so that the templates
can lay it out nicely.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:29: '{"tester":
[{"Permissions": "<code>tester</code>"}]}'
what about Learn More? No Learn More? No Permissions?

where are the tests for the extra logic?

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:72:
_JSCModel._GetAvailability = _FakeGetAvailability
this is what passing in a fake availability data source is for. please don't
overwrite private methods of classes.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:91:
fake_avail_factory = FakeAvailabilityDataSourceFactory()
inline

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:96:
TestFileSystem(deepcopy(_INTRO_TABLES_DATA))).ToDict()
save a reference in setUp?

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:112:
fake_avail_factory,
the only reason this works is because the test is disabled :)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:127:
TestFileSystem(deepcopy(_INTRO_TABLES_DATA))).ToDict()
ditto inline, safe reference comment

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:23: FILESYS =
'filesys'
use these in category= when creating the object store rather than namespacing
everything.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:45: %
svn_constants.API_PATH)
use third_party.json_schema_compiler.json_parse as json_parse here, to get the
ordered_dict behaviour.

nicer way to factor:

def read_json(path):
  return json_parse.Parse(svn_file_system.ReadSingle(path))
self._existing_availability = read_json(...)
self._permission_feature = read_json(...)
self._manifest_features = read_json(...)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:72: def
_CreateSvnFileSystemForBranch(self, branch):
maybe this should go as a little helper on SubversionFileSystem.

class SubversionFileSystem(object):
  @staticmethod
  CreateForBranch(branch):
    ... this

use from ServerInstance.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:102: channel =
api_info[0].get('channel')
don't use exceptions for control flow. Also, if it's a list we need to actually
find the min availability - the only reason these are in lists usually is to
whitelist the API in stable for some extension. This shouldn't show up as stable
here, it should show up as dev. Maybe BranchUtility needs to expose a util to
order these, like BranchUtility.Newest(list of branch names)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:120:
self._chrome_version_utility.GetBranchNumberForVersion(version))
I don't think you can put svn file systems in object stores, it's not supposed
to be pickleable data. Nor should we be pickling it anyway.

Just put a @memoize on the CreateSvnFileSystemForBranch.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:125:
permission_features = json.loads(json_comment_eater.Nom(json_string))
same comment re json parsing i.e. use json_parse.Parse (and inline it).

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:160: return
existence
similar comments

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:184: return
api_name in names
similar comments

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:206: return
str(version)
return version + 1 rather than incrementing

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:218:
availability = self._existing_availabilities.get(api_name, None)
the None is implicit in get()

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:240:
availability = None
throw a FileNotFoundError here or something, so we notice the failures. If our
data is correct it shouldn't happen, right?

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:249: return
availability
slightly nicer factoring:

if availability == 'stable':
  availability = self._FindEarliest...
self._obejcT_store.Set()
return availability

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source_test.py:105: 
what about dev/beta/trunk?

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:51: def
GetChannelInfoForChannelName(self, channel):
Ok I think that the "ForChannelName" is tautological in this context, since it's
getting channel info, one assumes it's for a channel name.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:161: svn_file_system)
I think we need to pass in a special trunk svn file system here?

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_data_source_test.py:55:
fake_avail_factory = _FakeFactory()
inline

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_data_source_test.py:60: {}) #
Unnecessary file system
just create a TestFileSystem and avoid the comment

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/alarms.html (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/alarms.html:1: <table></table>
if any of the intros end up looking like this, just delete them.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/app_runtime.html (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/app_runtime.html:1:
<table></table>
... etc.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:2:
"contextMenus" : "6",
no space between key and :.

Also these should probably be numbers not strings.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/intro_tables.json (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:3: {
"Permissions": "<code>\"alarms\"</code>" },
Permissions are always in <code>"..."</code>, and when they get replaced with
figuring out from _permission_features.json that will probably need to be
incorporated into the templates. So let's just make these the name of the
permissions.

(generally speaking, the less HTML specific layout in these files, the better)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:7: {
"Permissions": "None" },
Likewise, leave out permissions if there aren't any.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:8: { "Learn
more": "<a href='app_lifecycle.html'>Manage App Lifecycle</a><br><a
href='https://developers.google.com/live/shows/10291095/'>Chrome Office Hours:
Introduction to Chrome Apps</a>" }
Maybe "Learn more" should be a list in the JSON.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:34: {
"Permissions": "<code>\"cookies\"</code><br><a
href=\"declare_permissions.html\">host permissions</a>" }
Looking through this file more carefully, I'm thinking that 'Permissions'
actually needs to be a bit richer unfortunately, since there is a lot of extra
data in here that we probably shouldn't get rid of. Perhaps like

"Permissions": {"name": "contentSettings"}

"Permissions": {
  "name": "cookies",
  "extra": "<a href=\"declare_permissions.html#WHAT_IS_THE_ANCHOR\">host
permissions</a>"
}

all of the extra crap (examples, media galleries specifics, file system
specifics) can go in extra, which is HTML.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:40: {
"Permissions": "<code>\"declarativeContent\"</code><!--TODO: Mention host
permissions if/when some actions require them.-->" },
let's convert the HTML comments into JSON comments (presumably these go through,
or can go through, the json comment eater)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:144: ] }
put the closing } on its own line. sorry.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/intro_table.html (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:14: No
description is currently available.
We really need to generate something less embarrassing here. Can we fake it and
say like

The <code>chrome.{{api.name}}</code> API.

I think there's name info somewhere.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:29: No
availability is currently available.
If the availability isn't available, it's probably available. Maybe we should
say "Available". Kind of a cop-out but it's less embarrassing than admitting we
have incomplete docs.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:31: {{/}}
it's nice to close these off with the name of the tag, like HTML. same elsewhere
in this files.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:39:
{{?api.intro_permissions}}
I don't see the point of having this be intro_permissions. Just permissions is
more representative of the API model.

[epeterson, 2013-05-13 02:38:10]

Evening,

This should cover your requests from last time.
Still have issues with offline/online file access, though.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/api/alarms.idl:4: // TODO(mpcomplete): We need
documentation before we can release this.
On 2013/04/30 18:34:19, kalman wrote:
> erm, this comment seems wrong, given we released it...

Done. I noticed this earlier, but removing someone else's TODO just didn't feel
right..

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/api/alarms.idl:8: 
On 2013/04/30 18:34:19, kalman wrote:
> no extra newline. maybe the other IDL files have that, you should delete them.

Done. (For the other IDL files, as well)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/api/browser_action.json:8: "description": "Use browser
actions to put icons in the main Google Chrome toolbar, to the right of the
address bar. In addition to its <a href='#icon'>icon</a>, a browser action can
also have a <a href='#tooltip'>tooltip</a>, a <a href='#badge'>badge</a>, and a
<a href = '#popups'>popup</a>.",
On 2013/04/30 18:34:19, kalman wrote:
> Do these links actually go somewhere?

They do; just in case you're skeptical:
http://developer.chrome.com/extensions/browserAction.html

Although these parts of the page aren't too away from the actual intro table if
you just scroll down a bit..

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:94: json_string =
svn_file_system.ReadSingle('%s/intro_tables.json'
On 2013/04/30 18:34:19, kalman wrote:
> nicer line break would be
> svn_file_system.ReadSingle(
>     '%s/intro_tables.json' % svn_constants.JSON_PATH)
> 
> though, you might as well just pass in the intro table directly and hold onto
> that everywhere rather than the svn file system (rather than
> self._svn_file_system = ... have self._intro_table =
> svn_file_system.ReadSingle(...))

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:157: return None
On 2013/04/30 18:34:19, kalman wrote:
> pls don't use exceptions for control flow. Also assuming that the permissions
> come first is asking for a confused engineer at some point.
> 
> Something like:
> 
> permission_rows = [row for row in tab_info if 'Permissions' in row]
> return permission_rows[0] if permission_rows else None

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:169: return None
On 2013/04/30 18:34:19, kalman wrote:
> ditto regarding the filtering, not assuming it's there, not excepting, not
using
> [1].
> 
> but like I said, maybe this should actually return a list so that the
templates
> can lay it out nicely.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:29: '{"tester":
[{"Permissions": "<code>tester</code>"}]}'
On 2013/04/30 18:34:19, kalman wrote:
> what about Learn More? No Learn More? No Permissions?
> 
> where are the tests for the extra logic?

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:72:
_JSCModel._GetAvailability = _FakeGetAvailability
On 2013/04/30 18:34:19, kalman wrote:
> this is what passing in a fake availability data source is for. please don't
> overwrite private methods of classes.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:91:
fake_avail_factory = FakeAvailabilityDataSourceFactory()
On 2013/04/30 18:34:19, kalman wrote:
> inline

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:96:
TestFileSystem(deepcopy(_INTRO_TABLES_DATA))).ToDict()
On 2013/04/30 18:34:19, kalman wrote:
> save a reference in setUp?

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:112:
fake_avail_factory,
On 2013/04/30 18:34:19, kalman wrote:
> the only reason this works is because the test is disabled :)

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:127:
TestFileSystem(deepcopy(_INTRO_TABLES_DATA))).ToDict()
On 2013/04/30 18:34:19, kalman wrote:
> ditto inline, safe reference comment

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:23: FILESYS =
'filesys'
On 2013/04/30 18:34:19, kalman wrote:
> use these in category= when creating the object store rather than namespacing
> everything.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:45: %
svn_constants.API_PATH)
On 2013/04/30 18:34:19, kalman wrote:
> use third_party.json_schema_compiler.json_parse as json_parse here, to get the
> ordered_dict behaviour.
> 
> nicer way to factor:
> 
> def read_json(path):
>   return json_parse.Parse(svn_file_system.ReadSingle(path))
> self._existing_availability = read_json(...)
> self._permission_feature = read_json(...)
> self._manifest_features = read_json(...)

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:72: def
_CreateSvnFileSystemForBranch(self, branch):
On 2013/04/30 18:34:19, kalman wrote:
> maybe this should go as a little helper on SubversionFileSystem.
> 
> class SubversionFileSystem(object):
>   @staticmethod
>   CreateForBranch(branch):
>     ... this
> 
> use from ServerInstance.

Well, I had done this, and modified the old CreateOnline/GetOrCreateOffline
methods in ServerInstance. But these methods were separated from ServerInstance
with a previous patch, so I tried to work with that. See the updated code in
cron_servlet, instance_servlet.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:102: channel =
api_info[0].get('channel')
On 2013/04/30 18:34:19, kalman wrote:
> don't use exceptions for control flow. Also, if it's a list we need to
actually
> find the min availability - the only reason these are in lists usually is to
> whitelist the API in stable for some extension. This shouldn't show up as
stable
> here, it should show up as dev. Maybe BranchUtility needs to expose a util to
> order these, like BranchUtility.Newest(list of branch names)

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:120:
self._chrome_version_utility.GetBranchNumberForVersion(version))
On 2013/04/30 18:34:19, kalman wrote:
> I don't think you can put svn file systems in object stores, it's not supposed
> to be pickleable data. Nor should we be pickling it anyway.
> 
> Just put a @memoize on the CreateSvnFileSystemForBranch.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:125:
permission_features = json.loads(json_comment_eater.Nom(json_string))
On 2013/04/30 18:34:19, kalman wrote:
> same comment re json parsing i.e. use json_parse.Parse (and inline it).

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:160: return
existence
On 2013/04/30 18:34:19, kalman wrote:
> similar comments

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:184: return
api_name in names
On 2013/04/30 18:34:19, kalman wrote:
> similar comments

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:206: return
str(version)
On 2013/04/30 18:34:19, kalman wrote:
> return version + 1 rather than incrementing

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:218:
availability = self._existing_availabilities.get(api_name, None)
On 2013/04/30 18:34:19, kalman wrote:
> the None is implicit in get()

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:240:
availability = None
On 2013/04/30 18:34:19, kalman wrote:
> throw a FileNotFoundError here or something, so we notice the failures. If our
> data is correct it shouldn't happen, right?

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:249: return
availability
On 2013/04/30 18:34:19, kalman wrote:
> slightly nicer factoring:
> 
> if availability == 'stable':
>   availability = self._FindEarliest...
> self._obejcT_store.Set()
> return availability

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source_test.py:105: 
On 2013/04/30 18:34:19, kalman wrote:
> what about dev/beta/trunk?

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:51: def
GetChannelInfoForChannelName(self, channel):
On 2013/04/30 18:34:19, kalman wrote:
> Ok I think that the "ForChannelName" is tautological in this context, since
it's
> getting channel info, one assumes it's for a channel name.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:161: svn_file_system)
On 2013/04/30 18:34:19, kalman wrote:
> I think we need to pass in a special trunk svn file system here?
Definitely, thanks for catching that.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_data_source_test.py:55:
fake_avail_factory = _FakeFactory()
On 2013/04/30 18:34:19, kalman wrote:
> inline

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_data_source_test.py:60: {}) #
Unnecessary file system
On 2013/04/30 18:34:19, kalman wrote:
> just create a TestFileSystem and avoid the comment

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/alarms.html (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/alarms.html:1: <table></table>
On 2013/04/30 18:34:19, kalman wrote:
> if any of the intros end up looking like this, just delete them.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/app_runtime.html (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/app_runtime.html:1:
<table></table>
On 2013/04/30 18:34:19, kalman wrote:
> ... etc.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:2:
"contextMenus" : "6",
On 2013/04/30 18:34:19, kalman wrote:
> no space between key and :.
> 
> Also these should probably be numbers not strings.

They can be numbers, but it may also be useful to set channels here. For
example, the notifications API is really only available on dev but has some
methods available on stable. There's a note explaining this in
_permission_features.json where it's listed as 'stable'.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/intro_tables.json (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:3: {
"Permissions": "<code>\"alarms\"</code>" },
On 2013/04/30 18:34:19, kalman wrote:
> Permissions are always in <code>"..."</code>, and when they get replaced with
> figuring out from _permission_features.json that will probably need to be
> incorporated into the templates. So let's just make these the name of the
> permissions.
> 
> (generally speaking, the less HTML specific layout in these files, the better)

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:7: {
"Permissions": "None" },
On 2013/04/30 18:34:19, kalman wrote:
> Likewise, leave out permissions if there aren't any.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:8: { "Learn
more": "<a href='app_lifecycle.html'>Manage App Lifecycle</a><br><a
href='https://developers.google.com/live/shows/10291095/'>Chrome Office Hours:
Introduction to Chrome Apps</a>" }
On 2013/04/30 18:34:19, kalman wrote:
> Maybe "Learn more" should be a list in the JSON.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:34: {
"Permissions": "<code>\"cookies\"</code><br><a
href=\"declare_permissions.html\">host permissions</a>" }
On 2013/04/30 18:34:19, kalman wrote:
> Looking through this file more carefully, I'm thinking that 'Permissions'
> actually needs to be a bit richer unfortunately, since there is a lot of extra
> data in here that we probably shouldn't get rid of. Perhaps like
> 
> "Permissions": {"name": "contentSettings"}
> 
> "Permissions": {
>   "name": "cookies",
>   "extra": "<a href=\"declare_permissions.html#WHAT_IS_THE_ANCHOR\">host
> permissions</a>"
> }
> 
> all of the extra crap (examples, media galleries specifics, file system
> specifics) can go in extra, which is HTML.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:40: {
"Permissions": "<code>\"declarativeContent\"</code><!--TODO: Mention host
permissions if/when some actions require them.-->" },
On 2013/04/30 18:34:19, kalman wrote:
> let's convert the HTML comments into JSON comments (presumably these go
through,
> or can go through, the json comment eater)

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:144: ] }
On 2013/04/30 18:34:19, kalman wrote:
> put the closing } on its own line. sorry.

Done. (sorry.)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/intro_table.html (right):

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:14: No
description is currently available.
On 2013/04/30 18:34:19, kalman wrote:
> We really need to generate something less embarrassing here. Can we fake it
and
> say like
> 
> The <code>chrome.{{api.name}}</code> API.
> 
> I think there's name info somewhere.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:29: No
availability is currently available.
On 2013/04/30 18:34:19, kalman wrote:
> If the availability isn't available, it's probably available. Maybe we should
> say "Available". Kind of a cop-out but it's less embarrassing than admitting
we
> have incomplete docs.

Done. (ha.)

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:31: {{/}}
On 2013/04/30 18:34:19, kalman wrote:
> it's nice to close these off with the name of the tag, like HTML. same
elsewhere
> in this files.

Done.

https://codereview.chromium.org/12996003/diff/220001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_table.html:39:
{{?api.intro_permissions}}
On 2013/04/30 18:34:19, kalman wrote:
> I don't see the point of having this be intro_permissions. Just permissions is
> more representative of the API model.

It looks like 'permissions' is already reserved for other data in
api_data_source (used in determining what channel the API is being viewed from,
I think?)

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:123: 'permissions_':
self._GetPermissions(),
'permissions' is already reserved. There's a note about this in
intro_tables.html as well.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron_servlet_test.py:50:
ConfigureFakeFetchers()
*Should this need to be here? It prevents errors when fetching json files in
AvailabilityDataSource.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/instance_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/instance_servlet_test.py:9: from
fake_fetchers import ConfigureFakeFetchers
oops, I had a ConfigureFakeFetchers() call in setUp earlier..

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/local_renderer.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/local_renderer.py:21: assert not '\\' in
path
This should probably just be put in its own patch so it doesn't keep getting
held up... (unless it /has/ already been patched)

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/render_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/render_servlet_test.py:21:
ConfigureFakeFetchers()
Another ConfigureFakeFetchers call.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/api_availabilities.json
(right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/api_availabilities.json:7:
"webstore": "15"
These APIs are here because they are strange.

devtools APIs are in a subdirectory, which is unique. This may need to be
handled programatically though since there are other experimental devtools that
will probably be released.

input.ime's name is treated differently from other APIs (I think its name is
unix_style instead of camelCase like the rest of the APIs)

webstore previously had a manual API of 15, so I kept it. But, I think only
"private_webstore" is available that early in the extension_api.json files.

notifications has a special note about actually being on dev, although it is
listed as being stable so that a few of its methods can be used on stable (I
mentioned this note in a comment on patch set 10). The problem with this is that
it will need to be manually removed from this file when it goes stable.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/json/intro_tables.json (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/templates/json/intro_tables.json:97: "suffix": "
permission"
Here's an example of a fully fleshed-out permission.

[not at google - send to devlin, 2013-05-13 15:43:32]

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:53: '%s/%s' %
(svn_constants.API_PATH, MANIFEST_FEATURES))
here is one problem. The file systems are being read, but they might be offline.
All data needs to originate from the object stores (compiled file systems solve
this problem automatically by the way).

[not at google - send to devlin, 2013-05-13 21:26:41]

Plenty to go on with!

I'll be online as much as possible to get this through :)

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:95:
'%s/intro_tables.json' % svn_constants.JSON_PATH))
I don't think *this* should be trunk though should it? Just the ones that go
into the availability data source?

The permissions/intro is whatever the host file system says it is?

Also since these are shared across multiple object you should probably cache it
- therefore, try using the compiled_fs with a populate_fn that does this
json_parse stuff. It might even be enough to do

_json_cache = compiled_fs_factory.Create(json_parse.Parse, APIDataSource)

?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:123: 'permissions_':
self._GetPermissions(),
On 2013/05/13 02:38:10, epeterson wrote:
> 'permissions' is already reserved. There's a note about this in
> intro_tables.html as well.

Ohh. Why is it reserved?? Bummer, ok, well permissions_ is pretty confusing
then. Maybe api_permissions?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:154: def
_GetMoreLearning(self):
that is an amusing name :) I don't think that pluralising Lean More is More
Learning. Whatevs.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:18: class
AvailabilityDataSource(object):
Late stage for this, but this isn't actually a *DataSource* which by convention
implies an object which is exposed to templates. That has been confusing me
somewhat. Let's call it AvailabilityFinder.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:27:
create_file_system):
Can we try making this a method of ChromeVersionUtility? To create a host file
system for a given version?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:53: '%s/%s' %
(svn_constants.API_PATH, MANIFEST_FEATURES))
On 2013/05/13 15:43:32, kalman wrote:
> here is one problem. The file systems are being read, but they might be
offline.
> All data needs to originate from the object stores (compiled file systems
solve
> this problem automatically by the way).

Continuing from this comment, and reading below: I think you can get away
without injecting any file systems into here: (a) move the getting of a file
system into ChromeVersionUtility, and (b) use a CompiledFileSystem that calls
json_parse.Parse for all of these. You'll need to inject the paths though.

def __init__(self,
            chrome_version_util,
            object_store_creator,
            compiled_fs_factory,
            api_availabilities_path,
            permission_features_path,
            manifest_features_path,
            extension_api_path):
  self._json_cache = compiled_fs_factory.Create(json_parse.Parse,
AvailabilityDataSource)
  self._ ....

...

then instead of

self._permisison_features.get(api_name)

do

self._GetPermissionFeature(api_name)

with

def _GetPermissionFeature(self, api_name):
  return self._json_cache.GetFromFile(
      self._permission_features_path).get(api_name)

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:65:
self._orphan_apis = []
Looking at the way this is used, it's a bit strange to be holding onto this data
like that, but I hope they'll go away when you address the comments in this
file.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:70: def
_GetChannelFromPermissionFeatures(self, api_name, permission_features):
this function should be able to use self._permission_features, not take it as a
parameter?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:82: for entry
in api_info])
I don't think that the list comprehension is necessary, just passing the
generator in should work ok.

channel = BranchUtility.NewestChannel(entry.get('channel') for entry in
api_info)

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:125: existence
= False
existence = api_name in manifest_features

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:144: names =
svn_file_system.Read([svn_constants.API_PATH + '/']).Get()
ReadSingle

search in this file for ".Read(" and see if they can be ReadSIngle.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:155: if
int(version) > 17 or int(version) < 5:
make these assert, this method should only be called under the right
circumstances.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:159: existence
= self._names_object_store.Get('%s%s' % (api_name, version)).Get()
I don't think caching needs to be done here, you're already caching the result
of GetAvailability. It just complicates things?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:196: break
THis method is super cool, but a bit odd to read all of the extra while
conditions. Just make it so that every time you find the version return it, then
invert the conditions to do so, then not have all of the < X checks.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:211: if
self._CheckExtensionAPIExistence(api_name, str(version)):
why str(version)? Can we use ints everywhere?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:217: def
GetAvailability(self, api_name):
I will want to add event/function/property availability as well, so perhaps call
this GetApiAvailability.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:227:
availability = self._existing_availabilities.get(api_name)
e.g. in lazy creation world this would be
self._GetExistingAvailability(api_name)

and _GetExistingAvailability would know how to call ReadFromFile etc. Then
CompiledFileSystem implements the caching that you would otherwise need to
implement yourself.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:234:
self._permission_features)
This won't actually work properly, though: say if an API has just been promoted
from dev to beta, like, 10 minutes ago. This method will return 'beta', however,
in the actual beta channel it's still dev restricted, so won't work.

I think you need to step through all channels, starting from stable, until you
reach a point where it's available:

branch_util = self._branch_util
version_util = self._chrome_version_util

availability = ('trunk', version_util.GetLatestVersionNumber())
for channel in BranchUtility.GetChannels():
  if self._IsAvailable(api_name, self._GetFileSystemForChannel(channel)):
    availability = (channel, branch_util.GetChannelInfo(channel).version)
    break
if availability[0] == 'stable':
  availability[1] = self._FindEarliestAvailability(api, availability[1])
self._object_store.Set(api_name, availability)
return availability

then self._IsAvailable does what you're doing here pretty much; check
permission, manifest, then file system. I suppose it would end up looking very
similar to _FindEarliestAvailability - that does all the right things, except it
only works for stable - it needs to work for all the channels.

In the above sample I actually made availability be a (channel, version) tuple,
where channel is obvious, and version is the Chrome version that it first
appeared in that channel (only really useful for stable - but let callers decide
that). I think this would be clearer. It would also avoid the conflation of
channel and version that this method currently suffers from.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source_test.py
(right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source_test.py:43:
self._avail_ds._CheckFileSystemExistence('tabs', '20'))
The way that GetAvailability is implemented is irrelevant - e.g. don't test the
private methods. Just test the single public method, but choose test cases that
would need to trigger each code path (e.g. commands API for manifest, bluetooth
for permissions, etc etc).

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source_test.py:104:
self.assertEquals('10',
see comment in that other file. If GetAvailability returned a (channel, version)
tuple it'd be less magical and easier for the caller to intepret.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:22: return ['stable',
'beta', 'dev', 'trunk']
cleanup: use a tuple here not a list. my bad from a long time ago.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:38: for channel in
['trunk', 'dev', 'beta', 'stable']:
for channel in reversed(BranchUtility.GetAllChannelNames()): ?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:112:
self._object_store.Set('%s.%s' % (channel_name, data_type),
use two separate object stores with different categories (branch vs version)

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chrome_version_utility.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:9: class
ChromeVersionUtility(object):
This is so similar to BranchUtility, and BranchUtility even has knowledge of
versions. I'm not really sure where one stops and the other starts. We should
think about combining them into a single file like ChromeHistoryUtility or
something, but, that can wait until later. A follow up CL. Let's do the minimum
amount of cleanup necessary to get this patch in.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:39: return None
let's just let this exception fall all the way through.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:47: return None
throw a ValueError or something

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:49: def
GetLatestVersionNumber(self):
need to object-storify this or it won't work offline

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:51: version_json
= json.loads(self._fetcher.Fetch(self._base_path).content)
might want to lazily populate the version json beacuse you're reading it
multiple times (here and above). only need in-memory because the real caching
happens within the object store, and only the actual stuff we need to cache in
object stores is the results of functions.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:55: return None
ditto fall through

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:69: return
json.load(f)
delete _GetFakeData and the places where it's commented out

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron_servlet_test.py:50:
ConfigureFakeFetchers()
On 2013/05/13 02:38:10, epeterson wrote:
> *Should this need to be here? It prevents errors when fetching json files in
> AvailabilityDataSource.

no it shouldn't be here. The only thing allowed to call this is things that
can't be tested in isolation (e.g. integration test), and this can be tested in
isolatino.

added comment there.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/intro_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/intro_data_source_test.py:20:
ConfigureFakeFetchers()
shouldnt' need this.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/local_renderer.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/local_renderer.py:21: assert not '\\' in
path
On 2013/05/13 02:38:10, epeterson wrote:
> This should probably just be put in its own patch so it doesn't keep getting
> held up... (unless it /has/ already been patched)

yeah it's been fixed now, you can revert it

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/render_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/render_servlet_test.py:21:
ConfigureFakeFetchers()
On 2013/05/13 02:38:10, epeterson wrote:
> Another ConfigureFakeFetchers call.

again - shouldn't be necessary

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:76:
file_system_creator)
I don't think you need to pass it in here

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:152:
object_store_creator = ObjectStoreCreator(branch,
why do you need a different object store creator?just reuse the one that's
constructed above?

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:159:
CachingFileSystem(LocalFileSystem.Create(), object_store_creator),
for that matter, use the above function to construct this I suppose

[epeterson, 2013-05-15 07:38:33]

Alright, tried to finish most the requests. Logic for the creation of file
systems in AvailabilityFixer still needs to be moved into ChromeVersionUtility. 

Algorithm with the correct functionality in GetAvailability isn't quite there
yet, unfortunately.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:95:
'%s/intro_tables.json' % svn_constants.JSON_PATH))
On 2013/05/13 21:26:41, kalman wrote:
> I don't think *this* should be trunk though should it? Just the ones that go
> into the availability data source?
> 
> The permissions/intro is whatever the host file system says it is?
> 
> Also since these are shared across multiple object you should probably cache
it
> - therefore, try using the compiled_fs with a populate_fn that does this
> json_parse stuff. It might even be enough to do
> 
> _json_cache = compiled_fs_factory.Create(json_parse.Parse, APIDataSource)
> 
> ?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:123: 'permissions_':
self._GetPermissions(),
On 2013/05/13 21:26:41, kalman wrote:
> On 2013/05/13 02:38:10, epeterson wrote:
> > 'permissions' is already reserved. There's a note about this in
> > intro_tables.html as well.
> 
> Ohh. Why is it reserved?? Bummer, ok, well permissions_ is pretty confusing
> then. Maybe api_permissions?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_data_source.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:18: class
AvailabilityDataSource(object):
On 2013/05/13 21:26:41, kalman wrote:
> Late stage for this, but this isn't actually a *DataSource* which by
convention
> implies an object which is exposed to templates. That has been confusing me
> somewhat. Let's call it AvailabilityFinder.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:70: def
_GetChannelFromPermissionFeatures(self, api_name, permission_features):
On 2013/05/13 21:26:41, kalman wrote:
> this function should be able to use self._permission_features, not take it as
a
> parameter?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:82: for entry
in api_info])
On 2013/05/13 21:26:41, kalman wrote:
> I don't think that the list comprehension is necessary, just passing the
> generator in should work ok.
> 
> channel = BranchUtility.NewestChannel(entry.get('channel') for entry in
> api_info)

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:125: existence
= False
On 2013/05/13 21:26:41, kalman wrote:
> existence = api_name in manifest_features

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:144: names =
svn_file_system.Read([svn_constants.API_PATH + '/']).Get()
On 2013/05/13 21:26:41, kalman wrote:
> ReadSingle
> 
> search in this file for ".Read(" and see if they can be ReadSIngle.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:155: if
int(version) > 17 or int(version) < 5:
On 2013/05/13 21:26:41, kalman wrote:
> make these assert, this method should only be called under the right
> circumstances.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:159: existence
= self._names_object_store.Get('%s%s' % (api_name, version)).Get()
On 2013/05/13 21:26:41, kalman wrote:
> I don't think caching needs to be done here, you're already caching the result
> of GetAvailability. It just complicates things?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:196: break
On 2013/05/13 21:26:41, kalman wrote:
> THis method is super cool, but a bit odd to read all of the extra while
> conditions. Just make it so that every time you find the version return it,
then
> invert the conditions to do so, then not have all of the < X checks.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:211: if
self._CheckExtensionAPIExistence(api_name, str(version)):
On 2013/05/13 21:26:41, kalman wrote:
> why str(version)? Can we use ints everywhere?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:217: def
GetAvailability(self, api_name):
On 2013/05/13 21:26:41, kalman wrote:
> I will want to add event/function/property availability as well, so perhaps
call
> this GetApiAvailability.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:227:
availability = self._existing_availabilities.get(api_name)
On 2013/05/13 21:26:41, kalman wrote:
> e.g. in lazy creation world this would be
> self._GetExistingAvailability(api_name)
> 
> and _GetExistingAvailability would know how to call ReadFromFile etc. Then
> CompiledFileSystem implements the caching that you would otherwise need to
> implement yourself.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_data_source.py:234:
self._permission_features)
On 2013/05/13 21:26:41, kalman wrote:
> This won't actually work properly, though: say if an API has just been
promoted
> from dev to beta, like, 10 minutes ago. This method will return 'beta',
however,
> in the actual beta channel it's still dev restricted, so won't work.
> 
> I think you need to step through all channels, starting from stable, until you
> reach a point where it's available:
> 
> branch_util = self._branch_util
> version_util = self._chrome_version_util
> 
> availability = ('trunk', version_util.GetLatestVersionNumber())
> for channel in BranchUtility.GetChannels():
>   if self._IsAvailable(api_name, self._GetFileSystemForChannel(channel)):
>     availability = (channel, branch_util.GetChannelInfo(channel).version)
>     break
> if availability[0] == 'stable':
>   availability[1] = self._FindEarliestAvailability(api, availability[1])
> self._object_store.Set(api_name, availability)
> return availability
> 
> then self._IsAvailable does what you're doing here pretty much; check
> permission, manifest, then file system. I suppose it would end up looking very
> similar to _FindEarliestAvailability - that does all the right things, except
it
> only works for stable - it needs to work for all the channels.
> 
> In the above sample I actually made availability be a (channel, version)
tuple,
> where channel is obvious, and version is the Chrome version that it first
> appeared in that channel (only really useful for stable - but let callers
decide
> that). I think this would be clearer. It would also avoid the conflation of
> channel and version that this method currently suffers from.

So this is a bit of a mess now. Couldn't quite get it figured out tonight.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:22: return ['stable',
'beta', 'dev', 'trunk']
On 2013/05/13 21:26:41, kalman wrote:
> cleanup: use a tuple here not a list. my bad from a long time ago.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:38: for channel in
['trunk', 'dev', 'beta', 'stable']:
On 2013/05/13 21:26:41, kalman wrote:
> for channel in reversed(BranchUtility.GetAllChannelNames()): ?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:58: if first in
['trunk', 'dev', 'beta', 'stable']:
Changed this guy, too.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/chrome_version_utility.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:39: return None
On 2013/05/13 21:26:41, kalman wrote:
> let's just let this exception fall all the way through.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:47: return None
On 2013/05/13 21:26:41, kalman wrote:
> throw a ValueError or something

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:55: return None
On 2013/05/13 21:26:41, kalman wrote:
> ditto fall through

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/chrome_version_utility.py:69: return
json.load(f)
On 2013/05/13 21:26:41, kalman wrote:
> delete _GetFakeData and the places where it's commented out

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron_servlet_test.py:50:
ConfigureFakeFetchers()
On 2013/05/13 21:26:41, kalman wrote:
> On 2013/05/13 02:38:10, epeterson wrote:
> > *Should this need to be here? It prevents errors when fetching json files in
> > AvailabilityDataSource.
> 
> no it shouldn't be here. The only thing allowed to call this is things that
> can't be tested in isolation (e.g. integration test), and this can be tested
in
> isolatino.
> 
> added comment there.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/intro_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/intro_data_source_test.py:20:
ConfigureFakeFetchers()
On 2013/05/13 21:26:41, kalman wrote:
> shouldnt' need this.

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/render_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/render_servlet_test.py:21:
ConfigureFakeFetchers()
On 2013/05/13 21:26:41, kalman wrote:
> On 2013/05/13 02:38:10, epeterson wrote:
> > Another ConfigureFakeFetchers call.
> 
> again - shouldn't be necessary

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:152:
object_store_creator = ObjectStoreCreator(branch,
On 2013/05/13 21:26:41, kalman wrote:
> why do you need a different object store creator?just reuse the one that's
> constructed above?

Done.

https://codereview.chromium.org/12996003/diff/253001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:159:
CachingFileSystem(LocalFileSystem.Create(), object_store_creator),
On 2013/05/13 21:26:41, kalman wrote:
> for that matter, use the above function to construct this I suppose

Done.

[not at google - send to devlin, 2013-05-15 08:15:00]

just comments in availability finder and a couple in branch utility. please take
care to go over all of them, I repeated myself a few times in this round.

I also haven't gone over the availability test but will in more detail when this
stuff is stable. aim for 100% code coverage.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:18:
PERMISSION_FEATURES = '_permission_features.json'
these are all private so should start with a _, and maybe you should join them
with their prefixes (JSON_PATH, API_PATH, whatever) up-front to make that code
smaller + do less work.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:39: class
AvailabilityInfo(object):
this object is picklable, so it can be directly put into the object stores. you
don't need to convert it to and from tuples yourself.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:68:
self._json_cache = compiled_host_fs_factory.Create(self._ParseForCompiledFS,
just pass in json_parse.Parse, no need to wrap it in another function

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:74:
self._permission_apis = []
as I mentioned in IRC - holding onto these
(permission_apis,manifest_apis,orphan_apis) probably doesn't gain much since
you'll be caching the result of GetApiAvailability anyway, and will only end up
reading them once in the common case.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:84: #
TODO(epeterson) Move create_file_system logic into ChromeVersionUtility.
don't worry about it

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:88: return
compiled_fs_factory.Create(self._ParseForCompiledFS,
ditto

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:97: return None
as I mentioned, AvailabilityFinder is picklable, so this can just return it
directly. But since this function is only called in once place, simpler to
inline it.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:111: def
_ReadJson(self, svn_file_system, path):
not used

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:113: 
extra \n

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:132: 
extra \n

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:134: def
_CheckSimplePermissionExistence(self, api_name, version):
looks like this method can be combined with checkstablepermissionexistence, if
they were to take a file system.

And call it _ExistsInPermissionFeatures(api_name, file_system)

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:140: def
_CheckManifestExistence(self, api_name, version):
_ExistsInManifestFeatures, take a file system

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:158: def
_CheckFileSystemExistence(self, api_name, version):
_ExistsInFileSystem, take a file system

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:162: names =
self._object_store.Get(str(version)).Get()
shouldn't need to stringify version here

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:165:
svn_file_system = self._create_file_system(
use the compiled file system for this instead?

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:214: return version
+ 1
pls see earlier comments about simplifying the return statements in this
function

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:232: def
_IsAvailable(self, api_name, version):
yes, except pass this an api and a file system, not an api and a version. that
would require you to write CheckSimplePermissionExistence etc to take a file
system.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:253: return
self.AvailabilityInfo(availability[0], availability[1])
this block of code seems to be doing the same as the block before it.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:257: #
TODO(epeterson) This shouldn't be done upfront
yeah I think this block of code can basically be deleted.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:285: break
this all looks very complicated?

I think it can be:

availability = None
for channel_info in self._branch_utility.GetAllChannelInfo():
  if self._IsAvailableInFileSystem(
      api_name, self._create_file_system(channel_info.branch)):
    availability = AvailabilityFinder.AvailabilityInfo(channel_info.channel,
                                                       channel_info.version)
    break

# The API should at least be available on trunk. It's a bug otherwise.
assert availability, 'Did not find any availability for %s' % channel

# If the API is in stable, find the chrome version which it became stable in.
if availability.channel == 'stable':
  availability.version = self._FindEarliestAvailability(api_name,
                                                        availability.version)

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:25: return [(branch,
self.GetChannelInfoForChannelName(branch).branch)
This GetChannelInfoForChannelName doesn't actually exist. Need tests for this
file...

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:26: for branch in
BranchUtility.GetAllChannelNames()]
channel in

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:30: for branch in
self.GetAllChannelNames()]
channel in

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:32: def
GetChannelInfoForAllChannels(self):
can be just GetAllChannelInfo, shorter.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:34: for branch in
self.GetAllChannelNames()]
channel in

[not at google - send to devlin, 2013-05-16 18:56:15]

pls remove most of the references to ConfigureFakeFetchers and see how
FakeBranchUtility is used.  similar thing needed for ChromeVersionUtility.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron_servlet_test.py:50: #
ChromeVersionUtility attempts to fetch data from omaha proxy
then make it not, use a fake chrome version utility.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/preview.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/preview.py:73: ConfigureFakeFetchers()
shouldn't be needed. make LocalRenderer use a fake chrome vesion utilty. it's
fake data anyway, and this means we won't catch some bad behaviour elsewhere.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/render_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/render_servlet_test.py:22:
ConfigureFakeFetchers()
likewise

[epeterson, 2013-06-02 00:25:49]

Hey there,

Here's bigger and (hopefully) better CL for you to look at.
It's up-to-date with master at this point in time. 

Test are passing without any unnecessary ConfigureFakeFetchers() calls. Haven't
been able to test with the app engine yet, as the cron fails due to
api_availabilities.json and intro_tables.json not being in SVN yet.

But, at the very least, I thought you might want to see the progress that's been
made since I/O week.


Thank you for your time,

Evan

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:18:
PERMISSION_FEATURES = '_permission_features.json'
On 2013/05/15 08:15:00, kalman wrote:
> these are all private so should start with a _, and maybe you should join them
> with their prefixes (JSON_PATH, API_PATH, whatever) up-front to make that code
> smaller + do less work.

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:39: class
AvailabilityInfo(object):
On 2013/05/15 08:15:00, kalman wrote:
> this object is picklable, so it can be directly put into the object stores.
you
> don't need to convert it to and from tuples yourself.

Done. (I had tried this initially, but I think it must not've worked due to
being an inner class)

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:68:
self._json_cache = compiled_host_fs_factory.Create(self._ParseForCompiledFS,
On 2013/05/15 08:15:00, kalman wrote:
> just pass in json_parse.Parse, no need to wrap it in another function

compiled filesystems send a path and file data to their populate function, but
json_parse.Parse only accepts one paramater, a json string. So, using lambdas
here to compensate.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:74:
self._permission_apis = []
On 2013/05/15 08:15:00, kalman wrote:
> as I mentioned in IRC - holding onto these
> (permission_apis,manifest_apis,orphan_apis) probably doesn't gain much since
> you'll be caching the result of GetApiAvailability anyway, and will only end
up
> reading them once in the common case.

The intent was to use these to force APIs that were, for instance, found in the
_manifest_features.json file in trunk  after falling back from
_permission_features, to only be checked in _manifest_features for the other
versions.
APIs can shift around between the *_features.json files, so I thought that
needed to be restricted. 

Based on your comments on IRC, however, it sounds like it's fine if the APIs
move around between _features files, so they don't need to be divided up into
subsets like this.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:88: return
compiled_fs_factory.Create(self._ParseForCompiledFS,
On 2013/05/15 08:15:00, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:97: return None
On 2013/05/15 08:15:00, kalman wrote:
> as I mentioned, AvailabilityFinder is picklable, so this can just return it
> directly. But since this function is only called in once place, simpler to
> inline it.

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:111: def
_ReadJson(self, svn_file_system, path):
On 2013/05/15 08:15:00, kalman wrote:
> not used

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:113: 
On 2013/05/15 08:15:00, kalman wrote:
> extra \n

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:132: 
On 2013/05/15 08:15:00, kalman wrote:
> extra \n

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:134: def
_CheckSimplePermissionExistence(self, api_name, version):
On 2013/05/15 08:15:00, kalman wrote:
> looks like this method can be combined with checkstablepermissionexistence, if
> they were to take a file system.
> 
> And call it _ExistsInPermissionFeatures(api_name, file_system)

Logic has been moved up above.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:140: def
_CheckManifestExistence(self, api_name, version):
On 2013/05/15 08:15:00, kalman wrote:
> _ExistsInManifestFeatures, take a file system

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:158: def
_CheckFileSystemExistence(self, api_name, version):
On 2013/05/15 08:15:00, kalman wrote:
> _ExistsInFileSystem, take a file system

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:162: names =
self._object_store.Get(str(version)).Get()
On 2013/05/15 08:15:00, kalman wrote:
> shouldn't need to stringify version here

This has been removed completely. AvailabilityFinder's object store is only used
in GetApiAvailability.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:165:
svn_file_system = self._create_file_system(
On 2013/05/15 08:15:00, kalman wrote:
> use the compiled file system for this instead?

Done, although the logic's been moved into a function up there ^^

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:232: def
_IsAvailable(self, api_name, version):
On 2013/05/15 08:15:00, kalman wrote:
> yes, except pass this an api and a file system, not an api and a version. that
> would require you to write CheckSimplePermissionExistence etc to take a file
> system.

The functions for checking existence now take a file system. Since checking
_features files as well as file system existence requires different behavior,
two compiled file systems with unique populate functions are needed. I thought
it looked cleaner to create these compiled fs's here, instead of in the loop in
GetApiAvailability and passing them in, although this can be changed.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:253: return
self.AvailabilityInfo(availability[0], availability[1])
On 2013/05/15 08:15:00, kalman wrote:
> this block of code seems to be doing the same as the block before it.

The previous block is checking the object_store, while this block is checking
the api_availabilities.json file. I think this is a little more obvious with the
current code.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:257: #
TODO(epeterson) This shouldn't be done upfront
On 2013/05/15 08:15:00, kalman wrote:
> yeah I think this block of code can basically be deleted.

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:285: break
On 2013/05/15 08:15:00, kalman wrote:
> this all looks very complicated?
> 
> I think it can be:
> 
> availability = None
> for channel_info in self._branch_utility.GetAllChannelInfo():
>   if self._IsAvailableInFileSystem(
>       api_name, self._create_file_system(channel_info.branch)):
>     availability = AvailabilityFinder.AvailabilityInfo(channel_info.channel,
>                                                        channel_info.version)
>     break
> 
> # The API should at least be available on trunk. It's a bug otherwise.
> assert availability, 'Did not find any availability for %s' % channel
> 
> # If the API is in stable, find the chrome version which it became stable in.
> if availability.channel == 'stable':
>   availability.version = self._FindEarliestAvailability(api_name,
>                                                         availability.version)

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:25: return [(branch,
self.GetChannelInfoForChannelName(branch).branch)
On 2013/05/15 08:15:00, kalman wrote:
> This GetChannelInfoForChannelName doesn't actually exist. Need tests for this
> file...

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:26: for branch in
BranchUtility.GetAllChannelNames()]
On 2013/05/15 08:15:00, kalman wrote:
> channel in

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:30: for branch in
self.GetAllChannelNames()]
On 2013/05/15 08:15:00, kalman wrote:
> channel in

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:32: def
GetChannelInfoForAllChannels(self):
On 2013/05/15 08:15:00, kalman wrote:
> can be just GetAllChannelInfo, shorter.

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:34: for branch in
self.GetAllChannelNames()]
On 2013/05/15 08:15:00, kalman wrote:
> channel in

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron_servlet_test.py:50: #
ChromeVersionUtility attempts to fetch data from omaha proxy
On 2013/05/16 18:56:15, kalman wrote:
> then make it not, use a fake chrome version utility.

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/preview.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/preview.py:73: ConfigureFakeFetchers()
On 2013/05/16 18:56:15, kalman wrote:
> shouldn't be needed. make LocalRenderer use a fake chrome vesion utilty. it's
> fake data anyway, and this means we won't catch some bad behaviour elsewhere.

Done.

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/render_servlet_test.py (right):

https://codereview.chromium.org/12996003/diff/293001/chrome/common/extensions...
chrome/common/extensions/docs/server2/render_servlet_test.py:22:
ConfigureFakeFetchers()
On 2013/05/16 18:56:15, kalman wrote:
> likewise

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:34: 
A lot of the logic for checking individual files/file existence was moved out of
the AvailabilityFinder class. I also tried to make this generic enough to allow
for the possibility of additional, similar _features files being added in the
future (like _api_features.json)

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:123: def
_CreateFeaturesAndNamesFileSystems(self, version):
Checking _features.json files requires a different populate function for a
compiled fs than checking a list of file names (checking for file existence).
These file systems are created and returned together, though, since they're
always used in the same places.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:150: '''
I think this code looks a lot more readable, and simplifies the return logic.
It's recursive, though, and that might be taboo..

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:21: self._history_result
= self._fetcher.FetchAsync(history_url)
Save a future with the fetch result, and call .Get() on it later. Is this what
you had in mind by 'lazily populating' this data?

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:87: data =
self._version_object_store.Get(channel_name).Get()
Using two separate object_store with different categories.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:128: def
GetBranchNumberForVersion(self, version):
ChromeVersionUtility only had two methods left, and they've now been merged into
BranchUtility.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/branch_utility.py:152: latest_version =
self._version_object_store.Get('latest').Get()
Using an object_store here now, this was brought up in an older patchset.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron_servlet.py:249: branch_utility,
Passing a branch utility to AvailabilityFinder via ServerInstance. Since these
are created within delegate classes the behavior can be changed when using
offline/online file systems.

We might be able to incorporate the create_file_system method logic into
branch_utility.

[not at google - send to devlin, 2013-06-05 00:24:08]

I didn't look over this change 100% because it's gotten a bit too overwhelming
for me.

I'd like you to start pulling parts out of it and submit them separately to
reduce the diff. Here are some things you could pull out:

1. the little import and spelling fixes you've made
2. the test changes you made that weren't much related (I think there were some
of these?)
3. the branch utility changes + tests
4. the addition of AvailabilityFinder + tests
5. the hooking up of AvailbilityFinder into the Intro and API data sources.

I can then review each in isolation and we can start submitting some code in a
way that's manageable. Hopefully I'll find myself making fewer nitty comments as
we go on too.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:413: self._intro_cache
= create_compiled_fs(compiled_fs_parse, 'intro-cache')
ah right. yeah, using a lambda here would be better; lambda _ json:
json_parse.Parse(json)

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:551: else:
no else after a return statement

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:579: cache, ext =
self._DetermineCacheForGet(unix_name)
any particular reason why you split this out?

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:62:
'basic_docs_filesystem.json'))),
define this in python somewhere, not a JSON file, if you really need to share
it.

perhaps canned_test_file_system_data.py.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:154:
self._LoadJSON('api_learn_more_expected.json'))
please just get this data from the canned file system data directly rather than
duplicating it in a separate file.

or if that's not possible, hard code it here. moving the data into JSON files
obfuscates it.

same goes for the other things too.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_list_data_source.py (left):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:5: import logging
please revert all these import-only changes and submit them separately - perhaps
first, if you'd like. it would make the daunting number of files changed here
less daunting.

in fact if you can think of anything else to pull out and submit separately that
might make this go faster.

e.g. your json schema compiler changes would also be a good candidate for a
separate change, and the test cleanups.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:22: '''Handles
finding API channel information from _features.json files.
"Finds API channel information..."

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:33: return channel
more concise:

if feature is None:
  return None
if isinstance(feature, collections.Mapping):
  return feature.get('channel')
# Features can contain a list of entries. Take the newest branch.
return BranchUtility.NewestChannel(entry.get('channel')
                                   for entry in feature)

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:35: def
_GetFeature(api_name, file_system, path, unix_name=False):
this is only used in ExistsInFeatures, move it in there.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:40: api_name =
model.UnixName(api_name)
what is this unix_name thing?

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:43: def
_ExistsInFeatures(api_name, file_system, path, unix_name=False):
as per comment way below, call all of these methods like _GetChannelFromFeatures
and return the channel from all of them (or None).

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:56:
logging.warning('_api_features.json not found')
don't warn it'll pollute the logs - but good point.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:62: def
_ExistsInManifestFeatures(api_name, file_system):
_GetChannelFromManifestFeatures

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:97:
create_file_system):
please document create_file_system. create_file_system_at_version might be a
better name if that's what it does.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:129: fs_factory =
CompiledFileSystem.Factory(
recreating them each time like this is inefficent because we use in-memory
caches which will need to be repopulated each time.

but adding @memoize to this function will do it (from
third_party.json_schema_compiler)

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:134: '%sFeatures' %
version)
category=

also you don't need the version since the compiled file system is constructed
with it. just 'features'.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:137: '%sNames' %
version)
likewise category='names'

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:150: '''
On 2013/06/02 00:25:50, epeterson wrote:
> I think this code looks a lot more readable, and simplifies the return logic.
> It's recursive, though, and that might be taboo..

Yes much better, though recursiveness should be avoided if it's just as
reasonable to do it iteratively. Which, here it is. I'm pretty sure the
algorithm I wrote for you would have more or less worked fine?

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:152: features_fs,
names_fs = self._CreateFeaturesAndNamesFileSystems(version)
just have an "if version < 5: return version" at the top.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:159: # existence if
the API is not stable in any of the _features.json files.
could you put all of these comments inside the blocks that guard them? it would
be easier to read.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:185: def
_IsAvailableInFileSystem(self, api_name, version):
as noted below, make this _GetAvailableChannelForVersion

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:208: return
availability
more concise:

availability = AvailabilityInfo(api_info['channel'], api_info['version'])
self._object_store.Set(api_name, availability)
return availability

in fact there isn't much point caching it since json is cached anyway and this
stuff is basically free to look up. so this can all be just

if api_info is not None:
  return AvailabilityInfo(...)

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:217: break
i think this has a bug in scenarios such as: an API is present in the stable
build but only available to dev channel. This algorithm will report stable, but
it's really only dev.

Try:

for channel_info in self._branch_utility.GetAllChannelInfo():
  available_channel = self._GetAvailableChannelForVersion(
      channel_info.version)
  if (available_channel is not None and
      BranchUtility.NewestVersion((available_channel,
                                   channel_info.channel))
          == available_channel):
    availability = AvailabilityInfo(channel_info.channel,
                                    channel_info.version)
    break

and please test this!

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:232: class
AvailabilityInfo(object):
declare at top

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder_test.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder_test.py:27: class
AvailabilityInfo(object):
this isn't used anywhere -

generally speaking please go over your code before you send it out to clean up
things like this

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder_test.py:44:
TestFileSystem(_LoadJSON('test_avail_filesystem.json')['trunk']),
put this file system in python not JSON.

[epeterson, 2013-06-17 20:05:49]

The patch is currently being broken up into smaller patches, but I don't want to
lose track of your most recent batch of comments in the process.

Feel free to look over the changes here, but they will also be in the smaller
patches as those are uploaded.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:413: self._intro_cache
= create_compiled_fs(compiled_fs_parse, 'intro-cache')
On 2013/06/05 00:24:09, kalman wrote:
> ah right. yeah, using a lambda here would be better; lambda _ json:
> json_parse.Parse(json)

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:551: else:
On 2013/06/05 00:24:09, kalman wrote:
> no else after a return statement

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:579: cache, ext =
self._DetermineCacheForGet(unix_name)
On 2013/06/05 00:24:09, kalman wrote:
> any particular reason why you split this out?

When I was using that old "get2" method here, the code was shared between get
and get2, so I moved it out. But I'll move it back in now.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:62:
'basic_docs_filesystem.json'))),
On 2013/06/05 00:24:09, kalman wrote:
> define this in python somewhere, not a JSON file, if you really need to share
> it.
> 
> perhaps canned_test_file_system_data.py.

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:154:
self._LoadJSON('api_learn_more_expected.json'))
On 2013/06/05 00:24:09, kalman wrote:
> please just get this data from the canned file system data directly rather
than
> duplicating it in a separate file.
> 
> or if that's not possible, hard code it here. moving the data into JSON files
> obfuscates it.
> 
> same goes for the other things too.

Done, for the tests I added, at least. Should I go back through the rest of the
tests in the file and either inline or pythonify the JSON that's used as
expected tests results/test file system? Perhaps as a separate patch?

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:22: '''Handles
finding API channel information from _features.json files.
On 2013/06/05 00:24:09, kalman wrote:
> "Finds API channel information..."

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:33: return channel
On 2013/06/05 00:24:09, kalman wrote:
> more concise:
> 
> if feature is None:
>   return None
> if isinstance(feature, collections.Mapping):
>   return feature.get('channel')
> # Features can contain a list of entries. Take the newest branch.
> return BranchUtility.NewestChannel(entry.get('channel')
>                                    for entry in feature)

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:35: def
_GetFeature(api_name, file_system, path, unix_name=False):
On 2013/06/05 00:24:09, kalman wrote:
> this is only used in ExistsInFeatures, move it in there.

Done. It might be useful to access the rest of the feature information besides
just 'channel' in the future, though.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:40: api_name =
model.UnixName(api_name)
On 2013/06/05 00:24:09, kalman wrote:
> what is this unix_name thing?

_manifest_features.json uses unix_style API names, so the name needs to be
converted.
I'll document this.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:43: def
_ExistsInFeatures(api_name, file_system, path, unix_name=False):
On 2013/06/05 00:24:09, kalman wrote:
> as per comment way below, call all of these methods like
_GetChannelFromFeatures
> and return the channel from all of them (or None).

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:56:
logging.warning('_api_features.json not found')
On 2013/06/05 00:24:09, kalman wrote:
> don't warn it'll pollute the logs - but good point.

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:62: def
_ExistsInManifestFeatures(api_name, file_system):
On 2013/06/05 00:24:09, kalman wrote:
> _GetChannelFromManifestFeatures

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:97:
create_file_system):
On 2013/06/05 00:24:09, kalman wrote:
> please document create_file_system. create_file_system_at_version might be a
> better name if that's what it does.

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:129: fs_factory =
CompiledFileSystem.Factory(
On 2013/06/05 00:24:09, kalman wrote:
> recreating them each time like this is inefficent because we use in-memory
> caches which will need to be repopulated each time.
> 
> but adding @memoize to this function will do it (from
> third_party.json_schema_compiler)

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:134: '%sFeatures' %
version)
On 2013/06/05 00:24:09, kalman wrote:
> category=
> 
> also you don't need the version since the compiled file system is constructed
> with it. just 'features'.

Ok, this clears things up. thank you.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:137: '%sNames' %
version)
On 2013/06/05 00:24:09, kalman wrote:
> likewise category='names'

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:150: '''
On 2013/06/05 00:24:09, kalman wrote:
> On 2013/06/02 00:25:50, epeterson wrote:
> > I think this code looks a lot more readable, and simplifies the return
logic.
> > It's recursive, though, and that might be taboo..
> 
> Yes much better, though recursiveness should be avoided if it's just as
> reasonable to do it iteratively. Which, here it is. I'm pretty sure the
> algorithm I wrote for you would have more or less worked fine?

Here's an iterative version.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:152: features_fs,
names_fs = self._CreateFeaturesAndNamesFileSystems(version)
On 2013/06/05 00:24:09, kalman wrote:
> just have an "if version < 5: return version" at the top.

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:159: # existence if
the API is not stable in any of the _features.json files.
On 2013/06/05 00:24:09, kalman wrote:
> could you put all of these comments inside the blocks that guard them? it
would
> be easier to read.

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:185: def
_IsAvailableInFileSystem(self, api_name, version):
On 2013/06/05 00:24:09, kalman wrote:
> as noted below, make this _GetAvailableChannelForVersion

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:208: return
availability
On 2013/06/05 00:24:09, kalman wrote:
> more concise:
> 
> availability = AvailabilityInfo(api_info['channel'], api_info['version'])
> self._object_store.Set(api_name, availability)
> return availability
> 
> in fact there isn't much point caching it since json is cached anyway and this
> stuff is basically free to look up. so this can all be just
> 
> if api_info is not None:
>   return AvailabilityInfo(...)

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:217: break
On 2013/06/05 00:24:09, kalman wrote:
> i think this has a bug in scenarios such as: an API is present in the stable
> build but only available to dev channel. This algorithm will report stable,
but
> it's really only dev.
> 
> Try:
> 
> for channel_info in self._branch_utility.GetAllChannelInfo():
>   available_channel = self._GetAvailableChannelForVersion(
>       channel_info.version)
>   if (available_channel is not None and
>       BranchUtility.NewestVersion((available_channel,
>                                    channel_info.channel))
>           == available_channel):
>     availability = AvailabilityInfo(channel_info.channel,
>                                     channel_info.version)
>     break
> 
> and please test this!

Going to continue testing as I prepare to submit a patch devoted to
AvailabilityFixer and its test.

In the meantime, it looks like the if-test needs to be:
  if (available_channel is not None and
      BranchUtility.NewestVersion((available_channel,
                                   channel_info.channel))
          == channel_info.channel):

For example:
  - notifications is listed as 'dev' on beta and 'beta' on dev.
  - beta is checked first, and the available channel, 'dev', will be newer than
'beta'; this fails the NewestVersion(...) == channel_info.channel
  - dev is checked next, with the available_channel as 'beta' and
channel_info.channel as 'dev'. NewestVersion(...) will return 'dev', which will
pass the if-check.
  - notifications is available on the dev channel

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder.py:232: class
AvailabilityInfo(object):
On 2013/06/05 00:24:09, kalman wrote:
> declare at top

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder_test.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder_test.py:27: class
AvailabilityInfo(object):
On 2013/06/05 00:24:09, kalman wrote:
> this isn't used anywhere -
> 
> generally speaking please go over your code before you send it out to clean up
> things like this

Done.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder_test.py:44:
TestFileSystem(_LoadJSON('test_avail_filesystem.json')['trunk']),
On 2013/06/05 00:24:09, kalman wrote:
> put this file system in python not JSON.

Done, at least for the tests that I added. Should I go back through the tests
here and either inline or pythonify the JSON that's used for the test
results/test file system? Perhaps in a different patch?

[not at google - send to devlin, 2013-06-19 15:57:36]

Thanks again for splitting this up!

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:154:
self._LoadJSON('api_learn_more_expected.json'))
On 2013/06/17 20:05:49, epeterson wrote:
> On 2013/06/05 00:24:09, kalman wrote:
> > please just get this data from the canned file system data directly rather
> than
> > duplicating it in a separate file.
> > 
> > or if that's not possible, hard code it here. moving the data into JSON
files
> > obfuscates it.
> > 
> > same goes for the other things too.
> 
> Done, for the tests I added, at least. Should I go back through the rest of
the
> tests in the file and either inline or pythonify the JSON that's used as
> expected tests results/test file system? Perhaps as a separate patch?

Same reply to the other comment.

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/availability_finder_test.py (right):

https://codereview.chromium.org/12996003/diff/316001/chrome/common/extensions...
chrome/common/extensions/docs/server2/availability_finder_test.py:44:
TestFileSystem(_LoadJSON('test_avail_filesystem.json')['trunk']),
On 2013/06/17 20:05:49, epeterson wrote:
> On 2013/06/05 00:24:09, kalman wrote:
> > put this file system in python not JSON.
> 
> Done, at least for the tests that I added. Should I go back through the tests
> here and either inline or pythonify the JSON that's used for the test
> results/test file system? Perhaps in a different patch?

Nah don't worry about it, certainly not in this patch. Maybe a cleanup later.
You can add a TODO though.