Change the existing extension samples page to an automatically-generated searchable directory.

Change the existing extension samples page to an automatically-generated searchable directory.

 * Fixes a typo in the maps_app sample that prevents the manifest from being parsed as valid JSON.
 * Adds code to build a samples.json file in the documentation root, for use by the jstemplate docs rendering process.
 * Adds build/sample.py, a class which parses an individual sample's metadata out of the filesystem
 * Adds css/samples.css for directory-specific styles (could be merged into base styles if this is an issue)
 * Adds images/sample-default-icon.png for samples which don't have 128px icons.
 * Adds js/sample_search.js as the source for the sample search code (was difficult to escape this through the JSTemplate code without being in a different file).

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55111

[kurrik.chromium, 2010-07-14 00:07:14]

Here is the long-awaited CL to generate a samples directory as part of the
documentation build process.

[rafaelw, 2010-07-16 19:05:55]

http://codereview.chromium.org/2957009/diff/5001/6001
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/2957009/diff/5001/6001#newcode151
chrome/common/extensions/docs/build/build.py:151: def LoadAPIManifest():
My python is pretty weak. Is there a way to move most of this out to the
sample.py? It'd be nice to keep this file lean & mean.

http://codereview.chromium.org/2957009/diff/5001/6006
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/2957009/diff/5001/6006#newcode211
chrome/common/extensions/docs/js/api_page_generator.js:211: // If rendering the
samples page, add the samples manifest data + API modules.
I think it'd be nice if we could expose a generic way for pages to do some
per-page additional setup work like this.

What about having api_page_generator look for window.preRender() here and call
it if it is defined. have the static page (/static/samples.html, in this case)
include script that defines preRender() which does this below. If you call it
here, I think it should be defined and usable, but maybe I haven't given it a
try.

What do you think? If you like the idea, please comment here (where it's used)
what the expected interface is to this "plugin" mechanism.

http://codereview.chromium.org/2957009/diff/5001/6006#newcode216
chrome/common/extensions/docs/js/api_page_generator.js:216:
schema.each(function(mod) {
now that v8 has forEach & map, while you're in here, can you kill the
Array.prototype.each/.map and change the call-sites for forEach

[kurrik.chromium, 2010-07-31 01:04:58]

http://codereview.chromium.org/2957009/diff/5001/6001
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/2957009/diff/5001/6001#newcode151
chrome/common/extensions/docs/build/build.py:151: def LoadAPIManifest():
On 2010/07/16 19:05:56, rafaelw wrote:
> My python is pretty weak. Is there a way to move most of this out to the
> sample.py? It'd be nice to keep this file lean & mean.

Done.

http://codereview.chromium.org/2957009/diff/5001/6006
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/2957009/diff/5001/6006#newcode211
chrome/common/extensions/docs/js/api_page_generator.js:211: // If rendering the
samples page, add the samples manifest data + API modules.
Still looking into a good way to do this, will follow up with another update to
the review.

On 2010/07/16 19:05:56, rafaelw wrote:
> I think it'd be nice if we could expose a generic way for pages to do some
> per-page additional setup work like this.
> 
> What about having api_page_generator look for window.preRender() here and call
> it if it is defined. have the static page (/static/samples.html, in this case)
> include script that defines preRender() which does this below. If you call it
> here, I think it should be defined and usable, but maybe I haven't given it a
> try.
> 
> What do you think? If you like the idea, please comment here (where it's used)
> what the expected interface is to this "plugin" mechanism.

http://codereview.chromium.org/2957009/diff/5001/6006#newcode216
chrome/common/extensions/docs/js/api_page_generator.js:216:
schema.each(function(mod) {
On 2010/07/16 19:05:56, rafaelw wrote:
> now that v8 has forEach & map, while you're in here, can you kill the
> Array.prototype.each/.map and change the call-sites for forEach

Done.

http://codereview.chromium.org/2957009/diff/12001/13001
File chrome/common/extensions/docs/build/build.py (right):

http://codereview.chromium.org/2957009/diff/12001/13001#newcode154
chrome/common/extensions/docs/build/build.py:154:
parser.add_option("--page-name", dest="page_name")
I added an option here to render a single page if you need to build a page for
testing (so you don't have to wait for the whole build)

[kurrik.chromium, 2010-07-31 01:56:05]

Declaring a preRender function directly on the page or from an included script
didn't appear to work correctly, so I came up with a system that allows a page
to define a script block that is specially parsed and executed before the
template is rendered.  The method to use this functionality is documented in the
source of the update I just made to this review.  Let me know if you think it's
appropriate.

[rafaelw, 2010-08-02 22:13:03]

awesome. nice solution. lgtm modulo python code. thanks again for doing this.

antony can you back me up on the python? the build script is the only python
i've written and that was nearly a year ago =-)

[asargent_no_longer_on_chrome, 2010-08-03 16:48:56]

LGTM with a few nits and suggestions.

http://codereview.chromium.org/2957009/diff/18001/19002
File chrome/common/extensions/docs/build/directory.py (right):

http://codereview.chromium.org/2957009/diff/18001/19002#newcode30
chrome/common/extensions/docs/build/directory.py:30: methods: A string like
'chrome.foo.bar' or 'chrome.experimental.foo.onBar'
nit: you call this "method" in the function signature, but "methods" in the doc
here

http://codereview.chromium.org/2957009/diff/18001/19002#newcode89
chrome/common/extensions/docs/build/directory.py:89: return api_dict
nit: consider asserting that key is either "functions" or "events" since those
are the only two legal values

http://codereview.chromium.org/2957009/diff/18001/19002#newcode297
chrome/common/extensions/docs/build/directory.py:297: if placeholder_match is
not None:
nit: could this just be "if placeholder_match:" ?

http://codereview.chromium.org/2957009/diff/18001/19002#newcode384
chrome/common/extensions/docs/build/directory.py:384: api_calls.append(method)
Consider changing api_calls to a set - then this could just be:

if (code_contents.find(method) > -1):
  api_calls.add(method)

http://codereview.chromium.org/2957009/diff/18001/19002#newcode386
chrome/common/extensions/docs/build/directory.py:386: return api_calls
nit: consider writing as 

 return sorted(api_calls)

I think you can do this a few other places in this file as well.

(I'm not sure what version sorted was added to python, but it's in my cygwin
2.5.2 installation)

http://codereview.chromium.org/2957009/diff/18001/19002#newcode404
chrome/common/extensions/docs/build/directory.py:404: ext =
file_name[ext_start:]
You might be able to collapse the above 4 lines into just

ext = os.path.splitext(file_name)[1]

Then you could also remove the leading '.' from the '.html', 'json',...  list
below.

http://codereview.chromium.org/2957009/diff/18001/19002#newcode510
chrome/common/extensions/docs/build/directory.py:510: (self._manifest_path,
msg))
This pattern of "open file, read contents, parse as json, get results" with
exception handling seems to be repeated a couple of times. Maybe push into a
method like parse_json(filename)?

Also, you can probably use json.load instead of json.loads to avoid having to
open the file yourself, and maybe then you don't even need a separate method?

[kurrik.chromium, 2010-08-03 23:26:53]

Thanks for the review.  I've made your suggested changes.

http://codereview.chromium.org/2957009/diff/18001/19002
File chrome/common/extensions/docs/build/directory.py (right):

http://codereview.chromium.org/2957009/diff/18001/19002#newcode30
chrome/common/extensions/docs/build/directory.py:30: methods: A string like
'chrome.foo.bar' or 'chrome.experimental.foo.onBar'
On 2010/08/03 16:48:56, Antony Sargent wrote:
> nit: you call this "method" in the function signature, but "methods" in the
doc
> here

Done.

http://codereview.chromium.org/2957009/diff/18001/19002#newcode89
chrome/common/extensions/docs/build/directory.py:89: return api_dict
On 2010/08/03 16:48:56, Antony Sargent wrote:
> nit: consider asserting that key is either "functions" or "events" since those
> are the only two legal values

Done.

http://codereview.chromium.org/2957009/diff/18001/19002#newcode297
chrome/common/extensions/docs/build/directory.py:297: if placeholder_match is
not None:
On 2010/08/03 16:48:56, Antony Sargent wrote:
> nit: could this just be "if placeholder_match:" ?

Done.

http://codereview.chromium.org/2957009/diff/18001/19002#newcode384
chrome/common/extensions/docs/build/directory.py:384: api_calls.append(method)
On 2010/08/03 16:48:56, Antony Sargent wrote:
> Consider changing api_calls to a set - then this could just be:
> 
> if (code_contents.find(method) > -1):
>   api_calls.add(method)
> 
> 

Done.

http://codereview.chromium.org/2957009/diff/18001/19002#newcode386
chrome/common/extensions/docs/build/directory.py:386: return api_calls
Apparently added in 2.4 - if that's not OK, then I'll remove the change.

On 2010/08/03 16:48:56, Antony Sargent wrote:
> nit: consider writing as 
> 
>  return sorted(api_calls)
> 
> I think you can do this a few other places in this file as well.
> 
> (I'm not sure what version sorted was added to python, but it's in my cygwin
> 2.5.2 installation)

http://codereview.chromium.org/2957009/diff/18001/19002#newcode404
chrome/common/extensions/docs/build/directory.py:404: ext =
file_name[ext_start:]
On 2010/08/03 16:48:56, Antony Sargent wrote:
> You might be able to collapse the above 4 lines into just
> 
> ext = os.path.splitext(file_name)[1]
> 
> Then you could also remove the leading '.' from the '.html', 'json',...  list
> below.

Done.

http://codereview.chromium.org/2957009/diff/18001/19002#newcode510
chrome/common/extensions/docs/build/directory.py:510: (self._manifest_path,
msg))
Pushed this out into its own method, but I kept the wrapper so I could be
explicit in case something went wrong.

On 2010/08/03 16:48:56, Antony Sargent wrote:
> This pattern of "open file, read contents, parse as json, get results" with
> exception handling seems to be repeated a couple of times. Maybe push into a
> method like parse_json(filename)?
> 
> Also, you can probably use json.load instead of json.loads to avoid having to
> open the file yourself, and maybe then you don't even need a separate method?
> 

Done.

[asargent_no_longer_on_chrome, 2010-08-05 18:51:13]

(Just noticed this review is still open - in case it wasn't clear, on extensions
team we often have the convention of saying "lgtm with these
nits/suggestions..." meaning that we trust you'll address those and you can go
ahead and submit without needing to get a final lgtm)