Doc server manifest page generation

Docserver: generate the manifest page.

The manifest page is now unique for apps and extensions and the property descriptions have their own pages.

BUG=233966
R=kalman@chromium.org
NOTRY=true

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

[jshumway, 2013-04-27 01:33:40]

This is a rough draft of the fix.

I broke all of the property descriptions from the manifest page off into their
own page. I added two json files that are used to generate manifest files, one
for apps and one for extensions. Each json file links to a set of templates in
public/extensions or apps, which render the property documentation pages.
Currently these property documentation pages are shared among the two, but there
is no reason that they have to be if the property varies among apps and
extensions.

Obviously, many of the property pages are too short. Let me know what you would
like to have done with these pages.

I would like to add a test that renders all of the pages linked to from a
manifest json file to make sure they are all there, but I was not able to get it
working in time, so I settled for checking if the intermediate files exist. This
will be fixed next week.

[not at google - send to devlin, 2013-04-27 01:54:02]

Awesome, that was quick. Need to run so only looked at it for a couple of
minutes but have a couple of comments so far. Next week.. the rest.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/manifest_app.html:2: 
can you put these in articles/manifest/app.html,
articles/manifest/browserAction.html, etc?

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/extensions_manifest.json:7: },
I actually think we'll only want one of these files both both apps and
extensions, then the server can pull out what it needs (depending on whether
it's looking at apps or extensions). Sorry about that.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/extensions/manifest_app.html
(right):

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/extensions/manifest_app.html:1:
{{+partials.standard_extensions_article article:intros.manifest_app}}
it'd be nice to also put these in a subdirectory but we'd need to make sure that
the links are safe (e.g. have appropriate ../.. in them).

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

ok, review done. plenty of things to do.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system_source.py:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
(c) 2012 -> 2013. Check other files too.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system_source.py:7: class
CompiledFileSystemSource(object):
Anything more complicated than the Identity function here would certainly call
for a specialised data source, so if anything this should be IdentityDataSource.

However, it's just as likely that we'll want to modify the data in some way - so
I'd prefer creating a separate ManifestDataSource rather than trying to be
generic.

When you do that, write some basic unit test for ManifestDataSource (see comment
in template data source test).

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:71: lambda _,
contents: json.loads(contents), TemplateDataSource)
Use compiled_fs_factory.CreateIdentity() here.

besides which, the data source should be responsible for creating its own data
store/compiled_fs's etc, which is another reason it needs to be its own
ManifestDataSource.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:122: #  
self._manifest_json)
delete?

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:139: def
testJSONDocumentationSources(self):
We should just fix https://code.google.com/p/chromium/issues/detail?id=147747
rather than writing this test, but that's a bit of a yak since there are almost
certainly existing broken links.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:24: "<a
href="{{documentation}}">{{name}}</a>": <em>{{*example}}</em>,
Surprising to see JSON here... seems like it should be just {{example}}?

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:25: {{/}}
Pull this into a partial and pass the manifest data in as an argument, so this
would look like

{{manifest.required}}
...
{{/manifest.required}}

etc.

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

One more thing - when https://codereview.chromium.org/12996003/ goes in we'll
want to show the same availability info for manifest files. It can be in a
follow-up, but keep it in mind.

[jshumway, 2013-05-03 03:44:39]

Here is the second preview version of the changes. I still need to make the URL
changes if we want them. I wasn't sure how to make changing the manifest.json
file between apps and extensions the most accessible, so if there is a better
way, let me know.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system_source.py:1: #
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/04/30 16:21:09, kalman wrote:
> (c) 2012 -> 2013. Check other files too.

Correct in new file.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system_source.py:7: class
CompiledFileSystemSource(object):
On 2013/04/30 16:21:09, kalman wrote:
> Anything more complicated than the Identity function here would certainly call
> for a specialised data source, so if anything this should be
IdentityDataSource.
> 
> However, it's just as likely that we'll want to modify the data in some way -
so
> I'd prefer creating a separate ManifestDataSource rather than trying to be
> generic.
> 
> When you do that, write some basic unit test for ManifestDataSource (see
comment
> in template data source test).

I replaced this class with ManifestDataSource. It isn't trivial enough for an
IdentityDataSource, but isn't complicated enough that adding unit tests would
actually test anything.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:71: lambda _,
contents: json.loads(contents), TemplateDataSource)
On 2013/04/30 16:21:09, kalman wrote:
> Use compiled_fs_factory.CreateIdentity() here.
> 
> besides which, the data source should be responsible for creating its own data
> store/compiled_fs's etc, which is another reason it needs to be its own
> ManifestDataSource.

Done.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:122: #  
self._manifest_json)
On 2013/04/30 16:21:09, kalman wrote:
> delete?

Done.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source_test.py:139: def
testJSONDocumentationSources(self):
On 2013/04/30 16:21:09, kalman wrote:
> We should just fix https://code.google.com/p/chromium/issues/detail?id=147747
> rather than writing this test, but that's a bit of a yak since there are
almost
> certainly existing broken links.

Good point, I have been noticing a lot of comments with the same message.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:24: "<a
href="{{documentation}}">{{name}}</a>": <em>{{*example}}</em>,
On 2013/04/30 16:21:09, kalman wrote:
> Surprising to see JSON here... seems like it should be just {{example}}?

I used this accessor so that we can write literal json as the keys of 'example'
and have it be rendered properly as example json. It is convenient for render
numbers without quotes, strings with them, dictionaries with nested types, etc,
all of which we have in the current example page. Of course, I could remove it
if you would prefer.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:25: {{/}}
On 2013/04/30 16:21:09, kalman wrote:
> Pull this into a partial and pass the manifest data in as an argument, so this
> would look like
> 
> {{manifest.required}}
> ...
> {{/manifest.required}}
> 
> etc.

Done.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/manifest_app.html:2: 
On 2013/04/27 01:54:02, kalman wrote:
> can you put these in articles/manifest/app.html,
> articles/manifest/browserAction.html, etc?

I did this and now IntroDataSource can handle looking up files in subdirectories
in general. I noticed the comment to change the name of this class in the source
code. I made some pretty extensive changes to it already, should I change the
name while I'm at it?

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/json/extensions_manifest.json:7: },
On 2013/04/27 01:54:02, kalman wrote:
> I actually think we'll only want one of these files both both apps and
> extensions, then the server can pull out what it needs (depending on whether
> it's looking at apps or extensions). Sorry about that.

Done. Would you prefer a different way to change the manifest file per apps or
extensions that applying functions in ManifestDataSource? (not exactly
accessible to tech writers)

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/extensions/manifest_app.html
(right):

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/extensions/manifest_app.html:1:
{{+partials.standard_extensions_article article:intros.manifest_app}}
On 2013/04/27 01:54:02, kalman wrote:
> it'd be nice to also put these in a subdirectory but we'd need to make sure
that
> the links are safe (e.g. have appropriate ../.. in them).

Just to clarify, I should make it such that the URL
server/extensions/manifest/name.html renders the template in
/public/extensions/manifest/name.html?

[not at google - send to devlin, 2013-05-03 15:56:24]

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system_source.py:7: class
CompiledFileSystemSource(object):
On 2013/05/03 03:44:39, jaredshumway94 wrote:
> On 2013/04/30 16:21:09, kalman wrote:
> > Anything more complicated than the Identity function here would certainly
call
> > for a specialised data source, so if anything this should be
> IdentityDataSource.
> > 
> > However, it's just as likely that we'll want to modify the data in some way
-
> so
> > I'd prefer creating a separate ManifestDataSource rather than trying to be
> > generic.
> > 
> > When you do that, write some basic unit test for ManifestDataSource (see
> comment
> > in template data source test).
> 
> I replaced this class with ManifestDataSource. It isn't trivial enough for an
> IdentityDataSource, but isn't complicated enough that adding unit tests would
> actually test anything.

Any amount of > 0 triviality is worth testing, and it's > 0 triviality. Besides
which it's nice to set things up for later tests anyway.

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:24: "<a
href="{{documentation}}">{{name}}</a>": <em>{{*example}}</em>,
On 2013/05/03 03:44:39, jaredshumway94 wrote:
> On 2013/04/30 16:21:09, kalman wrote:
> > Surprising to see JSON here... seems like it should be just {{example}}?
> 
> I used this accessor so that we can write literal json as the keys of
'example'
> and have it be rendered properly as example json. It is convenient for render
> numbers without quotes, strings with them, dictionaries with nested types,
etc,
> all of which we have in the current example page. Of course, I could remove it
> if you would prefer.

nope, the way you have it is good. I like it.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/extensions/manifest_app.html
(right):

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/extensions/manifest_app.html:1:
{{+partials.standard_extensions_article article:intros.manifest_app}}
On 2013/05/03 03:44:39, jaredshumway94 wrote:
> On 2013/04/27 01:54:02, kalman wrote:
> > it'd be nice to also put these in a subdirectory but we'd need to make sure
> that
> > the links are safe (e.g. have appropriate ../.. in them).
> 
> Just to clarify, I should make it such that the URL
> server/extensions/manifest/name.html renders the template in
> /public/extensions/manifest/name.html?

Yep. Transform manifest_foo.html into manifest/foo.html for all applicable. I
think you'll need to modify the sidebar code to give absolute paths (don't
forget to include the branch)

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:6: import functools
f < H

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:102:
self._identity_fs = compiled_fs_factory.CreateIdentity(IntroDataSource)
hm ok, I don't think any of the changes to this file are actually needed after I
submit https://codereview.chromium.org/14856006/ which changes preview.py to use
LocalFileSystem. The bug appears to be that something in the fake fetchers is
failing to resolve the /, whereas I just tested this and it works without
changes within that patch.

one thing to check though - that SubversionFileSystem can actually handle / in
paths. Could you add a test in there?

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:119: return
self._cache.GetFromFile(os.path.join(base_path, path))
caches always use '/' as a path separator, the classes that care about OS paths
(e.g. LocalFileSystem) convert it themselves.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:9: from
svn_constants import JSON_PATH
this is the kind of thing to pass into the constructor of ManifestDataSource to
make it easier to test.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:11: def
apply_apps_transformations(manifest):
should be _ApplyAppsTransformations

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:15: pass
almost there - but this needs to filter the manifest items according to whether
they're available to extensions vs apps, and for that these methods needs access
to the manifest feature file.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:23: lambda _,
contents: json.loads(contents), store_type)
the store_type variable is so that each class can guarantee its own area. don't
pass it in, it should be ManifestDataSource.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:28: if not
self.loaded:
CompiledFileSystem implements this loading logic itself (and combines it with
various caching to share it across instances).

I would expect to see instead of

lambda _, contents: json.loads(contents), store_type

above, instead a reference to a function which does everything in get() up to
actually returning the key.

self._compiled_fs = compiled_fs_factory.Create(
  _CreateManifestData, ManifestDataSource)

def _CreateManifestData(self_, contents):
  ...
  return {'apps': ..., 'extensions': ...}

def get(self):
  return self._compiled_fs.GetFromFile(...)[key]

then the KeyError happens automatically, too.

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

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:71:
compiled_fs_factory, TemplateDataSource)
pass this in from ServerInstance like the other dependencies.

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

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source_test.py:17: import
svn_constants
neither of these changes needed anymore

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/articles/apps_manifest.html
(right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:18: </p>
Stuff above here is shared with extensions_manifest, should go in
manifest_example?

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/manifest_properties.html
(right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/manifest_properties.html:3:
{{/}}
nit: multi line tags close off explicitly.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/manifest_properties.html:4: 
extra /n?

[jshumway, 2013-05-10 02:08:36]

The manifest example is being generated from both manifest.json and
_manifest_features now. Properties leftover in the features file are appended to
the appropriate example pages. URLs of the form /extensions/manifest/name.html
now work, and paths that were relative should now be properly absolute.

However, most of the /manifest/*.hmtl pages pulled out of the old manifest
example page are extremely short and have strange titles (with unix_style_names)
(e.g. /extensions/manifest/name.html).

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

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/compiled_file_system_source.py:7: class
CompiledFileSystemSource(object):
On 2013/05/03 15:56:24, kalman wrote:
> On 2013/05/03 03:44:39, jaredshumway94 wrote:
> > On 2013/04/30 16:21:09, kalman wrote:
> > > Anything more complicated than the Identity function here would certainly
> call
> > > for a specialised data source, so if anything this should be
> > IdentityDataSource.
> > > 
> > > However, it's just as likely that we'll want to modify the data in some
way
> -
> > so
> > > I'd prefer creating a separate ManifestDataSource rather than trying to be
> > > generic.
> > > 
> > > When you do that, write some basic unit test for ManifestDataSource (see
> > comment
> > > in template data source test).
> > 
> > I replaced this class with ManifestDataSource. It isn't trivial enough for
an
> > IdentityDataSource, but isn't complicated enough that adding unit tests
would
> > actually test anything.
> 
> Any amount of > 0 triviality is worth testing, and it's > 0 triviality.
Besides
> which it's nice to set things up for later tests anyway.

Done.

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/extensions/manifest_app.html
(right):

https://codereview.chromium.org/14273041/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/extensions/manifest_app.html:1:
{{+partials.standard_extensions_article article:intros.manifest_app}}
On 2013/05/03 15:56:24, kalman wrote:
> On 2013/05/03 03:44:39, jaredshumway94 wrote:
> > On 2013/04/27 01:54:02, kalman wrote:
> > > it'd be nice to also put these in a subdirectory but we'd need to make
sure
> > that
> > > the links are safe (e.g. have appropriate ../.. in them).
> > 
> > Just to clarify, I should make it such that the URL
> > server/extensions/manifest/name.html renders the template in
> > /public/extensions/manifest/name.html?
> 
> Yep. Transform manifest_foo.html into manifest/foo.html for all applicable. I
> think you'll need to modify the sidebar code to give absolute paths (don't
> forget to include the branch

I got absolute paths working for apps/extensions, but everything appears to be
/trunk with preview.py. I also changed the sample's link to be absolute.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:6: import functools
On 2013/05/03 15:56:25, kalman wrote:
> f < H

Done.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:102:
self._identity_fs = compiled_fs_factory.CreateIdentity(IntroDataSource)
On 2013/05/03 15:56:25, kalman wrote:
> hm ok, I don't think any of the changes to this file are actually needed after
I
> submit https://codereview.chromium.org/14856006/ which changes preview.py to
use
> LocalFileSystem. The bug appears to be that something in the fake fetchers is
> failing to resolve the /, whereas I just tested this and it works without
> changes within that patch.
> 
> one thing to check though - that SubversionFileSystem can actually handle / in
> paths. Could you add a test in there?

I reverted my changes to this file and everything worked. I also added the test.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:119: return
self._cache.GetFromFile(os.path.join(base_path, path))
On 2013/05/03 15:56:25, kalman wrote:
> caches always use '/' as a path separator, the classes that care about OS
paths
> (e.g. LocalFileSystem) convert it themselves.
My bad

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:9: from
svn_constants import JSON_PATH
On 2013/05/03 15:56:25, kalman wrote:
> this is the kind of thing to pass into the constructor of ManifestDataSource
to
> make it easier to test.

Done.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:11: def
apply_apps_transformations(manifest):
On 2013/05/03 15:56:25, kalman wrote:
> should be _ApplyAppsTransformations

Done.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:15: pass
On 2013/05/03 15:56:25, kalman wrote:
> almost there - but this needs to filter the manifest items according to
whether
> they're available to extensions vs apps, and for that these methods needs
access
> to the manifest feature file.

This was one of the major of my patch. Now the manifest.json file is checked and
paired with entries in _manifest_features. Features is consulted to check what
entries should be show on which branches. After the entries in manifest.json are
exhausted, the remaining entries in features are added to the bottom of the
manifest example as plain text (not links) with an example of '...'.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:23: lambda _,
contents: json.loads(contents), store_type)
On 2013/05/03 15:56:25, kalman wrote:
> the store_type variable is so that each class can guarantee its own area.
don't
> pass it in, it should be ManifestDataSource.

Done.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:28: if not
self.loaded:
On 2013/05/03 15:56:25, kalman wrote:
> CompiledFileSystem implements this loading logic itself (and combines it with
> various caching to share it across instances).
> 
> I would expect to see instead of
> 
> lambda _, contents: json.loads(contents), store_type
> 
> above, instead a reference to a function which does everything in get() up to
> actually returning the key.
> 
> self._compiled_fs = compiled_fs_factory.Create(
>   _CreateManifestData, ManifestDataSource)
> 
> def _CreateManifestData(self_, contents):
>   ...
>   return {'apps': ..., 'extensions': ...}
> 
> def get(self):
>   return self._compiled_fs.GetFromFile(...)[key]
> 
> then the KeyError happens automatically, too.

Done.

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/manifest_properties.html
(right):

https://codereview.chromium.org/14273041/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/manifest_properties.html:3:
{{/}}
On 2013/05/03 15:56:25, kalman wrote:
> nit: multi line tags close off explicitly.

Done.

[not at google - send to devlin, 2013-05-10 04:14:14]

Had to rush a bit at the end there, mostly little things. It looks fantastic.

One last thing, I couldn't find where in the code to add it - it'd be nice to
show in the page headers like

Manifest - app

rather than

app

which looks a bit out of context.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:1: # Copyright (c)
2013 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:6: import json
pls use third_party.json_schema_compiler.json_parse

also means you won't need the Nom'ing, much as it's a cool name.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:14: def
__init__(self, compiled_fs_factory, manifest_path, features_path, channel):
line is too long

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:22: # Used to
cache the final apps and extensions manifest json objects.
None of these comments add much IMO.

Three comments with the cached file system:
(1) unless they have category= they'll share the same namespace, which would be
bad, so they need a category=.
(2) perhaps json_cache rather than compiled_fs.
(3) 4 space indent when wrapping lines, not 2.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:25: 
no \n

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:29:
manifest['required'][0]['example'] = 'My Application'
just App

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:37: """
how does this make testing easier? what does it have to do with manifest
specifically?

in any case... please use json_parse from json_schema_compiler here. It gets the
OrderedDict treatment, which may become important.

This method probably isn't much value then.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:45: features_json
= self._compiled_fs.GetFromFile(self._features_path)
... that said about the caches, I don't think this extra cache is actually doing
anything, since this method will only ever be called once and the result will be
cached.

I.e. reading from the file system would be just as good, and put less strain on
memcache.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:49: channels =
{"stable": 0, "beta": 1, "dev": 2, "trunk": 3}
try to use ' not " for strings. Less RSI.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:50: app_types =
frozenset(['packaged_app', 'hosted_app', 'platform_app'])
Just platform_app.

I guess this logic will become simpler.

Generally though, if you're creating an immutable list then use tuples not lists
i.e. frozenset((..., ..., ...))

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:69: return added
Also as I mentioned in IRC - we will need to group nested features, e.g. render
app.background as though it's inside app (you can imagine how that should look).

But do it in a follow-up. There will be some trickier things like what to do
about it in terms of required vs not-required. Maybe it should all be done
inside manifest.json rather than parsing the features file?

Let's decide later.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:73: for mproperty
in manifest_json[property_type]:
mproperty -> manifest_key?

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:78: if
channels[feature['channel']] <= channels[self._channel]:
interesting. This is something we'll need to think about how to surface when we
only show 'trunk'. For now, I'm not sure whether it's worth the extra code,
it'll probably change when Evan's availability stuff is finished.

So I reckon just show them no matter what. In a follow-up we can think about
generating a comment like "dev only" and/or showing a warning in the manifest
overview pages.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:80: del
features_json[mproperty['name']]
makes me nervous, better to take a copy of it before you mutate objects that are
owned by other things - but since this will come from a file system later,
that's fine.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:83: for feature in
sorted(features_json.keys()):
as I mentioned - it looks a bit odd the way it is with half of the optional
things sorted then the other half sorted. Just combine them into one.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:85: # In two
cases, a features_json entry is a list for no obvious reason.
it represents union :) one is usually for whitelisting. You can ignore that one.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source_test.py (right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:12: class
Factory(object):
It doesn't look like having a Factory here achieves anything. Constructor is
fine.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:88:
MockCompiledFileSystem.Factory(), 'manifest_path', 'features_path',
4 space

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:91: # Prevent
an actual file load by patching in our load function.
cool test data, but the test object store / file system / etc infrastructure
should be powerful enough to not require overriding private methods.

E.g. you should use a TestFileSystem with data pointing at the right place
(perhaps you need to write the dicts to JSON which the data source expects),
like

file_system = TestFileSystem({
  'manifest.json': json.dumps(...)
  '_manifest_features.jon': json.dumps(...)
})
compiled_fs_factory = CompiledFileSystem.Factory(
    file_system, ObjectStoreCreator.ForTest())
mds = ManifestDataSource(compiled_fs_factory, 'manifest.json',
'_manifest_features.json', 'dev')

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

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:91:
os.path.join(svn_constants.JSON_PATH, 'manifest.json'),
'/'.join, these are URL paths not os paths.

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

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:28: # else:
what is this? getting late, running out of brainpower.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:55: "name": "app"
this should move up to required

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/manifest_properties.html
(right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/manifest_properties.html:4:
{{:}}
:documentation

[not at google - send to devlin, 2013-05-10 05:39:14]

Two more things:
- please make sure to update the version in app.yaml. there should be a big
presubmit warning soon.
- make sure you update the manifest content before submitting, there have been
changes to manifest.html in the meantime.

[not at google - send to devlin, 2013-05-10 16:04:16]

Hey Meggin - are you around?

What do you think of this change? It pulls all of the manifest docs into
separate files and out of that giant monolithic manifest.html file. However,
some of them look a bit odd because they're only a single line.

I've convinced myself that this is ok and that having them inline is worse, but
WDYT?

[jshumway, 2013-05-11 02:37:08]

Here are the changes you asked for. The patch is looking pretty good.

One interesting problem I came across: to look up a manifest URL, the server
searches through public/{apps,extensions}/manifest for a small "forwarding"
template that really just calls a partial to do most of its work. I put a bunch
of new files in these two directories, one for each entry in manifest.json.
However, according to _manifest_features.json, not all of the files are needed
by both. So currently, /extensions/manifest/kiosk_enabled.html is a valid URL,
though it isn't actually listed anywhere.

Also, I had some trouble syncing today so I eventually gave up, cancelled the
sync, and started working on the patch. This made the merges pretty ugly, and
git-cl though I changed a bunch of C++ files I hadn't. I think I found a way to
get my patch uploaded properly, but if anything is strange, let me know and I
will try to sort it out. Git-cl was also giving me an error at the end of an
upload, despite the fact that it stealth-committed my changes in the background.
That is why I accidentally uploaded twice. My bad.

Hope everything looks good. If there is anything that needs to be changed before
IO, let me know and I can come in tomorrow to fix it.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:1: # Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/05/10 04:14:14, kalman wrote:
> no (c)

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:6: import json
On 2013/05/10 04:14:14, kalman wrote:
> pls use third_party.json_schema_compiler.json_parse
> 
> also means you won't need the Nom'ing, much as it's a cool name.

It is a cool function name, changed.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:14: def
__init__(self, compiled_fs_factory, manifest_path, features_path, channel):
On 2013/05/10 04:14:14, kalman wrote:
> line is too long

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:22: # Used to
cache the final apps and extensions manifest json objects.
On 2013/05/10 04:14:14, kalman wrote:
> None of these comments add much IMO.
> 
> Three comments with the cached file system:
> (1) unless they have category= they'll share the same namespace, which would
be
> bad, so they need a category=.
> (2) perhaps json_cache rather than compiled_fs.
> (3) 4 space indent when wrapping lines, not 2.

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:25: 
On 2013/05/10 04:14:14, kalman wrote:
> no \n

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:29:
manifest['required'][0]['example'] = 'My Application'
On 2013/05/10 04:14:14, kalman wrote:
> just App

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:37: """
On 2013/05/10 04:14:14, kalman wrote:
> how does this make testing easier? what does it have to do with manifest
> specifically?
> 
> in any case... please use json_parse from json_schema_compiler here. It gets
the
> OrderedDict treatment, which may become important.
> 
> This method probably isn't much value then.

Removed it.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:45: features_json
= self._compiled_fs.GetFromFile(self._features_path)
On 2013/05/10 04:14:14, kalman wrote:
> ... that said about the caches, I don't think this extra cache is actually
doing
> anything, since this method will only ever be called once and the result will
be
> cached.
> 
> I.e. reading from the file system would be just as good, and put less strain
on
> memcache.

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:49: channels =
{"stable": 0, "beta": 1, "dev": 2, "trunk": 3}
On 2013/05/10 04:14:14, kalman wrote:
> try to use ' not " for strings. Less RSI.

My mistake

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:50: app_types =
frozenset(['packaged_app', 'hosted_app', 'platform_app'])
On 2013/05/10 04:14:14, kalman wrote:
> Just platform_app.
> 
> I guess this logic will become simpler.
> 
> Generally though, if you're creating an immutable list then use tuples not
lists
> i.e. frozenset((..., ..., ...))

Changed.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:73: for mproperty
in manifest_json[property_type]:
On 2013/05/10 04:14:14, kalman wrote:
> mproperty -> manifest_key?

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:78: if
channels[feature['channel']] <= channels[self._channel]:
On 2013/05/10 04:14:14, kalman wrote:
> interesting. This is something we'll need to think about how to surface when
we
> only show 'trunk'. For now, I'm not sure whether it's worth the extra code,
> it'll probably change when Evan's availability stuff is finished.
> 
> So I reckon just show them no matter what. In a follow-up we can think about
> generating a comment like "dev only" and/or showing a warning in the manifest
> overview pages.

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:80: del
features_json[mproperty['name']]
On 2013/05/10 04:14:14, kalman wrote:
> makes me nervous, better to take a copy of it before you mutate objects that
are
> owned by other things - but since this will come from a file system later,
> that's fine.

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:83: for feature in
sorted(features_json.keys()):
On 2013/05/10 04:14:14, kalman wrote:
> as I mentioned - it looks a bit odd the way it is with half of the optional
> things sorted then the other half sorted. Just combine them into one.

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:85: # In two
cases, a features_json entry is a list for no obvious reason.
On 2013/05/10 04:14:14, kalman wrote:
> it represents union :) one is usually for whitelisting. You can ignore that
one.

I changed the comment.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source_test.py (right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:12: class
Factory(object):
On 2013/05/10 04:14:14, kalman wrote:
> It doesn't look like having a Factory here achieves anything. Constructor is
> fine.

Replaced all of this stuff with the project's test fixtures.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:88:
MockCompiledFileSystem.Factory(), 'manifest_path', 'features_path',
On 2013/05/10 04:14:14, kalman wrote:
> 4 space

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:91: # Prevent
an actual file load by patching in our load function.
On 2013/05/10 04:14:14, kalman wrote:
> cool test data, but the test object store / file system / etc infrastructure
> should be powerful enough to not require overriding private methods.
> 
> E.g. you should use a TestFileSystem with data pointing at the right place
> (perhaps you need to write the dicts to JSON which the data source expects),
> like
> 
> file_system = TestFileSystem({
>   'manifest.json': json.dumps(...)
>   '_manifest_features.jon': json.dumps(...)
> })
> compiled_fs_factory = CompiledFileSystem.Factory(
>     file_system, ObjectStoreCreator.ForTest())
> mds = ManifestDataSource(compiled_fs_factory, 'manifest.json',
> '_manifest_features.json', 'dev')

Fixed them up. I still override the two Apply*Transformations functions which
shouldn't be part of the tests because they don't have a specification/change on
the fly depending on how we want to change the content.

Thanks for the code example, it saved me a lot of time trying to figure out the
best way to set up test fixtures.

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

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:91:
os.path.join(svn_constants.JSON_PATH, 'manifest.json'),
On 2013/05/10 04:14:14, kalman wrote:
> '/'.join, these are URL paths not os paths.

Done.

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

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:28: # else:
On 2013/05/10 04:14:14, kalman wrote:
> what is this? getting late, running out of brainpower.

This makes an absolute base URL available to all the templates. I needed a base
URL to make all of the sidenav links, and the sample link at the top of the
page, absolute. Being at /extensions/manifest/something.html would break all the
links on the page. This fixes that. It all adds the channel name to apps or
extensions, which might not be needed anymore. The commented out else is
something I meant to remove.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:55: "name": "app"
On 2013/05/10 04:14:14, kalman wrote:
> this should move up to required

Done.

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/manifest_properties.html
(right):

https://codereview.chromium.org/14273041/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/manifest_properties.html:4:
{{:}}
On 2013/05/10 04:14:14, kalman wrote:
> :documentation

Done.

[not at google - send to devlin, 2013-05-11 18:03:06]

> One interesting problem I came across: to look up a manifest URL, the server
> searches through public/{apps,extensions}/manifest for a small "forwarding"
> template that really just calls a partial to do most of its work. I put a
bunch
> of new files in these two directories, one for each entry in manifest.json.
> However, according to _manifest_features.json, not all of the files are needed
> by both. So currently, /extensions/manifest/kiosk_enabled.html is a valid URL,
> though it isn't actually listed anywhere.

Yep good point, I hadn't thought of that. The approach here should be the same
as with APIs - we shouldn't have public html files for things that don't exist,
so there should be no kiosk_enabled.html under extensions/. Ditto all the others
of course. Sounds scriptable...

Now to review the rest.

[not at google - send to devlin, 2013-05-11 18:32:01]

I agree you're very close!

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-1-1
As per my new verbose warning on presubmit, this should be 2-2-0.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:39: extensions =
defaultdict(list)
defaultdict isn't ordered right? I think that's a requirement. But it's almost
as easy?

def create_dict():
  d = OrderedDict()  # from json_parse
  for category in ['required', 'only_one', 'optional']:
    d[category] = []
  return d
apps = create_dict()
extensions = create_dict()

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:41: def
add_property(feature, manifest_key, type_='optional'):
just explicitly pass in 'optional' for type_ below, this isn't an idiomatically
optional parameter.

also type_ is a distressingly misleading name for this, type_ typically refers
to, you know, int/float/whatever. Maybe category.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:58: return added
this could all be more concise:

added = False
extension_types = feature['extension_types']
if extension_types == 'all' or 'platform_app' in extension_types:
  apps[category].append(manifest_key)
if extension_types == 'all' or 'extension' in extension_types:
  extensions[category].append(manifest_key)
return added

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:61: for
property_type in manifest_json:
I think my objection to type here is the same.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:80:
apps['optional'] = sorted(apps['optional'], key=lambda d: d['name'])
from operator import itemgetter
...
apps['optional'].sort(key=itemgetter('name'))
extensions['optional'].sort(key=itemgetter('name'))

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source_test.py (right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:89:
mds._ApplyExtensionsTransformations = types.MethodType(Nothing, mds)
pls never override private methods. it greatly reduces the usefulness of tests
since it enforces < 100% code coverage, which is what you should always aim for
in tests.

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:90:
'/'.join([svn_constants.JSON_PATH, 'manifest.json']),
join([svn_constants.JSON_PATH, 'manifest.json']) ->
join((svn_constants.JSON_PATH, 'manifest.json'))

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:27:
channel_dict['base_url'] = '/%s/%s/' % (channel_name, splitpath[0])
Ok I think this is the wrong place for this. sidenav_data_source should just
normalise the data into proper links, and detect whether they're absolute or
relative (easy enough).

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:79:
_MakeChannelDict(self.channel_name, path),
I preferred the way it was with storing the _branch_info, but if there's some
reason I'm not seeing for changing it, then this should be self._channel_name
not self.channel_name.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/extensions_sidenav.json
(right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/extensions_sidenav.json:209:
"absolute_url": true,
I expect you'd need to do a similar thing in apps_sidenav? But anyway, see
comment in template_data_source. sidenav_data_source can just normalise this
data.

I guess you'd probably change the data representation here to be "url" rather
than "fileName".

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:36: features_json
= Parse(self._local_file_system.Read(
ReadSingle. And to answer your question in IRC - yes this is right, though
local_file_system is a misleading name. Just file_system is fine.

[jshumway, 2013-05-11 22:08:19]

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-1-1
On 2013/05/11 18:32:01, kalman wrote:
> As per my new verbose warning on presubmit, this should be 2-2-0.

Done.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:36: features_json
= Parse(self._local_file_system.Read(
On 2013/05/11 20:42:04, kalman wrote:
> ReadSingle. And to answer your question in IRC - yes this is right, though
> local_file_system is a misleading name. Just file_system is fine.

Done.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:39: extensions =
defaultdict(list)
On 2013/05/11 18:32:01, kalman wrote:
> defaultdict isn't ordered right? I think that's a requirement. But it's almost
> as easy?
> 
> def create_dict():
>   d = OrderedDict()  # from json_parse
>   for category in ['required', 'only_one', 'optional']:
>     d[category] = []
>   return d
> apps = create_dict()
> extensions = create_dict()

Done.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:41: def
add_property(feature, manifest_key, type_='optional'):
On 2013/05/11 18:32:01, kalman wrote:
> just explicitly pass in 'optional' for type_ below, this isn't an
idiomatically
> optional parameter.
> 
> also type_ is a distressingly misleading name for this, type_ typically refers
> to, you know, int/float/whatever. Maybe category.

Sorry, that was a poor choice of name.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:58: return added
On 2013/05/11 18:32:01, kalman wrote:
> this could all be more concise:
> 
> added = False
> extension_types = feature['extension_types']
> if extension_types == 'all' or 'platform_app' in extension_types:
>   apps[category].append(manifest_key)
> if extension_types == 'all' or 'extension' in extension_types:
>   extensions[category].append(manifest_key)
> return added

Done.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:61: for
property_type in manifest_json:
On 2013/05/11 18:32:01, kalman wrote:
> I think my objection to type here is the same.

Done.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:80:
apps['optional'] = sorted(apps['optional'], key=lambda d: d['name'])
On 2013/05/11 18:32:01, kalman wrote:
> from operator import itemgetter
> ...
> apps['optional'].sort(key=itemgetter('name'))
> extensions['optional'].sort(key=itemgetter('name'))

I had never heard of that module before, I have a feeling I'll start using it
more often.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source_test.py (right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:89:
mds._ApplyExtensionsTransformations = types.MethodType(Nothing, mds)
On 2013/05/11 18:32:01, kalman wrote:
> pls never override private methods. it greatly reduces the usefulness of tests
> since it enforces < 100% code coverage, which is what you should always aim
for
> in tests.

Fixed that.

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:90:
'/'.join([svn_constants.JSON_PATH, 'manifest.json']),
On 2013/05/11 18:32:01, kalman wrote:
> join([svn_constants.JSON_PATH, 'manifest.json']) ->
> join((svn_constants.JSON_PATH, 'manifest.json'))

Done.

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

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:27:
channel_dict['base_url'] = '/%s/%s/' % (channel_name, splitpath[0])
On 2013/05/11 18:32:01, kalman wrote:
> Ok I think this is the wrong place for this. sidenav_data_source should just
> normalise the data into proper links, and detect whether they're absolute or
> relative (easy enough).

I went with the changes to *_sidenav.json and removed these changes to
MakeChannelDict.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:79:
_MakeChannelDict(self.channel_name, path),
On 2013/05/11 18:32:01, kalman wrote:
> I preferred the way it was with storing the _branch_info, but if there's some
> reason I'm not seeing for changing it, then this should be self._channel_name
> not self.channel_name.

Done.

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/extensions_sidenav.json
(right):

https://codereview.chromium.org/14273041/diff/36001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/extensions_sidenav.json:209:
"absolute_url": true,
On 2013/05/11 18:32:01, kalman wrote:
> I expect you'd need to do a similar thing in apps_sidenav? But anyway, see
> comment in template_data_source. sidenav_data_source can just normalise this
> data.
> 
> I guess you'd probably change the data representation here to be "url" rather
> than "fileName".

I did a git-cl patch with 15054006 to get the changes you made to the json
files, so they aren't part of this patch.

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

lgtm with a couple more comments

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:11: def
create_dict():
if a function is only relevant to a couple of local variables I typically
declare it above them, if it's at this scope it needs to be _CreateDict and
probably a less generic name as well.

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:22: features_path,
channel):
channel not needed anymore

also if things don't fix on 1 line then 1 line per argument pls.

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:42: features_json
= Parse(self._file_system.Read(
ReadSingle

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source_test.py (right):

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:94: }
pls augment the test so that all of the cases are covered i.e. make sure there
are some entries in recommended/only_one for both apps and extensions.

[jshumway, 2013-05-11 22:50:41]

Got the final changes, merged in the json files.

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:11: def
create_dict():
On 2013/05/11 22:19:04, kalman wrote:
> if a function is only relevant to a couple of local variables I typically
> declare it above them, if it's at this scope it needs to be _CreateDict and
> probably a less generic name as well.

Done.

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:22: features_path,
channel):
On 2013/05/11 22:19:04, kalman wrote:
> channel not needed anymore
> 
> also if things don't fix on 1 line then 1 line per argument pls.

Done.

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:42: features_json
= Parse(self._file_system.Read(
On 2013/05/11 22:19:04, kalman wrote:
> ReadSingle

Done.

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source_test.py (right):

https://codereview.chromium.org/14273041/diff/44001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source_test.py:94: }
On 2013/05/11 22:19:04, kalman wrote:
> pls augment the test so that all of the cases are covered i.e. make sure there
> are some entries in recommended/only_one for both apps and extensions.

Done, I removed the channel attributes in the fake manifest_features stuff
because they are no longer relevant to manifest_data_source.

[commit-bot: I haz the power, 2013-05-11 22:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jaredshumway94@gmail.com/14273041/46003

[not at google - send to devlin, 2013-05-12 03:06:59]

Committed patchset #8 manually as r199633.

[not at google - send to devlin, 2013-05-12 03:14:22]

Sorry I thought if I committed it myself it would say "patch by..." but....
apparently not! Sorry :(

I wanted to get this up and running tonight and the tree aint going green.

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

It turns out we can't submit this patch yet, sorry - we're blocked on only
showing trunk docs, and for that we're blocked on Evan's patch, so that only
showing trunk docs isn't super confusing.

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

https://codereview.chromium.org/14273041/diff/46003/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:89:
LocalFileSystem.Create(),
sorry for not picking this up, but this should be host_file_system not
LocalFileSystem.

It's not why I reverted the patch though.

[jshumway, 2013-05-14 05:25:00]

I went ahead and changed that line. I didn't change the app,cron.yaml files
though, because they have already been incremented.

[jshumway, 2013-07-20 00:17:43]

Updated this patch for the new channel-less doc server. Turns out there were a
ton of broken links! Fixed them and we are down to 124 broken links on the doc
server, yay.

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

lgtm but a couple of things to fix up and 1 question.

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

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:115: 
where did this empty line come from?

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/subversion_file_system_test.py
(right):

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/subversion_file_system_test.py:64: # test.
Compare.
What were these changes for? I can't remember.

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

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:25: def
_MakeChannelDict(channel_name):
No I deleted it :)

[jshumway, 2013-07-22 17:16:27]

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

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:115: 
On 2013/07/20 02:49:03, kalman wrote:
> where did this empty line come from?

I think something strange happened in the conflict resolution stage. Removed.

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/subversion_file_system_test.py
(right):

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/subversion_file_system_test.py:64: # test.
Compare.
On 2013/07/20 02:49:03, kalman wrote:
> What were these changes for? I can't remember.

The first small block 44-48 came form your comment

> one thing to check though - that SubversionFileSystem can actually handle / in
paths. Could you add a test in there?

This block 64-98 is an error and has been removed. (I had to do some weird stuff
with git and I think this was a byproduct of that.)

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

https://codereview.chromium.org/14273041/diff/91001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source.py:25: def
_MakeChannelDict(channel_name):
On 2013/07/20 02:49:03, kalman wrote:
> No I deleted it :)

Ah, oops. Was in a bit of a rush to get it up and missed one of my notes.
Removed it.

[commit-bot: I haz the power, 2013-07-22 17:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jaredshumway94@gmail.com/14273041/104001

[commit-bot: I haz the power, 2013-07-22 17:20:37]

Change committed as 212903