Linking AvailabilityFinder with APIDataSource and intro-table templates.

Linking AvailabilityFinder with APIDataSource and intro-table templates.

BUG=233968, 163418
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212595

[epeterson, 2013-07-03 19:05:01]

Ben,

Here is a patch that hooks up AvailabilityFinder with APIDataSource, in addition
to actually displaying the generated intro-tables.

I apologize about the massive amount of HTML files touched. I went through the
API pages on preview.py, and looked over all of the files as well, trying to be
as careful as possible in checking for typos, bad links, broken tags, etc.

Hard-coded intro-tables were removed from the HTML intros, as well as
'classSummaries', since these were moved into the API schemas as descriptions.

I've also left a few initial comments for you.

Alright, thank you for reviewing.

Have a good day,

Evan

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/types.json:8: "description": "The
<code>chrome.types</code> API contains type declarations for Chrome. Currently,
this comprises only a prototype for giving other APIs access to manage Chrome
browser settings.",
proxy.settings is linked to right after the intro-table... but also, the ref
wasn't interpreted when it was pulled from the JSON here.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-9-0
Middle version bumped, since there have been changes to the templates/additional
data being sent to the templates.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:174: available =
available == _STABLE
There was a bug here: file system existence was checked even when an API was
listed as being beta/dev/trunk in one of the features files. 

The AvailabilityFinder test didn't catch this because I didn't put mock
.json/.idl files into the test file system where the APIs were in the mock
_features.json files. But I've added to the test file system so that this case
is tested now.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/api_availabilities.json:17: },
Here's an interesting point with checking availability in versions 18 and 19:

chrome.storage is listed as 20 in its current hardcoded intro-table.

Using AvailabilityFinder, chrome.storage's availability comes out to be 19. This
is because the _features.json files don't have 'channel' information in version
19 and don't exist in version 18.

So, file system existence is used, and storage.json exists in version 19, but
not in 18.

It doesn't seem like there's really a good reference point to use for versions
18 and 19.

[not at google - send to devlin, 2013-07-03 19:54:56]

awesome!

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/types.json:8: "description": "The
<code>chrome.types</code> API contains type declarations for Chrome. Currently,
this comprises only a prototype for giving other APIs access to manage Chrome
browser settings.",
On 2013/07/03 19:05:02, epeterson wrote:
> proxy.settings is linked to right after the intro-table... but also, the ref
> wasn't interpreted when it was pulled from the JSON here.

Hm. It would be nice to make the $ref work, and probably not that much effort.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:6: import json as
json_library
not used?

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:171: return None
invert the condition here:

if self._CheckExperimental():
  # A special message...
  return None
...

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:173: def
_CheckExperimental(self):
IsExperimental

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source_test.py:65: 'test')
indentation

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-9-0
On 2013/07/03 19:05:02, epeterson wrote:
> Middle version bumped, since there have been changes to the
templates/additional
> data being sent to the templates.

sgtm

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:174: available =
available == _STABLE
On 2013/07/03 19:05:02, epeterson wrote:
> There was a bug here: file system existence was checked even when an API was
> listed as being beta/dev/trunk in one of the features files. 
> 
> The AvailabilityFinder test didn't catch this because I didn't put mock
> .json/.idl files into the test file system where the APIs were in the mock
> _features.json files. But I've added to the test file system so that this case
> is tested now.

Cool, nice catch.

But I don't like the way that available is sometime a bool and other times a
channel. Could you introduce another variable like available_channel or
something?

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/cron_servlet.py:50: return
SubversionFileSystem.Create(branch)
maybe we should just rename CreateHostFIleSystemForBranchAndRevision to
CreateHostFileSystem(...) and make revision optional.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/cron_servlet.py:242: object_store_creator)
let's introduce a class like HostFileSystemCreator (vaguely analogous to
ObjectStoreCreator) with a method like CreateAtVersion.

then inject that into ServerInstance rather than this magic function.

I think there are other places that we could use such a class, but, worry about
that later.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/instance_servlet.py:49:
OfflineFileSystem(self._delegate.CreateHostFileSystemForBranch(
HostFileSystemCreator would need an offline= flag in the constructor I suppose.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/server_instance.py:163:
TestBranchUtility.CreateWithCannedData(),
and then on HostFileSystemCreator ForTest and ForLocal?

see how all that looks.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/api_availabilities.json:17: },
On 2013/07/03 19:05:02, epeterson wrote:
> Here's an interesting point with checking availability in versions 18 and 19:
> 
> chrome.storage is listed as 20 in its current hardcoded intro-table.
> 
> Using AvailabilityFinder, chrome.storage's availability comes out to be 19.
This
> is because the _features.json files don't have 'channel' information in
version
> 19 and don't exist in version 18.
> 
> So, file system existence is used, and storage.json exists in version 19, but
> not in 18.
> 
> It doesn't seem like there's really a good reference point to use for versions
> 18 and 19.

Ah right. How many does this affect?

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/intro_tables.json:4: {
"permission": "alarms" }
we should be able to figure this out from _api_data_source and the feature files
now.

we can also get better data and link to the manifest declaration, when Jared has
submitted that.

However, since you've put in the work, and since it might as well wait until
that manifest patch goes in, you can clean this up in a follow-up. As in, please
do it in a follow up, not now.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/intro_tables.json:97: "suffix": "
permission"
Let's just link the whole thing, and then later we can have like

The $ref:activeTab permission

Later.

For now, this prefix/suffix thing is a bit.. bizarre.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:5: </tr>
what is this empty row for..?

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:14: The
<code>chrome.{{api.name}}</code> API
end in fullstop

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:30:
{{/api.experimental}}
We need a richer data model here. Probably from API data source we'll set
- is_experimental
- is_dev
- is_beta
- stable_version

(only one of those).

There are already templates that exist for showing the dev/beta warnings, maybe
the experimental one, and then you'll need to add the "stable since Chrome X"
thing in here.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:43: <br>
why the <br>?

[not at google - send to devlin, 2013-07-08 17:08:02]

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:184: def
_GetMoreLearning(self):
Update: this actually needs to be more generic. See here:

https://codereview.chromium.org/18186004/diff/1/chrome/common/extensions/docs...

I think we'll need to slightly redesign this stuff to pass just a list of data
to the templates, and have it render in a generic way. Like, a list of tuples
like (title, content), where
- title would be something like "Description", "availability", etc.
- content would be a rich structure that captures what needs to be rendered.
this includes
  - the text to render
  - whether it's a link, in which case the link URL
  - what the style is
  - in fact it would probably be a list of such things.

So the example linked to above, it would be structure like:

[
  { "title": "Description",
    "content": [ { "text": "Use the Chrome In-App..." } ]
  },
  { "title": "Availability",
    "content": [ { "text": "Stable since version 29" } ]
  },
  { "title": "Samples",
    "content": [ { "link": "http://...", "text": "in-app-payments" } ]
  },
  { "title": "Learn more",
    "content": [
      { "link": "...", "text": "Google Wallet" },
      { "link": "...", "text": "Google Wallet for Digital Goods" }
    ]
  }
]

Now, the first two of those - Description/Availability - we can generate in the
way that you already are (description from the json, availability from logic +
those hard-coded things in the JSON. For availability though, we'll need to
generate a "class" property which would be "dev" or "beta" etc to include the
colour.

The others - samples/permission/learn more - we can generate from that other
JSON thing you've written, with the "permissions" later being automatically
generated.

[epeterson, 2013-07-09 20:51:17]

Afternoon,

The patch grew with this upload, and I'm sorry if it's getting a little daunting
for review.
I'm sure I can further break this up if you'd prefer.

Some stuff that you'll probably most definitely want to have input on:

The api_availabilities.json file:

I shared the results of my research on APIs around version 18/19.


The HostFileSystemCreator:

It's a little messy. I believe it has to be conscious of the delegates somehow,
otherwise it's not going to get the create method right for servlet use/testing.
So I ended up passing the delegate create method into it. Don't know if this is
what you intended (probably not?).


The Intro Tables Formatting/Templating:

Here's some screenshots for you to look at.
I changed the warning banner borders only if a 'beta', 'dev', or 'trunk' class
is added to them in addition to the base 'warning' class.

And, in order to get just the channel keyword highlighted, I ended up doing some
HTML stuff in AvailabilityFinder. I realize it isn't really the server's
business to directly do HTML, but I was having trouble figuring out how to do
specific HTML for the Availability table row while maintaining a generic-ish
data structure for building the tables.

Intro Table Links:

stable: http://i.imgur.com/55fq0oc.png
  (note that stable's color ends up being the same color as visited links)

beta: http://i.imgur.com/iQ49VEQ.png
  (beta's color is the same as the <code> tagged text)

dev: http://i.imgur.com/L3dikmu.png

trunk: http://i.imgur.com/cNafsBe.png


Let me know what you think.

Thanks,

Evan

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/types.json:8: "description": "The
<code>chrome.types</code> API contains type declarations for Chrome. Currently,
this comprises only a prototype for giving other APIs access to manage Chrome
browser settings.",
On 2013/07/03 19:54:56, kalman wrote:
> On 2013/07/03 19:05:02, epeterson wrote:
> > proxy.settings is linked to right after the intro-table... but also, the ref
> > wasn't interpreted when it was pulled from the JSON here.
> 
> Hm. It would be nice to make the $ref work, and probably not that much effort.

Ok, I made the change in APIDataSource. It wasn't too difficult.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:6: import json as
json_library
On 2013/07/03 19:54:56, kalman wrote:
> not used?

Done.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:171: return None
On 2013/07/03 19:54:56, kalman wrote:
> invert the condition here:
> 
> if self._CheckExperimental():
>   # A special message...
>   return None
> ...

Done.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:173: def
_CheckExperimental(self):
On 2013/07/03 19:54:56, kalman wrote:
> IsExperimental

Done.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source_test.py:65: 'test')
On 2013/07/03 19:54:56, kalman wrote:
> indentation

Done.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/availability_finder.py:174: available =
available == _STABLE
On 2013/07/03 19:54:56, kalman wrote:
> On 2013/07/03 19:05:02, epeterson wrote:
> > There was a bug here: file system existence was checked even when an API was
> > listed as being beta/dev/trunk in one of the features files. 
> > 
> > The AvailabilityFinder test didn't catch this because I didn't put mock
> > .json/.idl files into the test file system where the APIs were in the mock
> > _features.json files. But I've added to the test file system so that this
case
> > is tested now.
> 
> Cool, nice catch.
> 
> But I don't like the way that available is sometime a bool and other times a
> channel. Could you introduce another variable like available_channel or
> something?

Done. Also updated 'channel' to 'available_channel' in the
_GetAvailableChannelForVersion method to be consistent.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/cron_servlet.py:50: return
SubversionFileSystem.Create(branch)
On 2013/07/03 19:54:56, kalman wrote:
> maybe we should just rename CreateHostFIleSystemForBranchAndRevision to
> CreateHostFileSystem(...) and make revision optional.

The cron_servlet_test likes to count up how many file_systems it's been created
along with stat and read counts, so only having CreateHostFileSystem throws off
those tests.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/cron_servlet.py:242: object_store_creator)
On 2013/07/03 19:54:56, kalman wrote:
> let's introduce a class like HostFileSystemCreator (vaguely analogous to
> ObjectStoreCreator) with a method like CreateAtVersion.
> 
> then inject that into ServerInstance rather than this magic function.
> 
> I think there are other places that we could use such a class, but, worry
about
> that later.

Done.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/instance_servlet.py:49:
OfflineFileSystem(self._delegate.CreateHostFileSystemForBranch(
On 2013/07/03 19:54:56, kalman wrote:
> HostFileSystemCreator would need an offline= flag in the constructor I
suppose.

Done.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/server_instance.py:163:
TestBranchUtility.CreateWithCannedData(),
On 2013/07/03 19:54:56, kalman wrote:
> and then on HostFileSystemCreator ForTest and ForLocal?
> 
> see how all that looks.

Done.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/api_availabilities.json:17: },
On 2013/07/03 19:54:56, kalman wrote:
> On 2013/07/03 19:05:02, epeterson wrote:
> > Here's an interesting point with checking availability in versions 18 and
19:
> > 
> > chrome.storage is listed as 20 in its current hardcoded intro-table.
> > 
> > Using AvailabilityFinder, chrome.storage's availability comes out to be 19.
> This
> > is because the _features.json files don't have 'channel' information in
> version
> > 19 and don't exist in version 18.
> > 
> > So, file system existence is used, and storage.json exists in version 19,
but
> > not in 18.
> > 
> > It doesn't seem like there's really a good reference point to use for
versions
> > 18 and 19.
> 
> Ah right. How many does this affect?

Other APIs like browsingData, topSites, privacy, and debugger, fall into the
version 18/19 availability, but comparing these with information listed on the
whats_new page, they appear to have the correct generated availability.

Looking at the pageCapture API, the whats_new page lists it as stable in 18, but
17 is the generated availability, since it shows up in extension_api.json in
version 17 (and I thought that if it wasn't tagged as 'experimental' back in the
extension_api.json days, then it was 'stable').

input.ime is listed as available at 21 in the whats_new page. 
input.ime is experimental in version 17 in extension_api.json, and has a .json
file present up until then, so its given an availability of 18. It was not
represented in _manifest or _permission_features.json. 

Not sure how to get something accurate for cases like this, since there's no
hint as to what channels these APIs are available on before version 20, and
there's not a concrete method to use pre-version 20. This is kind of a letdown.
I apologize.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/api_availabilities.json:17: },
For reference, here's a list of the API availabilities:

scriptBadge: trunk
declarativeContent: trunk
systemInfo.memory: 29/dev
bluetooth: 29/dev
sessionRestore: 29/dev
systemInfo.cpu: 29/dev
feedbackPrivate: 29/dev
identity: 29/dev
downloads: 28/beta
syncFileSystem: 27
power: 27
usb: 26
systemInfo.display: 26
declarativeWebRequest: 26
tabCapture: 25
commands: 25
pushMessaging: 24
socket: 24
fileSystem: 23
serial: 23
app.window: 23
app.runtime: 23
mediaGalleries: 22
fontSettings: 22
alarms: 22
runtime: 22
events: 21
notifications: 20
browsingData: 19
topSites: 19
storage: 19 (should be 20)
debugger: 18
*devtools.inspectedWindow: 18
input.ime: 18 (should be 21)
privacy: 18
*devtools.panels: 18
*devtools.network: 18
webRequest: 17
pageCapture: 17 (should be 18)
webNavigation: 16
permissions: 16
contentSettings: 16
*webstore: 15
tts: 14
ttsEngine: 14
types: 13
proxy: 13
fileBrowserHandler: 12
omnibox: 9
management: 8
cookies: 6
contextMenus: 6
idle: 6
tabs: 5
pageAction: 5
browserAction: 5
i18n: 5
bookmarks: 5
extension: 5
windows: 5
history: 5

* = predetermined availability, pulled from version of api_availabilities.json
     currently on SVN

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/intro_tables.json:4: {
"permission": "alarms" }
On 2013/07/03 19:54:56, kalman wrote:
> we should be able to figure this out from _api_data_source and the feature
files
> now.
> 
> we can also get better data and link to the manifest declaration, when Jared
has
> submitted that.
> 
> However, since you've put in the work, and since it might as well wait until
> that manifest patch goes in, you can clean this up in a follow-up. As in,
please
> do it in a follow up, not now.

Ok, I put in a TODO.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/intro_tables.json:97: "suffix": "
permission"
On 2013/07/03 19:54:56, kalman wrote:
> Let's just link the whole thing, and then later we can have like
> 
> The $ref:activeTab permission
> 
> Later.
> 
> For now, this prefix/suffix thing is a bit.. bizarre.

Alright, the goal was to avoid using {{{triple-handlebars}}} while allowing for
the option of placing text before and after a link, but this has all changed a
lot now.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:5: </tr>
On 2013/07/03 19:54:56, kalman wrote:
> what is this empty row for..?

It was in the hardcoded intro tables, so I kind of just rolled with it.

But it looks like that scope attribute does nothing for browsers but is used for
screen readers?
I'm just going to get rid of it for now..

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:14: The
<code>chrome.{{api.name}}</code> API
On 2013/07/03 19:54:56, kalman wrote:
> end in fullstop

Done.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:30:
{{/api.experimental}}
On 2013/07/03 19:54:56, kalman wrote:
> We need a richer data model here. Probably from API data source we'll set
> - is_experimental
> - is_dev
> - is_beta
> - stable_version
> 
> (only one of those).
> 
> There are already templates that exist for showing the dev/beta warnings,
maybe
> the experimental one, and then you'll need to add the "stable since Chrome X"
> thing in here.

This comment may have been outdated by your most recent request to change the
data model.

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:43: <br>
On 2013/07/03 19:54:56, kalman wrote:
> why the <br>?

There's the possibility of permissions line-wrapping, like here:
  http://developer.chrome.com/trunk/apps/mediaGalleries.html

<br>'s prevent this, at the expense of shorter permissions not being able to fit
all on one line, I suppose.
Doesn't having each permission on a separate line more accurately model how
permissions look when declared in a manifest, as well?

Of course, if permissions should look like they do in manifest files, then they
need double-quotes (which is what they have in the hardcoded tables)...so I
added those just now.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:1: {
I modified this file so that it is similar in structure to the generic data
structure passed to the templates by api_data_source

[not at google - send to devlin, 2013-07-09 21:07:16]

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/api_availabilities.json:17: },
> Not sure how to get something accurate for cases like this, since there's no
> hint as to what channels these APIs are available on before version 20, and
> there's not a concrete method to use pre-version 20. This is kind of a
letdown.
> I apologize.

Heh, not at all, it was pretty undisciplined back then. Perhaps these
restrictions were implemented procedurally or something.

For things as far back s 18/19/20 I think nobody will notice if they're a couple
of versions off, though we should try our best, and yeah if you're not sure then
just be conservative and use the most recent version.

[not at google - send to devlin, 2013-07-09 23:11:55]

not a complete set of comments - you're right, the change is getting a bit
sprawling again and it'd be good to consolidate - I'm a bit unsure of what
change is for what exactly and what the state of things are.

Could you pull out
- those IDL/JSON changes
- the HostFileSystemCreator changes
into separate patches?

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/cron_servlet.py:50: return
SubversionFileSystem.Create(branch)
On 2013/07/09 20:51:18, epeterson wrote:
> On 2013/07/03 19:54:56, kalman wrote:
> > maybe we should just rename CreateHostFIleSystemForBranchAndRevision to
> > CreateHostFileSystem(...) and make revision optional.
> 
> The cron_servlet_test likes to count up how many file_systems it's been
created
> along with stat and read counts, so only having CreateHostFileSystem throws
off
> those tests.

ah right. and this does actually break the test? I can see how it would.

maybe we can add a primary_for_test flag to this method and cron_servlet_test
can use that flag to determine whether it should add to its list thing.

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

https://codereview.chromium.org/18323018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/private/intro_table.html:43: <br>
On 2013/07/09 20:51:18, epeterson wrote:
> On 2013/07/03 19:54:56, kalman wrote:
> > why the <br>?
> 
> There's the possibility of permissions line-wrapping, like here:
>   http://developer.chrome.com/trunk/apps/mediaGalleries.html
> 
> <br>'s prevent this, at the expense of shorter permissions not being able to
fit
> all on one line, I suppose.
> Doesn't having each permission on a separate line more accurately model how
> permissions look when declared in a manifest, as well?
> 
> Of course, if permissions should look like they do in manifest files, then
they
> need double-quotes (which is what they have in the hardcoded tables)...so I
> added those just now.

sgtm

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:188:
node['content'].append(obj)
node['content'] += table_info[category]

? or even just

intro_list.append({
  'title': category,
  'content': table_info.get(category, [])
})

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:23: '<a
href="experimental.html#using">How to use experimental APIs</a>).')
ah. so maybe we need to actually pass _templates_ into that table? definitely
can't be embedding HTML in the server code.

so like, allow passing a 'partial' argument as well (like 'text'), then the
intro content would look something like

{{?link ... }}
{{?class ... }}
{{?text text /}}
{{?partial +partial /}}
{{?class...}}
{{?link ...}}

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:267:
availability_id, availability.channel, availability.channel)
Yeah ok so same thing with the partials. We'll need to to some tweaking of the
language. Maybe for the beta version it shlud be

"<a ...>Beta and dev channels</a> only"

dev

"<a...>Dev channel</a> only"

trunk

"Only when <a>built from source</a>"

and we can add the same links there that we already have for the warnings.

oh and, the screenshots look awesome but it makes me realise that the colours
there are actually really confusing, so let's give up on that avenue :( sorry.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet.py:233: offline=False)
False should be the default here so you can leave it out

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet.py:234: # Using
HostFileSystemCreator
yep let's use it

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/host_file_system_creator.py:19:
object_store_creator=None,
how/why can/should this be None?

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/host_file_system_creator.py:20:
offline=True):
always default to False. it makes sense here.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/patch_servlet.py:65: offline=False)
=False also not needed here

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:63:
host_file_system_creator.CreateAtVersion)
pass the host_file_system_creator to AvailabilityFinder, not just that function.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:1: {
On 2013/07/09 20:51:18, epeterson wrote:
> I modified this file so that it is similar in structure to the generic data
> structure passed to the templates by api_data_source

sgtm

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:12: "perm":
"alarms"
can this be

"text": "\"alarms\"",
"class": "code"

then in site.css where it defines the style for the code element also define it
for the code class.

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:165: "perm":
"fileSystem.retainEntries"
these are actually wrong, should be:

"\"fileSystem\""
"{\"fileSystem\": [\"write\"]}"
"{\"fileSystem\": [\"write\", \"retainEntries\"]}"

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:231: "text":
"\"read\", \"allAutoDetected\""
ok maybe I'll need to read the python to understand this.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/intro_table.html:16: {{{text}}}
so regarding your thing about not needing {{{ }}} - yes, it would be great not
to need it and to encode any styling into the model.

I think I made some comments earlier which allude to you hopefully not needing
the "perm" special case at all.

can this look like:

{{#content}}
  {{?link}}<a href="{{link}}">{{/}}
  {{?class}}<span class="{{class}}">{{/}}
  {{text}}
  {{?class}}</span>{{/}}
  {{?link}}</a>{{/}}
{{/content}}

it would be much simpler... though maybe it would look better expanded, dunno.

{{#content}}
  {{?link}}
    <a href="{{link}}">
  {{/link}}
  {{?class}}
    <span class="{{class}}">
  {{/class}}
  {{text}}
  {{?class}}
    </span>
  {{/class}}
  {{?link}}
    </a>
  {{/link}}
{{/content}}

maybe not.

[epeterson, 2013-07-16 00:28:23]

Good evening Ben,

I apologize this is coming in so late today, but I tried to be extra cautious
about the smaller details/making sure everything worked properly. 

There are some rough spots that you'll want to look at, and I pointed them out
for you (mainly cron_servlet_test, and of course the
Availability-intro-table-row template interaction).


Thank you for your time,

Evan

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:188:
node['content'].append(obj)
On 2013/07/09 23:11:55, kalman wrote:
> node['content'] += table_info[category]
> 
> ? or even just
> 
> intro_list.append({
>   'title': category,
>   'content': table_info.get(category, [])
> })

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:23: '<a
href="experimental.html#using">How to use experimental APIs</a>).')
On 2013/07/09 23:11:55, kalman wrote:
> ah. so maybe we need to actually pass _templates_ into that table? definitely
> can't be embedding HTML in the server code.
> 
> so like, allow passing a 'partial' argument as well (like 'text'), then the
> intro content would look something like
> 
> {{?link ... }}
> {{?class ... }}
> {{?text text /}}
> {{?partial +partial /}}
> {{?class...}}
> {{?link ...}}

Solution to this no longer involves availability_finder for HTML, which is good.
The logic is over in api_data_source.

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:267:
availability_id, availability.channel, availability.channel)
On 2013/07/09 23:11:55, kalman wrote:
> Yeah ok so same thing with the partials. We'll need to to some tweaking of the
> language. Maybe for the beta version it shlud be
> 
> "<a ...>Beta and dev channels</a> only"
> 
> dev
> 
> "<a...>Dev channel</a> only"
> 
> trunk
> 
> "Only when <a>built from source</a>"
> 
> and we can add the same links there that we already have for the warnings.
> 
> oh and, the screenshots look awesome but it makes me realise that the colours
> there are actually really confusing, so let's give up on that avenue :( sorry.

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet.py:233: offline=False)
On 2013/07/09 23:11:55, kalman wrote:
> False should be the default here so you can leave it out

Done.

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet.py:234: # Using
HostFileSystemCreator
On 2013/07/09 23:11:55, kalman wrote:
> yep let's use it

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/host_file_system_creator.py:19:
object_store_creator=None,
On 2013/07/09 23:11:55, kalman wrote:
> how/why can/should this be None?

It is None no longer!

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/host_file_system_creator.py:20:
offline=True):
On 2013/07/09 23:11:55, kalman wrote:
> always default to False. it makes sense here.

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/patch_servlet.py:65: offline=False)
On 2013/07/09 23:11:55, kalman wrote:
> =False also not needed here

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:63:
host_file_system_creator.CreateAtVersion)
On 2013/07/09 23:11:55, kalman wrote:
> pass the host_file_system_creator to AvailabilityFinder, not just that
function.

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:12: "perm":
"alarms"
On 2013/07/09 23:11:55, kalman wrote:
> can this be
> 
> "text": "\"alarms\"",
> "class": "code"
> 
> then in site.css where it defines the style for the code element also define
it
> for the code class.

Done.

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:165: "perm":
"fileSystem.retainEntries"
On 2013/07/09 23:11:55, kalman wrote:
> these are actually wrong, should be:
> 
> "\"fileSystem\""
> "{\"fileSystem\": [\"write\"]}"
> "{\"fileSystem\": [\"write\", \"retainEntries\"]}"

Done.

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

https://codereview.chromium.org/18323018/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/intro_table.html:16: {{{text}}}
On 2013/07/09 23:11:55, kalman wrote:
> so regarding your thing about not needing {{{ }}} - yes, it would be great not
> to need it and to encode any styling into the model.
> 
> I think I made some comments earlier which allude to you hopefully not needing
> the "perm" special case at all.
> 
> can this look like:
> 
> {{#content}}
>   {{?link}}<a href="{{link}}">{{/}}
>   {{?class}}<span class="{{class}}">{{/}}
>   {{text}}
>   {{?class}}</span>{{/}}
>   {{?link}}</a>{{/}}
> {{/content}}
> 
> it would be much simpler... though maybe it would look better expanded, dunno.
> 
> {{#content}}
>   {{?link}}
>     <a href="{{link}}">
>   {{/link}}
>   {{?class}}
>     <span class="{{class}}">
>   {{/class}}
>   {{text}}
>   {{?class}}
>     </span>
>   {{/class}}
>   {{?link}}
>     </a>
>   {{/link}}
> {{/content}}
> 
> maybe not.

The "perm" special case is gone.

I think the {{{ }}}'s are going to be hard to avoid, at least with this model.

Currently, each node represents a piece of text wrapped in a span or link.

We'd need to be able to put text both before and after a link or span, and we'd
need to be able to do it multiple times for one block of text, as with an API
description, where there could be multiple spans or links present.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:178: }
I tried a lot of different things here.

I couldn't get something like the 
  {{?partial}}{{+partial}}{{/partial}} 
idea to work, but I may have been approaching it the wrong way.

Currently, this information will get passed to the availability_messages partial
template because 'message' resolves to true. 

status will be the name of the channel, or 'experimental', which
availability_messages will check and provide an appropriate message for.

'version' will be included as {{version}} {{?stable}} evaluates to true in the
availability_messages (status == 'stable' here).

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-9-3
updated app.yaml bump

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:226: return
availability
Previously if an API had an entry in api_availabilities.json (a predetermined
availability), then that result wasn't cached.

(although this may have been intentional?)

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet_test.py:65: num_file_systems
= 14
This is a pretty magical number. I mean, anything with that much justification
is suspicious.

Now that an AvailabilityFinder instance is actually being called from
APIDataSource, extra file systems are being created.

So, 'num_file_systems' is determined by the data that the TestBranchUtility has
access to, which currently has 'dev' set as version 28. And AvailabilityFinder
goes down to version 17 before bailing because it can't find extension_api.json.
So that's 13 file systems (17-28 and trunk) plus another host file system for
the server instance.

TestBranchUtility could probably expose a method that calculates this
number (and there could be a nice explanative doc comment there), though I'm not
sure if this is a better idea than the test divergent primary_for_test flag,
which came up in the HostFileSystemCreator code review.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/api_availabilities.json:4: // or
from the whats_new page.
added some comments to api_availabilities, so they're not AS arbitrary.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/availability_messages.html
(right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/availability_messages.html:17:
{{/message}}
All of the possible availability messages were put into this file to keep them
nice and tidy.

This template will have a content node from the intro_list passed into it, so it
'knows' which message to display and has access to the version if an API is
'stable.'

[not at google - send to devlin, 2013-07-16 22:25:00]

I'm not sure I got everything, since I've been reviewing this patch for 6 hours
with constant distractions and forgotten where I was with it - but this should
be enough to go with.

I think this is basically ready to submit, the last big question is that partial
argument thign.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:163: })
more concise:

intro_list = [{
  'title': 'Description',
  'content': ...
}]

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:177: 'version':
availability.version if status == 'stable' else None
maybe hold onto version rather than availability, to avoid awkward python
ternary syntax.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:178: }
On 2013/07/16 00:28:23, epeterson wrote:
> I tried a lot of different things here.
> 
> I couldn't get something like the 
>   {{?partial}}{{+partial}}{{/partial}} 
> idea to work, but I may have been approaching it the wrong way.
> 
> Currently, this information will get passed to the availability_messages
partial
> template because 'message' resolves to true. 
> 
> status will be the name of the channel, or 'experimental', which
> availability_messages will check and provide an appropriate message for.
> 
> 'version' will be included as {{version}} {{?stable}} evaluates to true in the
> availability_messages (status == 'stable' here).

I think you could combine message and status into a single partial argument.

You'd set the partial to whatever the status implies it to be. so if it's dev,
retrieve the template for the dev warning, and pass it that.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-9-3
On 2013/07/16 00:28:23, epeterson wrote:
> updated app.yaml bump

2-9-0 :)

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:226: return
availability
On 2013/07/16 00:28:23, epeterson wrote:
> Previously if an API had an entry in api_availabilities.json (a predetermined
> availability), then that result wasn't cached.
> 
> (although this may have been intentional?)

Umm yeah I remember something like this, since we were caching the
api_availabilities.json object there didn't seem much point in double-caching
it.

Let's keep it that way, but add a comment explaining it.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet_test.py:65: num_file_systems
= 14
On 2013/07/16 00:28:23, epeterson wrote:
> This is a pretty magical number. I mean, anything with that much justification
> is suspicious.
> 
> Now that an AvailabilityFinder instance is actually being called from
> APIDataSource, extra file systems are being created.
> 
> So, 'num_file_systems' is determined by the data that the TestBranchUtility
has
> access to, which currently has 'dev' set as version 28. And AvailabilityFinder
> goes down to version 17 before bailing because it can't find
extension_api.json.
> So that's 13 file systems (17-28 and trunk) plus another host file system for
> the server instance.
> 
> TestBranchUtility could probably expose a method that calculates this
> number (and there could be a nice explanative doc comment there), though I'm
not
> sure if this is a better idea than the test divergent primary_for_test flag,
> which came up in the HostFileSystemCreator code review.

Ok maybe the answer here is just the test isn't meaningful anymore (at least,
the assertEqual(num_file_systems...) part).

So let's delete those.

Then this test could be something like:

response = CronServlet(Request.ForTest('trunk'), ...)
self.assertEqual(200, response.status)

# Save the file systems created, start with a fresh set for the next run.
first_run_file_systems = delegate.file_systems[:]
delegate.file_systems[:] = []

# When re-running....
response = CronServlet(Request.ForTest('trunk'), ...)
self.assertEqual(200, response.status)

self.assertEqual(len(first_run_file_systems), len(delegate.file_systems))
for i, second_run_file_system in enumerate(delegate.file_systems):
  self.assertTrue(*second_run_file_system.CheckAndReset(
      read_count=0,
      stat_count=first_run_file_systems[i].GetStatCount())

Does that seem ok?

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/api_availabilities.json:19: //
Generated availability of version 18.
myeh, these "Generated..." comments are confusing.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:254: "text": "See
<a href='#manifest'>Manifest</a> below for more information."
I mean, you could have like

"partial": "intro_tables/permissions.html"

and then add templates/private/intro_tables/permissions.html with

See <a href=....> below for more information

(incidentally it's a bit of a shame to make that partial path resolve
automatically into the "private" directory, but I have a plan/TODO to fix that
stuff up so it's fine for now).

anyway - likewise for the other rich-HTML things here. There aren't that many.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/availability_messages.html
(right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/availability_messages.html:3:
Stable since version {{version}}.
Stable since Chrome {{version}}.

Also could you add

{{- TODO(epeterson): link to the entry in What's New page once it's generated.
-}}

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/availability_messages.html:17:
{{/message}}
On 2013/07/16 00:28:23, epeterson wrote:
> All of the possible availability messages were put into this file to keep them
> nice and tidy.
> 
> This template will have a content node from the intro_list passed into it, so
it
> 'knows' which message to display and has access to the version if an API is
> 'stable.'

yeah so I'll comment in the data source. we'd need to pull the "stable since
Chrome {{version}}: into a separate partial but other than that they're all
there.

may also need to pull the wording out into separate partials?

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/intro_table.html:12:
{{?message}}{{+partials.availability_messages message:content}}{{/message}}
{{?partial}}
  {{+partial content:content}}
{{/partial}}

i.e. no message.

but I'll comment as such in the api data source.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/warning_beta.html (right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/warning_beta.html:1: <p
class="warning beta">
do we still need these changes?

[epeterson, 2013-07-17 23:48:22]

Partials are being passed to the templates now. 
Just about all of the logic for that is in api_data_source.

Moved the intro_tables-related partials into private/intro_tables/

Other than that, just small changes.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:163: })
On 2013/07/16 22:25:00, kalman wrote:
> more concise:
> 
> intro_list = [{
>   'title': 'Description',
>   'content': ...
> }]

Done.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:177: 'version':
availability.version if status == 'stable' else None
On 2013/07/16 22:25:00, kalman wrote:
> maybe hold onto version rather than availability, to avoid awkward python
> ternary syntax.

Done.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:178: }
On 2013/07/16 22:25:00, kalman wrote:
> On 2013/07/16 00:28:23, epeterson wrote:
> > I tried a lot of different things here.
> > 
> > I couldn't get something like the 
> >   {{?partial}}{{+partial}}{{/partial}} 
> > idea to work, but I may have been approaching it the wrong way.
> > 
> > Currently, this information will get passed to the availability_messages
> partial
> > template because 'message' resolves to true. 
> > 
> > status will be the name of the channel, or 'experimental', which
> > availability_messages will check and provide an appropriate message for.
> > 
> > 'version' will be included as {{version}} {{?stable}} evaluates to true in
the
> > availability_messages (status == 'stable' here).
> 
> I think you could combine message and status into a single partial argument.
> 
> You'd set the partial to whatever the status implies it to be. so if it's dev,
> retrieve the template for the dev warning, and pass it that.

Done. But I had to add in extra logic to pass in a compiled_fs for retrieving
templates and giving them to Handlebar().

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-9-3
On 2013/07/16 22:25:00, kalman wrote:
> On 2013/07/16 00:28:23, epeterson wrote:
> > updated app.yaml bump
> 
> 2-9-0 :)

Done.
(So much for being cautious about the smaller details!)

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/availability_finder.py:226: return
availability
On 2013/07/16 22:25:00, kalman wrote:
> On 2013/07/16 00:28:23, epeterson wrote:
> > Previously if an API had an entry in api_availabilities.json (a
predetermined
> > availability), then that result wasn't cached.
> > 
> > (although this may have been intentional?)
> 
> Umm yeah I remember something like this, since we were caching the
> api_availabilities.json object there didn't seem much point in double-caching
> it.
> 
> Let's keep it that way, but add a comment explaining it.

Done.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/cron_servlet_test.py:65: num_file_systems
= 14
On 2013/07/16 22:25:00, kalman wrote:
> On 2013/07/16 00:28:23, epeterson wrote:
> > This is a pretty magical number. I mean, anything with that much
justification
> > is suspicious.
> > 
> > Now that an AvailabilityFinder instance is actually being called from
> > APIDataSource, extra file systems are being created.
> > 
> > So, 'num_file_systems' is determined by the data that the TestBranchUtility
> has
> > access to, which currently has 'dev' set as version 28. And
AvailabilityFinder
> > goes down to version 17 before bailing because it can't find
> extension_api.json.
> > So that's 13 file systems (17-28 and trunk) plus another host file system
for
> > the server instance.
> > 
> > TestBranchUtility could probably expose a method that calculates this
> > number (and there could be a nice explanative doc comment there), though I'm
> not
> > sure if this is a better idea than the test divergent primary_for_test flag,
> > which came up in the HostFileSystemCreator code review.
> 
> Ok maybe the answer here is just the test isn't meaningful anymore (at least,
> the assertEqual(num_file_systems...) part).
> 
> So let's delete those.
> 
> Then this test could be something like:
> 
> response = CronServlet(Request.ForTest('trunk'), ...)
> self.assertEqual(200, response.status)
> 
> # Save the file systems created, start with a fresh set for the next run.
> first_run_file_systems = delegate.file_systems[:]
> delegate.file_systems[:] = []
> 
> # When re-running....
> response = CronServlet(Request.ForTest('trunk'), ...)
> self.assertEqual(200, response.status)
> 
> self.assertEqual(len(first_run_file_systems), len(delegate.file_systems))
> for i, second_run_file_system in enumerate(delegate.file_systems):
>   self.assertTrue(*second_run_file_system.CheckAndReset(
>       read_count=0,
>       stat_count=first_run_file_systems[i].GetStatCount())
> 
> Does that seem ok?

That seems ok.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/api_availabilities.json:19: //
Generated availability of version 18.
On 2013/07/16 22:25:00, kalman wrote:
> myeh, these "Generated..." comments are confusing.

You're right, they don't really help anyone much.
And trying to explain the details of the availability algorithm within this file
would likely be confusing as well.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/intro_tables.json:254: "text": "See
<a href='#manifest'>Manifest</a> below for more information."
On 2013/07/16 22:25:00, kalman wrote:
> I mean, you could have like
> 
> "partial": "intro_tables/permissions.html"
> 
> and then add templates/private/intro_tables/permissions.html with
> 
> See <a href=....> below for more information
> 
> (incidentally it's a bit of a shame to make that partial path resolve
> automatically into the "private" directory, but I have a plan/TODO to fix that
> stuff up so it's fine for now).
> 
> anyway - likewise for the other rich-HTML things here. There aren't that many.

Done.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/availability_messages.html
(right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/availability_messages.html:3:
Stable since version {{version}}.
On 2013/07/16 22:25:00, kalman wrote:
> Stable since Chrome {{version}}.
> 
> Also could you add
> 
> {{- TODO(epeterson): link to the entry in What's New page once it's generated.
> -}}

Done.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/availability_messages.html:17:
{{/message}}
On 2013/07/16 22:25:00, kalman wrote:
> On 2013/07/16 00:28:23, epeterson wrote:
> > All of the possible availability messages were put into this file to keep
them
> > nice and tidy.
> > 
> > This template will have a content node from the intro_list passed into it,
so
> it
> > 'knows' which message to display and has access to the version if an API is
> > 'stable.'
> 
> yeah so I'll comment in the data source. we'd need to pull the "stable since
> Chrome {{version}}: into a separate partial but other than that they're all
> there.
> 
> may also need to pull the wording out into separate partials?

Done.

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

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/intro_table.html:12:
{{?message}}{{+partials.availability_messages message:content}}{{/message}}
On 2013/07/16 22:25:00, kalman wrote:
> {{?partial}}
>   {{+partial content:content}}
> {{/partial}}
> 
> i.e. no message.
> 
> but I'll comment as such in the api data source.

Done.

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/warning_beta.html (right):

https://codereview.chromium.org/18323018/diff/67002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/warning_beta.html:1: <p
class="warning beta">
On 2013/07/16 22:25:00, kalman wrote:
> do we still need these changes?

Nope, reverted them.

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

https://codereview.chromium.org/18323018/diff/80001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:207:
intro_list.append(node)
Also, this became messy with the special behavior required for partials.

[not at google - send to devlin, 2013-07-18 02:20:30]

lg just those last 2 comments!

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

https://codereview.chromium.org/18323018/diff/80001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:123:
self._partial_cache = partial_cache
can you pass in the TemplateDataSource instance instead? You will probably need
to add a SetTemplateDataSource method in there somewhere, since
TemplateDataSource needs an APIDataSource already.

that already does all its own caching etc.

https://codereview.chromium.org/18323018/diff/80001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:201: if category in
table_info.keys():
early-exit here:

if category not in table_info.keys():
  continue

https://codereview.chromium.org/18323018/diff/80001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:207:
intro_list.append(node)
On 2013/07/17 23:48:23, epeterson wrote:
> Also, this became messy with the special behavior required for partials.

ah right. Yeah - oh well.

when is isinstance(obj['partial'], Handlebar) actually true? It should always be
false. Please comment it, and I think this could be structured a little more
clearly:

# Transform the 'partial' argument from the partial name to the template itself.
content = table_info[category]
if 'partial' in content:
  content['partial'] =
self._template_data_source.GetTemplate(content['partial'])
intro_list.append({
  'title': category,
  'content': content
})

[epeterson, 2013-07-18 18:17:07]

How does this look?

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

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:203: if 'partial' in
node and isinstance (node['partial'], basestring):
Similar to the isinstance(.. Handlebar) check, though perhaps clearer with the
added comment?

If this check isn't here, then the 'partial' argument can be fetched again,
which throws an error (Handlebar object has no .endswith() method for
.endswith('html')) since it's no longer a string to look up.

An alternative would be to mutate the dict and add a separate argument with the
Handlebar data, like 'template', and not directly mutation the 'partial'
argument's data.

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:450:
self._template_data_source = template_data_source_factory.Create(None, '')
No request or path data is given here, since this template data source doesn't
need the information for getting template data. (Also, I'm not sure how I would
magick up a request and path here)

Creating just one template data source here instead of setting a factory and
passing that factory to JSCModel and having it create tons of template data
sources.

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-10-0
Updated the version bump.

Also, if that third number should roll back to 0 when the middle number is
updated, then the Presubmit check should probably be changed.

Currently it suggests '2-5-2 -> 2-6-2'

[not at google - send to devlin, 2013-07-18 21:24:15]

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

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:203: if 'partial' in
node and isinstance (node['partial'], basestring):
On 2013/07/18 18:17:07, epeterson wrote:
> Similar to the isinstance(.. Handlebar) check, though perhaps clearer with the
> added comment?
> 
> If this check isn't here, then the 'partial' argument can be fetched again,
> which throws an error (Handlebar object has no .endswith() method for
> .endswith('html')) since it's no longer a string to look up.
> 
> An alternative would be to mutate the dict and add a separate argument with
the
> Handlebar data, like 'template', and not directly mutation the 'partial'
> argument's data.

Oh so it shouldn't be computing this more than once because it's supposed to be
caching the result of ToDict. This method is only called for ToDict and ToDict
is only called from within callbacks of a CompiledFileSystem...

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:450:
self._template_data_source = template_data_source_factory.Create(None, '')
On 2013/07/18 18:17:07, epeterson wrote:
> No request or path data is given here, since this template data source doesn't
> need the information for getting template data. (Also, I'm not sure how I
would
> magick up a request and path here)
> 
> Creating just one template data source here instead of setting a factory and
> passing that factory to JSCModel and having it create tons of template data
> sources.

Ah ok. That's fine for now. I will try to refactor things a bit here to have
more sensible dependencies.

[epeterson, 2013-07-19 00:05:34]

Ok, here are some quick changes.

Let me know what you think.

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

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:203: if 'partial' in
node and isinstance (node['partial'], basestring):
On 2013/07/18 21:24:15, kalman wrote:
> On 2013/07/18 18:17:07, epeterson wrote:
> > Similar to the isinstance(.. Handlebar) check, though perhaps clearer with
the
> > added comment?
> > 
> > If this check isn't here, then the 'partial' argument can be fetched again,
> > which throws an error (Handlebar object has no .endswith() method for
> > .endswith('html')) since it's no longer a string to look up.
> > 
> > An alternative would be to mutate the dict and add a separate argument with
> the
> > Handlebar data, like 'template', and not directly mutation the 'partial'
> > argument's data.
> 
> Oh so it shouldn't be computing this more than once because it's supposed to
be
> caching the result of ToDict. This method is only called for ToDict and ToDict
> is only called from within callbacks of a CompiledFileSystem...

Added a TODO, but I'm curious as to what I may have done that stopped this from
caching properly... or if it's been that way? (hopefully not)

https://codereview.chromium.org/18323018/diff/99001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:569: 
This stuff is gone, it was only used for the warning bar.

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

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:154:
'development_stage': self._GetDevelopmentStage()
So, 'development_stage' instead of 'permissions'?

I just thought permissions wasn't really relevant. And 'development_channel' or
'dev_channel' could be confusing.

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/fileSystem.html (left):

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/fileSystem.html:1: <table
class="intro">
Found this intro, so it's gone now.
(Apologies to the tech writer who worked so hard on it)

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

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/standard_apps_api.html:16:
{{?api.development_stage.beta}}{{+partials.warning_beta}}{{/}}
permissions -> development_stage

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
File
chrome/common/extensions/docs/templates/private/standard_extensions_api.html
(right):

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/standard_extensions_api.html:16:
{{?api.development_stage.beta}}{{+partials.warning_beta}}{{/}}
permissions -> development_stage

[not at google - send to devlin, 2013-07-19 00:18:04]

lgtm

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

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:207: if 'partial' in
node and not isinstance (node['partial'], Handlebar):
no space between isinstance and the opening parens

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:220: return {
self._GetApiAvailability().channel: True }
how about "channel warning" rather than "development stage". but yes!

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

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/standard_apps_api.html:16:
{{?api.development_stage.beta}}{{+partials.warning_beta}}{{/}}
On 2013/07/19 00:05:35, epeterson wrote:
> permissions -> development_stage

channel_warning would be even better.

[epeterson, 2013-07-19 00:50:19]

Ok, here's some final small changes.

This patch is going to require a lot of work with SVN, though, right?

It's going to need access to:
  - intro_tables.json
  - updated api_availabilities.json (if that's important)
  - the intro_table-related templates that have been added

across the development channels.
Or maybe that's not quite right?

Whatever the case, thank you for putting in all the time to review this.

- Evan

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

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:207: if 'partial' in
node and not isinstance (node['partial'], Handlebar):
On 2013/07/19 00:18:04, kalman wrote:
> no space between isinstance and the opening parens

Done.

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:220: return {
self._GetApiAvailability().channel: True }
On 2013/07/19 00:18:04, kalman wrote:
> how about "channel warning" rather than "development stage". but yes!

Done.

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

https://codereview.chromium.org/18323018/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/standard_apps_api.html:16:
{{?api.development_stage.beta}}{{+partials.warning_beta}}{{/}}
On 2013/07/19 00:18:04, kalman wrote:
> On 2013/07/19 00:05:35, epeterson wrote:
> > permissions -> development_stage
> 
> channel_warning would be even better.

Done.

[not at google - send to devlin, 2013-07-19 01:01:58]

Yeah I'm going to submit https://codereview.chromium.org/15087006/ after this
and we'll never need to merge again!

[commit-bot: I haz the power, 2013-07-19 16:14:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/18323018/12...

[commit-bot: I haz the power, 2013-07-19 16:15:15]

Failed to apply patch for
chrome/common/extensions/docs/templates/intros/tts.html:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/docs/templates/intros/tts.html
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/docs/templates/intros/tts.html.rej

Patch:       chrome/common/extensions/docs/templates/intros/tts.html
Index: chrome/common/extensions/docs/templates/intros/tts.html
diff --git a/chrome/common/extensions/docs/templates/intros/tts.html
b/chrome/common/extensions/docs/templates/intros/tts.html
index
a7387d6ad4a199183595adf873ee5dc83ef9c311..486756f2277db84cbf2436b4c6f774138504f0e9
100644
--- a/chrome/common/extensions/docs/templates/intros/tts.html
+++ b/chrome/common/extensions/docs/templates/intros/tts.html
@@ -1,35 +1,3 @@
-<table class="intro">
-  <tr>
-    <th scope="col"></th>
-    <th scope="col"></th>
-  </tr>
-  <tr>
-    <td><strong>Description:</strong></td>
-    <td>Use the <code>chrome.tts</code> module to play synthesized
-    text-to-speech (TTS).
-    See also the related
-    <a
href="http://developer.chrome.com/extensions/ttsEngine.html">ttsEngine</a>
-    module, which allows an extension to implement a speech engine.</td>
-  </tr>
-  <tr>
-    <td><strong>Availability:</strong></td>
-    <td>Google Chrome 14</td>
-  </tr>
-  <tr>
-    <td><strong>Permissions:</strong></td>
-    <td><code>"tts"</code></td>
-  </tr>
-  <tr>
-    <td><strong>Learn more:</strong></td>
-    <td><a href="https://developers.google.com/live/shows/7320022-7001/">Chrome
Office Hours: Text to Speech API</a></td>
-  </tr>
-</table>
-
-<p id="classSummary">
-
-</p>
-
-
 <h2 id="overview">Overview</h2>
 
 <p>Chrome provides native support for speech on Windows (using SAPI

[commit-bot: I haz the power, 2013-07-19 16:40:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/18323018/12...

[commit-bot: I haz the power, 2013-07-19 16:47:33]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-19 16:52:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/18323018/12...

[commit-bot: I haz the power, 2013-07-19 16:55:20]

Change committed as 212595