Extensions Docs Server: Samples zip files

Extensions Docs Server: Samples zip files

Implemented a zip_data_source which will zip a directory given to it. This
required some changes to LocalFetcher so it would behave like SubversionFetcher
when given a path to a directory.

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

[cduvall, 2012-07-10 23:00:16]

Here's what I have for downloading zip files. Go to samples.html for a few
tests.

[not at google - send to devlin, 2012-07-11 00:35:09]

Mostly high-level comments; but it will involve a bit of restructuring.

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/echo_handler.py:4: # found in the LICENSE
file.
At what point do we rename this file from echo_handler to something that gives
it more credit?

(not in this patch, but yeah...)

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/echo_handler.py:36: ZIP_IGNORE_PATH =
EXTENSIONS_PATH + '/' + DOCS_PATH
The ignore path is prefix that should be stripped from requests or something?
I'm confused. From looking at it, I thought it was something to do with
blacklisting a directory.

If we put the listing of files on Fetcher then this code will change anyway, and
I don't think we'll need it anymore?

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/local_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/local_fetcher.py:9: a SubversionFetcher
would give.
I think this will be unnecessary if listing files is incorporated into the
Fetcher interface.

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/server_instance.py:33: if
path.endswith('.zip'):
I'd be more comfortable if this was restricted specifically to the examples
directory (injected of course).

Maybe a glob like "%s/**/*.zip" % examples_dir (there's a glob library
somewhere). Or whatever glob refers to "directory under examples that's not the
toplevel directory".

We also need to check for a manifest.json in that directory before we zip it,
and make sure we return None if it isn't found.

A job for another class obviously; perhaps ZipDataSource can turn into that.
Call like ExampleZipper or something (hehe).

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/subversion_data_source.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/subversion_data_source.py:7: class
SubversionDataSource(object):
How about just putting a list-files method on the Fetcher interface, not having
this class at all.

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/subversion_data_source.py:17: page_dir =
re.search('<title>.* (.*)</title>', page).group(1)
Could we try using an HTML parser here rather than regular expressions? Regex is
inefficient and hard to maintain, and looks kind of awkward.

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/templates/public/samples.html
(right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/public/samples.html:3: <a
href="examples/api/i18n/getMessage.zip">getMessage</a><br/>
Yep, as per IRC convo, generate this?

If it's too hard/inefficient I also think it's fine to actually check in a
samples.json file. We don't add samples that often.

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/zip_data_source.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/zip_data_source.py:12: class
ZipDataSource(SubversionDataSource):
Why does this need to expose a data source interface, do templates need to be
able to embed zip files? I doubt it. Just a normal class would be fine.

[Aaron Boodman, 2012-07-11 02:54:50]

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/subversion_data_source.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/subversion_data_source.py:17: page_dir =
re.search('<title>.* (.*)</title>', page).group(1)
On 2012/07/11 00:35:09, kalman wrote:
> Could we try using an HTML parser here rather than regular expressions? Regex
is
> inefficient and hard to maintain, and looks kind of awkward.

The directory listing page looks like valid xml to me. If so, you can just do
that. If not, there's BeautifulSoup.

[not at google - send to devlin, 2012-07-11 07:23:57]

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/subversion_data_source.py (right):

http://codereview.chromium.org/10689144/diff/12001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/subversion_data_source.py:17: page_dir =
re.search('<title>.* (.*)</title>', page).group(1)
On 2012/07/11 02:54:51, Aaron Boodman wrote:
> On 2012/07/11 00:35:09, kalman wrote:
> > Could we try using an HTML parser here rather than regular expressions?
Regex
> is
> > inefficient and hard to maintain, and looks kind of awkward.
> 
> The directory listing page looks like valid xml to me. If so, you can just do
> that. If not, there's BeautifulSoup.

I think there's actually an HTML parser in python?
http://docs.python.org/library/htmlparser.html is BeautifulSoup better or
something?

Anyway, cool if it's valid XML.

It might be worth quickly looking to see whether you can make svn just return
the file list (like svn ls does) then this wouldn't be necessary at all.

[Aaron Boodman, 2012-07-11 07:43:59]

On Wed, Jul 11, 2012 at 12:23 AM,  <kalman@chromium.org> wrote:
> I think there's actually an HTML parser in python?
> http://docs.python.org/library/htmlparser.html is BeautifulSoup better
> or something?

Huh, didn't know about that one. I always heard people talk about
BeautifulSoup, so I guess I assumed there wasn't a built in one.

- a

[cduvall, 2012-07-11 08:34:46]

On 2012/07/11 07:43:59, Aaron Boodman wrote:
> On Wed, Jul 11, 2012 at 12:23 AM,  <mailto:kalman@chromium.org> wrote:
> > I think there's actually an HTML parser in python?
> > http://docs.python.org/library/htmlparser.html is BeautifulSoup better
> > or something?
> 
> Huh, didn't know about that one. I always heard people talk about
> BeautifulSoup, so I guess I assumed there wasn't a built in one.
> 
> - a

Also, this looks like it might be useful for what we need:
http://docs.python.org/library/xml.dom.minidom.html

[cduvall, 2012-07-11 20:56:30]

Reworked the Zipper. I also made a simple samples page (just a list of all the
samples and links) but we won't be able to test the load speed on svn until its
committed.

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/echo_handler.py:4: # found in the LICENSE
file.
On 2012/07/11 00:35:09, kalman wrote:
> At what point do we rename this file from echo_handler to something that gives
> it more credit?
> 
> (not in this patch, but yeah...)

Yeah definitely no longer an echo handler. echo_handler++

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/echo_handler.py:36: ZIP_IGNORE_PATH =
EXTENSIONS_PATH + '/' + DOCS_PATH
On 2012/07/11 00:35:09, kalman wrote:
> The ignore path is prefix that should be stripped from requests or something?
> I'm confused. From looking at it, I thought it was something to do with
> blacklisting a directory.
> 
> If we put the listing of files on Fetcher then this code will change anyway,
and
> I don't think we'll need it anymore?

Done.

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
File chrome/common/extensions/docs/server2/local_fetcher.py (right):

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/local_fetcher.py:9: a SubversionFetcher
would give.
On 2012/07/11 00:35:09, kalman wrote:
> I think this will be unnecessary if listing files is incorporated into the
> Fetcher interface.

Done.

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/server_instance.py:33: if
path.endswith('.zip'):
On 2012/07/11 00:35:09, kalman wrote:
> I'd be more comfortable if this was restricted specifically to the examples
> directory (injected of course).
> 
> Maybe a glob like "%s/**/*.zip" % examples_dir (there's a glob library
> somewhere). Or whatever glob refers to "directory under examples that's not
the
> toplevel directory".
> 
> We also need to check for a manifest.json in that directory before we zip it,
> and make sure we return None if it isn't found.
> 
> A job for another class obviously; perhaps ZipDataSource can turn into that.
> Call like ExampleZipper or something (hehe).

Done. I used re because glob is only for the filesystem.

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
File chrome/common/extensions/docs/server2/subversion_data_source.py (right):

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/subversion_data_source.py:7: class
SubversionDataSource(object):
On 2012/07/11 00:35:09, kalman wrote:
> How about just putting a list-files method on the Fetcher interface, not
having
> this class at all.

Done.

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
File chrome/common/extensions/docs/server2/templates/public/samples.html
(right):

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/public/samples.html:3: <a
href="examples/api/i18n/getMessage.zip">getMessage</a><br/>
On 2012/07/11 00:35:09, kalman wrote:
> Yep, as per IRC convo, generate this?
> 
> If it's too hard/inefficient I also think it's fine to actually check in a
> samples.json file. We don't add samples that often.

Done.

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
File chrome/common/extensions/docs/server2/zip_data_source.py (right):

https://chromiumcodereview.appspot.com/10689144/diff/12001/chrome/common/exte...
chrome/common/extensions/docs/server2/zip_data_source.py:12: class
ZipDataSource(SubversionDataSource):
On 2012/07/11 00:35:09, kalman wrote:
> Why does this need to expose a data source interface, do templates need to be
> able to embed zip files? I doubt it. Just a normal class would be fine.

Done.

[not at google - send to devlin, 2012-07-12 00:22:48]

You uploaded a new patchset half way through reviewing the older one, so the
comments are a bit spread out.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/example_zipper.py (right):

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:17: self._svn_cache =
cache_builder.build(lambda x: x)
Is it actually worth having a cache for the svn stuff here? It seems like all we
really need is the zip cache + urlfetch (with its automatic memcaching) since
we're unlikely to need to read any of those files except while zipping them...
especially from within this class.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:26: return None
This leaks zip_file, but see comment below.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:30:
filename.replace(prefix, ''),
filename[len(prefix):] ?

This actually leaves a leading / when these paths are absolute (which seems to
be happening), but not when relative, so you'll also need to strip the leading
'/' before putting them in the zip file, or you'll get messages like

warning:  stripped absolute path spec from /make_page_red/background.js

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:33: logging.info(e)
Should we be serving partial zip files on exceptions?

Perhaps let this exception fall through to the handlers and put the
zip_file.close() in a finally block.

zip_bytes = ...
zip_path = ...
prefix = ...
if zip_path ...
  return None

zip_file = ZipFile(...)
try:
  for filename in files:
    ...
finally:
  zip_file.close()
return zip_bytes.getvalue()

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:37: def
__getitem__(self, key):
Remove this method?

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:40: def get(self, key):
A more sensible interface for this class, I think, would be like

def create(self, path):
  """ Creates a new zip file from the recursive contents of |path|
  as returned by |_zip_cache|.
  Paths within the zip file are given relative to and including |path|.
  """

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:41: if not
re.match(self._match_path + '/.*\.zip$', key):
See comment just above; none of this checking should be necessary, nor should
this class need to strip out ".zip".

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:45: return
self._zip_cache.get(self._base_path + '/' + base, True)
This really threw me. Can we put two methods on the Fetchers, "getFromFile" and
"getFromListingFiles" or something? getFromFile would throw an error if it's
given a directory, getFromListingFiles would throw an error if it's not given a
directory.

Would make this easier to read, less magical, probably less error prone.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:47: return None
We should let the top-level handler deal with error exceptions like these (we'd
want to serve a 500 not a 404, and log it properly).

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/fetcher_cache.py (right):

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:37: def get(self, key,
get_dir=False):
2 separate methods please :)

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/local_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_fetcher.py:28: def
FetchDirectory(self, directory):
Could you make recursive an optional parameter, default to False?

Also, call FetchDirectoryList or ListDirectory? "FetchDirectory" might imply
fetching the contents of each file in the directory.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_fetcher.py:38: # Remove |_base_path|
from files before returning.
This is the kind of contract that should be documented.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:34: if
path.endswith('.zip'):
Could be even smarter here:

if fnmatch(path, 'examples/*.zip'):
  ...

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:40:
request_handler.response.headers['content-type'] = 'text/plain'
This has the potential to be pretty cool if we want; a way to browse the
examples without actually downloading them. The current docs don't let us do
that (as far as I know).

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:27:
files.remove('..')
I presume it doesn't also return '.'

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:40: def
FetchDirectory(self, path):
Ditto, change name, make recursive an option.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:30:
self._file_cache = cache_builder.build(lambda x: x)
Same comment as earlier. file_cache not necessary given the files are already
being memcached, and we're caching at a higher level so don't need to also cache
at a low level.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:49: return
api_items + list(set(funcs) - set(api_items))
This is pretty indirect, just pass through set and add() to it? Or rather,

funcs = ...
api_items.update(funcs)

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:62: if
filename.rsplit('/')[-1] == 'manifest.json':
if filename.rsplit('/')[-1] != 'manifest.json':
  continue

?

You could also do something like let the Fetchers accept a filter, but that's
probably overengineering for now.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:94: return
samples_list
wow badass.

Definitely pull this stuff out of here and into a SampleFetcher.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:113: 'samples':
self._dir_cache.get(self._samples_path, True),
Can we make this lazily populate somehow? Most templates won't care about the
samples, e.g. write a SamplesDataSource.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:37: '',
fake.. something?

[cduvall, 2012-07-12 01:51:23]

Still needs to handle __MSG_ strings but other than that its looking good.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/example_zipper.py (right):

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:17: self._svn_cache =
cache_builder.build(lambda x: x)
On 2012/07/12 00:22:48, kalman wrote:
> Is it actually worth having a cache for the svn stuff here? It seems like all
we
> really need is the zip cache + urlfetch (with its automatic memcaching) since
> we're unlikely to need to read any of those files except while zipping them...
> especially from within this class.

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:26: return None
On 2012/07/12 00:22:48, kalman wrote:
> This leaks zip_file, but see comment below.

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:30:
filename.replace(prefix, ''),
On 2012/07/12 00:22:48, kalman wrote:
> filename[len(prefix):] ?
> 
> This actually leaves a leading / when these paths are absolute (which seems to
> be happening), but not when relative, so you'll also need to strip the leading
> '/' before putting them in the zip file, or you'll get messages like
> 
> warning:  stripped absolute path spec from /make_page_red/background.js

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:33: logging.info(e)
On 2012/07/12 00:22:48, kalman wrote:
> Should we be serving partial zip files on exceptions?
> 
> Perhaps let this exception fall through to the handlers and put the
> zip_file.close() in a finally block.
> 
> zip_bytes = ...
> zip_path = ...
> prefix = ...
> if zip_path ...
>   return None
> 
> zip_file = ZipFile(...)
> try:
>   for filename in files:
>     ...
> finally:
>   zip_file.close()
> return zip_bytes.getvalue()

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:37: def
__getitem__(self, key):
On 2012/07/12 00:22:48, kalman wrote:
> Remove this method?

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:40: def get(self, key):
On 2012/07/12 00:22:48, kalman wrote:
> A more sensible interface for this class, I think, would be like
> 
> def create(self, path):
>   """ Creates a new zip file from the recursive contents of |path|
>   as returned by |_zip_cache|.
>   Paths within the zip file are given relative to and including |path|.
>   """

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:41: if not
re.match(self._match_path + '/.*\.zip$', key):
On 2012/07/12 00:22:48, kalman wrote:
> See comment just above; none of this checking should be necessary, nor should
> this class need to strip out ".zip".

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:45: return
self._zip_cache.get(self._base_path + '/' + base, True)
On 2012/07/12 00:22:48, kalman wrote:
> This really threw me. Can we put two methods on the Fetchers, "getFromFile"
and
> "getFromListingFiles" or something? getFromFile would throw an error if it's
> given a directory, getFromListingFiles would throw an error if it's not given
a
> directory.
> 
> Would make this easier to read, less magical, probably less error prone.

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:47: return None
On 2012/07/12 00:22:48, kalman wrote:
> We should let the top-level handler deal with error exceptions like these
(we'd
> want to serve a 500 not a 404, and log it properly).

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/fetcher_cache.py (right):

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/fetcher_cache.py:37: def get(self, key,
get_dir=False):
On 2012/07/12 00:22:48, kalman wrote:
> 2 separate methods please :)

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/local_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_fetcher.py:28: def
FetchDirectory(self, directory):
On 2012/07/12 00:22:48, kalman wrote:
> Could you make recursive an optional parameter, default to False?
> 
> Also, call FetchDirectoryList or ListDirectory? "FetchDirectory" might imply
> fetching the contents of each file in the directory.

Done.

http://codereview.chromium.org/10689144/diff/1022/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/local_fetcher.py:38: # Remove |_base_path|
from files before returning.
On 2012/07/12 00:22:48, kalman wrote:
> This is the kind of contract that should be documented.

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:34: if
path.endswith('.zip'):
On 2012/07/12 00:22:48, kalman wrote:
> Could be even smarter here:
> 
> if fnmatch(path, 'examples/*.zip'):
>   ...

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:27:
files.remove('..')
On 2012/07/12 00:22:48, kalman wrote:
> I presume it doesn't also return '.'

Correct.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:40: def
FetchDirectory(self, path):
On 2012/07/12 00:22:48, kalman wrote:
> Ditto, change name, make recursive an option.

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:30:
self._file_cache = cache_builder.build(lambda x: x)
On 2012/07/12 00:22:48, kalman wrote:
> Same comment as earlier. file_cache not necessary given the files are already
> being memcached, and we're caching at a higher level so don't need to also
cache
> at a low level.

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:49: return
api_items + list(set(funcs) - set(api_items))
On 2012/07/12 00:22:48, kalman wrote:
> This is pretty indirect, just pass through set and add() to it? Or rather,
> 
> funcs = ...
> api_items.update(funcs)

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:62: if
filename.rsplit('/')[-1] == 'manifest.json':
On 2012/07/12 00:22:48, kalman wrote:
> if filename.rsplit('/')[-1] != 'manifest.json':
>   continue
> 
> ?
> 
> You could also do something like let the Fetchers accept a filter, but that's
> probably overengineering for now.

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:113: 'samples':
self._dir_cache.get(self._samples_path, True),
On 2012/07/12 00:22:48, kalman wrote:
> Can we make this lazily populate somehow? Most templates won't care about the
> samples, e.g. write a SamplesDataSource.

Done.

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10689144/diff/6014/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:37: '',
On 2012/07/12 00:22:48, kalman wrote:
> fake.. something?

Done.

[not at google - send to devlin, 2012-07-12 07:06:29]

I don't want to hold this up any more (time difference and all that) so lgtm, I
trust you to fix up those things and submit.

Do try and write some tests for the directory listing stuff though. Presumably
you'd need to fake some XML for the directory listing in that test.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/example_zipper.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:41: True)
Be explicit here with recursive=True, makes this call site easier to understand.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:39: elif
path.startswith('example'):
examples/ ?

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:42: elif
path.startswith('static'):
static/ ?

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:25: files =
map(lambda x: x.childNodes[0].data, dom.getElementsByTagName('a'))
I think the "pythonic" way of doing map() is usually with list comprehensions

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:35:
all_files.extend(self.ListDirectory(dir_name).content)
need to pass recursive=True here?

Or call _RecursiveList directly?

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:41: result =
self._url_fetcher.fetch(self._base_path + '/' + path)
I don't see how this works, doesn't this return HTML, so if recursive is False
then it'll return some HTML rather than the list of files?

I would have expected this to be structured like

return _RecursiveList(path) if recursive else _ListDir(path)

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_fetcher_test.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher_test.py:17: 
tests for listing files?

[cduvall, 2012-07-12 18:09:02]

Added some tests and cleaned things up. I'll handle the __MSG_ stuff in another
patch.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/example_zipper.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/example_zipper.py:41: True)
On 2012/07/12 07:06:29, kalman wrote:
> Be explicit here with recursive=True, makes this call site easier to
understand.

Done.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:39: elif
path.startswith('example'):
On 2012/07/12 07:06:29, kalman wrote:
> examples/ ?

Done.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:42: elif
path.startswith('static'):
On 2012/07/12 07:06:29, kalman wrote:
> static/ ?

Done.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_fetcher.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:25: files =
map(lambda x: x.childNodes[0].data, dom.getElementsByTagName('a'))
On 2012/07/12 07:06:29, kalman wrote:
> I think the "pythonic" way of doing map() is usually with list comprehensions

Done.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:35:
all_files.extend(self.ListDirectory(dir_name).content)
On 2012/07/12 07:06:29, kalman wrote:
> need to pass recursive=True here?
> 
> Or call _RecursiveList directly?

Done.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher.py:41: result =
self._url_fetcher.fetch(self._base_path + '/' + path)
On 2012/07/12 07:06:29, kalman wrote:
> I don't see how this works, doesn't this return HTML, so if recursive is False
> then it'll return some HTML rather than the list of files?
> 
> I would have expected this to be structured like
> 
> return _RecursiveList(path) if recursive else _ListDir(path)

Done.

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/subversion_fetcher_test.py (right):

http://codereview.chromium.org/10689144/diff/2026/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/subversion_fetcher_test.py:17: 
On 2012/07/12 07:06:29, kalman wrote:
> tests for listing files?

Done.