Extensions Docs Server: Apps samples page

Extensions Docs Server: Apps samples page

This page pulls the apps samples from github and constructs a page similar to
the extensions samples page. The links take you to github instead of
downloading zip files.

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

[cduvall, 2012-07-27 22:25:22]

Samples page for apps!

[not at google - send to devlin, 2012-07-30 11:24:05]

Nice one.

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:12: GITHUB_BASE =
'https://github.com/GoogleChrome/chrome-app-samples/tree/master'
looks like this is kinda duplicated in echo_handler. Pull out into a
"url_constants" module or something (you could put the svn/omaha stuff in there
too, why not).

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/templates/public/apps_samples.html
(right):

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/public/apps_samples.html:12: <h1
class="page_title">Apps Samples</h1>
Apps Samples -> Sample Apps

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/templates/public/samples.html
(right):

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/public/samples.html:12: <h1
class="page_title">Samples</h1>
Sample Extensions?

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/zip_file_system.py (right):

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/zip_file_system.py:13: class
ZipFileSystem(file_system.FileSystem):
This is super cool, though it would be nice to re-use the MemcacheFileSystem
infrastructure (which is tested etc).

Furthermore, how big are these zip files going to be? We should be careful, we
might check in a lot of data (images, sounds, etc) and memcache will start being
mean to us for trying to put a large zip file in there.

Does github expose a more granular API? Like, the ability to list files, get
their versions, read files without downloading the whole zip file? I hope so.
Then this can be like SubversionFileSystem (call it GithubFileSystem, or if it's
possible to use generic git protocols then that's even better since Chromium
will at some point migrate to git, and call it GitFileSystem).

Sorry to ask you do throw away this file... though it's only a small amount of
code :) it happens.

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/zip_file_system.py:40: self._GetZip()
You should try to put these expensive operations (getting the zip file from
github) inside the Future i.e. make it asynchronous.

[cduvall, 2012-08-02 01:14:53]

See response to your comment in ZipFileSystem for the problem I was having.

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:12: GITHUB_BASE =
'https://github.com/GoogleChrome/chrome-app-samples/tree/master'
On 2012/07/30 11:24:05, kalman wrote:
> looks like this is kinda duplicated in echo_handler. Pull out into a
> "url_constants" module or something (you could put the svn/omaha stuff in
there
> too, why not).

Done.

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/public/apps_samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/apps_samples.html:12: <h1
class="page_title">Apps Samples</h1>
On 2012/07/30 11:24:05, kalman wrote:
> Apps Samples -> Sample Apps

Done.

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/public/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/samples.html:12: <h1
class="page_title">Samples</h1>
On 2012/07/30 11:24:05, kalman wrote:
> Sample Extensions?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
File chrome/common/extensions/docs/server2/zip_file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
chrome/common/extensions/docs/server2/zip_file_system.py:13: class
ZipFileSystem(file_system.FileSystem):
On 2012/07/30 11:24:05, kalman wrote:
> This is super cool, though it would be nice to re-use the MemcacheFileSystem
> infrastructure (which is tested etc).
> 
> Furthermore, how big are these zip files going to be? We should be careful, we
> might check in a lot of data (images, sounds, etc) and memcache will start
being
> mean to us for trying to put a large zip file in there.
> 
> Does github expose a more granular API? Like, the ability to list files, get
> their versions, read files without downloading the whole zip file? I hope so.
> Then this can be like SubversionFileSystem (call it GithubFileSystem, or if
it's
> possible to use generic git protocols then that's even better since Chromium
> will at some point migrate to git, and call it GitFileSystem).
> 
> Sorry to ask you do throw away this file... though it's only a small amount of
> code :) it happens.

So I switched to fetching individual files and directories like
SubversionFileSystem, but it is soooo slow. It can't complete a request without
timing out even on the App Engine Server. I'm pretty sure it works because I
limited the samples (like to only samples that start with 'a'), and the page
renders right, but with all the samples, it goes way too slow.

https://chromiumcodereview.appspot.com/10825067/diff/3008/chrome/common/exten...
chrome/common/extensions/docs/server2/zip_file_system.py:40: self._GetZip()
On 2012/07/30 11:24:05, kalman wrote:
> You should try to put these expensive operations (getting the zip file from
> github) inside the Future i.e. make it asynchronous.

Done.

[not at google - send to devlin, 2012-08-02 13:27:48]

Ah, sorry to have lead you down that path. I guess we will need to download a
zip fie and memcache* it after all.

But yeah, that should just be an implementation detail of GithubFileSystem; to
get a single file, download the whole zip then return files from within it (as I
believe you were already doing in the ZipFileSystem), cache it for however long.

You should use a similar approach to MemcacheFileSystem, that is, only
re-download the zip if the stat has changed, for the whole directory. Check
every minute etc (you can get stat info from
https://api.github.com/repos/GoogleChrome/chrome-app-samples/commits/HEAD
)

It would be nice to re-use MemcacheFileSystem and make GithubFileSystem really
dumb, but I don't see an easy way of doing that without increasing the power of
FileSystems or something, and, eh. Can you think of anything?

* (I just checked and the apps zip is only 4k, so that's fine to fit in
memcache. I guess some day it might get large and we'll need to put it in
blobstore, but not yet).

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/zip_file_system.py (right):

http://codereview.chromium.org/10825067/diff/3008/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/zip_file_system.py:13: class
ZipFileSystem(file_system.FileSystem):
On 2012/08/02 01:14:53, cduvall wrote:
> On 2012/07/30 11:24:05, kalman wrote:
> > This is super cool, though it would be nice to re-use the MemcacheFileSystem
> > infrastructure (which is tested etc).
> > 
> > Furthermore, how big are these zip files going to be? We should be careful,
we
> > might check in a lot of data (images, sounds, etc) and memcache will start
> being
> > mean to us for trying to put a large zip file in there.
> > 
> > Does github expose a more granular API? Like, the ability to list files, get
> > their versions, read files without downloading the whole zip file? I hope
so.
> > Then this can be like SubversionFileSystem (call it GithubFileSystem, or if
> it's
> > possible to use generic git protocols then that's even better since Chromium
> > will at some point migrate to git, and call it GitFileSystem).
> > 
> > Sorry to ask you do throw away this file... though it's only a small amount
of
> > code :) it happens.
> 
> So I switched to fetching individual files and directories like
> SubversionFileSystem, but it is soooo slow. It can't complete a request
without
> timing out even on the App Engine Server. I'm pretty sure it works because I
> limited the samples (like to only samples that start with 'a'), and the page
> renders right, but with all the samples, it goes way too slow.

Ah. I wonder if that's because we're getting rate limited by github.

http://developer.github.com/v3/#rate-limiting

Ok, I guess zip file it is.

http://codereview.chromium.org/10825067/diff/11002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/file_system.py (right):

http://codereview.chromium.org/10825067/diff/11002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/file_system.py:49: return version1 >
version2
This won't work for git.

http://codereview.chromium.org/10825067/diff/11002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

http://codereview.chromium.org/10825067/diff/11002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/memcache_file_system.py:43: if
self._file_system.CheckStat(self.Stat(path).version, version):
yeah, needs to be != (and besides, I don't think that CheckStat should exist,
it's simpler to just write !=)

[cduvall, 2012-08-02 23:20:01]

So it turns out that the size of the samples zip is 4MB not 4KB, so it doesn't
fit in memcache. This patch doesn't even try to memcache it, but I'm still
looking into blobstore.

https://chromiumcodereview.appspot.com/10825067/diff/11002/chrome/common/exte...
File chrome/common/extensions/docs/server2/file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/11002/chrome/common/exte...
chrome/common/extensions/docs/server2/file_system.py:49: return version1 >
version2
On 2012/08/02 13:27:48, kalman wrote:
> This won't work for git.

I had this overridden in GithubFileSystem using !=.

https://chromiumcodereview.appspot.com/10825067/diff/11002/chrome/common/exte...
File chrome/common/extensions/docs/server2/memcache_file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/11002/chrome/common/exte...
chrome/common/extensions/docs/server2/memcache_file_system.py:43: if
self._file_system.CheckStat(self.Stat(path).version, version):
On 2012/08/02 13:27:48, kalman wrote:
> yeah, needs to be != (and besides, I don't think that CheckStat should exist,
> it's simpler to just write !=)

Done.

[cduvall, 2012-08-03 18:44:32]

Now blobstore is used to store the zip file.

[not at google - send to devlin, 2012-08-07 01:35:38]

nice

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/appengine_blobstore.py (right):

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_blobstore.py:14: """
methods could do with some documentation, e.g. what's the "version" parameter
for?

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_blobstore.py:17: self._blob_keys
= {}
If the keys are only stored in volatile memory (memory or memstore) then it
seems like we could end up losing blobs sometimes. We should probably replace
the memcache layer with a datastore layer?

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_blobstore.py:19: def Set(self,
blob, key, version):
usually the key would be before the value in the parameter list.

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/github_file_system.py:24:
self._blobstore.Delete(ZIP_KEY, self._old_version)
Won't blobstore also need to be namespaced (like AppengineMemcache is) for this
to not potentially result in namespace conflicts?

I guess AppengineBlobstore only uses it within its own memory and using an
AppengineMemcache instance, so if it's the same as here then it'll be safe, but
something to keep in mind when other persistent storage is being used.

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/github_file_system.py:26:
self._blobstore.Set(blob, ZIP_KEY, self._new_version)
Why are you versioning the zip files? Just curious.

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/github_file_system.py:47: self._version))
Nice, doing this asynchronously is the way to go.

However, after calling _GetZip in Read, it either calls _ReadFile or _ListDir
which just calls _zip_file.Get() anyway, so the effort to make this asynchronous
is kinda lost.

This needs to be rearchitected slightly.

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/github_file_system.py:67: if
True:#self._zip_file is None or version != self._version:
?

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/integration_test.py (right):

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/integration_test.py:17:
'apps_samples.html'
Perhaps we should add comments here explaining why each page fails.

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:107: l10n_data =
self._GetDataFromManifest(sample_path, file_system)
l10n_data is getting to be a confusing variable name

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:158:
self._samples_path + '/')
I think we should do the same kind of thing you did for the api list data source
here;

return {
  'apps': ...
  'extensions': ...
}.get(key, None)

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/templates/public/apps_samples.html
(right):

http://codereview.chromium.org/10825067/diff/8002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/public/apps_samples.html:12: <h1
class="page_title">Sample Apps</h1>
This is also a pretty complex template that it might be useful to do the
"is_apps" treatment to.

I would suggest passing in both the list of samples and whether it's for apps as
separate parameters.

[cduvall, 2012-08-08 19:27:25]

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_blobstore.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_blobstore.py:14: """
On 2012/08/07 01:35:38, kalman wrote:
> methods could do with some documentation, e.g. what's the "version" parameter
> for?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_blobstore.py:17: self._blob_keys
= {}
On 2012/08/07 01:35:38, kalman wrote:
> If the keys are only stored in volatile memory (memory or memstore) then it
> seems like we could end up losing blobs sometimes. We should probably replace
> the memcache layer with a datastore layer?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_blobstore.py:19: def Set(self,
blob, key, version):
On 2012/08/07 01:35:38, kalman wrote:
> usually the key would be before the value in the parameter list.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:24:
self._blobstore.Delete(ZIP_KEY, self._old_version)
On 2012/08/07 01:35:38, kalman wrote:
> Won't blobstore also need to be namespaced (like AppengineMemcache is) for
this
> to not potentially result in namespace conflicts?
> 
> I guess AppengineBlobstore only uses it within its own memory and using an
> AppengineMemcache instance, so if it's the same as here then it'll be safe,
but
> something to keep in mind when other persistent storage is being used.

I think the version functions as a namespace for the blobstore. We also want all
the branches to have access to the same blobs, so I think this should work.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:26:
self._blobstore.Set(blob, ZIP_KEY, self._new_version)
On 2012/08/07 01:35:38, kalman wrote:
> Why are you versioning the zip files? Just curious.

I versioned them so if there's an old zip hanging around we will be sure to get
the new one. I guess its not super necessary, so I can take it out if you want.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:47: self._version))
On 2012/08/07 01:35:38, kalman wrote:
> Nice, doing this asynchronously is the way to go.
> 
> However, after calling _GetZip in Read, it either calls _ReadFile or _ListDir
> which just calls _zip_file.Get() anyway, so the effort to make this
asynchronous
> is kinda lost.
> 
> This needs to be rearchitected slightly.

I'm starting the fetch when the GithubFileSystem is constructed now. I'm not
sure if there's a way to take advantage of this being asynchronous after that.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:67: if
True:#self._zip_file is None or version != self._version:
On 2012/08/07 01:35:38, kalman wrote:
> ?

oops :)

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:17:
'apps_samples.html'
On 2012/08/07 01:35:38, kalman wrote:
> Perhaps we should add comments here explaining why each page fails.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:107: l10n_data =
self._GetDataFromManifest(sample_path, file_system)
On 2012/08/07 01:35:38, kalman wrote:
> l10n_data is getting to be a confusing variable name

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:158:
self._samples_path + '/')
On 2012/08/07 01:35:38, kalman wrote:
> I think we should do the same kind of thing you did for the api list data
source
> here;
> 
> return {
>   'apps': ...
>   'extensions': ...
> }.get(key, None)

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/public/apps_samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/apps_samples.html:12: <h1
class="page_title">Sample Apps</h1>
On 2012/08/07 01:35:38, kalman wrote:
> This is also a pretty complex template that it might be useful to do the
> "is_apps" treatment to.
> 
> I would suggest passing in both the list of samples and whether it's for apps
as
> separate parameters.

Done.

[not at google - send to devlin, 2012-08-09 07:52:36]

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:24:
self._blobstore.Delete(ZIP_KEY, self._old_version)
On 2012/08/08 19:27:25, cduvall wrote:
> On 2012/08/07 01:35:38, kalman wrote:
> > Won't blobstore also need to be namespaced (like AppengineMemcache is) for
> this
> > to not potentially result in namespace conflicts?
> > 
> > I guess AppengineBlobstore only uses it within its own memory and using an
> > AppengineMemcache instance, so if it's the same as here then it'll be safe,
> but
> > something to keep in mind when other persistent storage is being used.
> 
> I think the version functions as a namespace for the blobstore. We also want
all
> the branches to have access to the same blobs, so I think this should work.

sgtm. I don't know so much about the versions though.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:26:
self._blobstore.Set(blob, ZIP_KEY, self._new_version)
On 2012/08/08 19:27:25, cduvall wrote:
> On 2012/08/07 01:35:38, kalman wrote:
> > Why are you versioning the zip files? Just curious.
> 
> I versioned them so if there's an old zip hanging around we will be sure to
get
> the new one. I guess its not super necessary, so I can take it out if you
want.

I made comment in the new version of the code.

https://chromiumcodereview.appspot.com/10825067/diff/8002/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:47: self._version))
On 2012/08/08 19:27:25, cduvall wrote:
> On 2012/08/07 01:35:38, kalman wrote:
> > Nice, doing this asynchronously is the way to go.
> > 
> > However, after calling _GetZip in Read, it either calls _ReadFile or
_ListDir
> > which just calls _zip_file.Get() anyway, so the effort to make this
> asynchronous
> > is kinda lost.
> > 
> > This needs to be rearchitected slightly.
> 
> I'm starting the fetch when the GithubFileSystem is constructed now. I'm not
> sure if there's a way to take advantage of this being asynchronous after that.

Well... I realised there actually isn't much advantage to it anyway, since the
asynchronous wins come from when there are multiple things to get. Here there's
only one, but if we change Stat to be able to take multiple things at once...
maybe there would.

(to make this asynchronous you'd need to lift more things into Futures)

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:21:
logging.info(self._api_search)
remove logging?

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:28:
samples_list.append(sample)
All this seems like it could be a method on SamplesDataSource

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:34: # important to
render the API page.
I don't follow this comment nor why you need to catch Exception here.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_blobstore.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_blobstore.py:17: def Set(self,
key, blob, version):
yeah, I don't really see the need to store multiple versions. What's the
difference between getting foo1.zip then later adding a foo2.zip and removing
foo1.zip, versus overwriting foo.zip with the new version?

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_datastore.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:15: """A wrapper
around the datastore API that can store blob keys.
AppEngineDatastore is kind of a generic name to be for the class that
specifically stores blobstore keys, how about like, BlobReferenceStore or
something.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:19: 
nit:

def _MakeKey(self, namespace, key):
  return '.'.join([self._branch, namespace, key])

then use _MakeKey.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:25: return result
return result?

Or just return _Model.gql...

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:27: def Set(self,
key, value, namespace):
namespace is part of the key, right? So it go before value in the arguments. Or
better, before key.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:30: def Get(self,
key, namespace):
ditto

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:15: 
This is something you can do in a follow-up, but...

Downloading this stuff takes a little while, right? Like in the order of a few
seconds. Something we could do is put the downloading in a background thread,
and use the old version until it's finished.

It'd need to block if there's no old version (and we could use a warm-up request
to do that).

We'd also need to make sure not to spawn multiple gets etc etc. So yeah,
something to do in a follow-up.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:26:
self._blobstore.Set(ZIP_KEY, blob, self._new_version)
Yeah, so as I said, I don't see the difference between this and just replacing
the old version with the new.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:71: github_cache_builder =
FileSystemCache.Builder(github_file_system)
optimisation: the github file system isn't versioned by branch, so it can be
shared amongst all instances.

(i.e. put up in file-global scope; might as well)

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:18: 'samples.html',
... except now we're not testing the extensions either.

This paths need to be qualified, methinks. Like ['apps/samples.html',
'apps/app.html'].

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:38:
logging.getLogger().setLevel(logging.ERROR)
remove logging

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:112:
sample_base_path = (url_constants.RAW_GITHUB_BASE + '/' +
nit: line-break like

sample_base_path = (
    url_constants.RAW_GITHUB_BASE + '/' + sample_base_path)

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/private/api_reference.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/api_reference.html:33:
<li><strong><a href="{{github_path}}">{{name}}</a></strong> &ndash;
{{description}}</li>
a bit hard to read, so some nits, indent within the {{?is_apps}} and line-break
after the &ndash;

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/private/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/samples.html:33:
<span><a href="javascript:void(0)">{{name}}</a>{{^last}} | {{/}}</span>
pull repeated <span>...</span> into a partial?

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/samples.html:43: <div
class="sample" tags="{{#api_calls}}{{name}} {{/}}">
I'm sure this can be pulled into a partial with appropriate parameters.

[cduvall, 2012-08-09 19:20:08]

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:21:
logging.info(self._api_search)
On 2012/08/09 07:52:36, kalman wrote:
> remove logging?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:28:
samples_list.append(sample)
On 2012/08/09 07:52:36, kalman wrote:
> All this seems like it could be a method on SamplesDataSource

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:34: # important to
render the API page.
On 2012/08/09 07:52:36, kalman wrote:
> I don't follow this comment nor why you need to catch Exception here.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_blobstore.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_blobstore.py:17: def Set(self,
key, blob, version):
On 2012/08/09 07:52:36, kalman wrote:
> yeah, I don't really see the need to store multiple versions. What's the
> difference between getting foo1.zip then later adding a foo2.zip and removing
> foo1.zip, versus overwriting foo.zip with the new version?

See comment my reply to your other version comment (other instances won't know
when to fetch a new version). Also, I added a namespace so this has the same
interface as AppEngineMemcache.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_datastore.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:15: """A wrapper
around the datastore API that can store blob keys.
On 2012/08/09 07:52:36, kalman wrote:
> AppEngineDatastore is kind of a generic name to be for the class that
> specifically stores blobstore keys, how about like, BlobReferenceStore or
> something.
> 

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:19: 
On 2012/08/09 07:52:36, kalman wrote:
> nit:
> 
> def _MakeKey(self, namespace, key):
>   return '.'.join([self._branch, namespace, key])
> 
> then use _MakeKey.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:25: return result
On 2012/08/09 07:52:36, kalman wrote:
> return result?
> 
> Or just return _Model.gql...

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:27: def Set(self,
key, value, namespace):
On 2012/08/09 07:52:36, kalman wrote:
> namespace is part of the key, right? So it go before value in the arguments.
Or
> better, before key.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_datastore.py:30: def Get(self,
key, namespace):
On 2012/08/09 07:52:36, kalman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/github_file_system.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:15: 
On 2012/08/09 07:52:36, kalman wrote:
> This is something you can do in a follow-up, but...
> 
> Downloading this stuff takes a little while, right? Like in the order of a few
> seconds. Something we could do is put the downloading in a background thread,
> and use the old version until it's finished.
> 
> It'd need to block if there's no old version (and we could use a warm-up
request
> to do that).
> 
> We'd also need to make sure not to spawn multiple gets etc etc. So yeah,
> something to do in a follow-up.

Cool, made a bug: http://crbug.com/141674

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/github_file_system.py:26:
self._blobstore.Set(ZIP_KEY, blob, self._new_version)
On 2012/08/09 07:52:36, kalman wrote:
> Yeah, so as I said, I don't see the difference between this and just replacing
> the old version with the new.

Ah, I just remembered why I versioned the zips. For the instances of the server
to share the zip file in the blobstore, they need to know what version it is. If
it is not keyed with a version, then instances have no way of knowing if the zip
stored in the blobstore is the old version or the new version.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/handler.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/handler.py:71: github_cache_builder =
FileSystemCache.Builder(github_file_system)
On 2012/08/09 07:52:36, kalman wrote:
> optimisation: the github file system isn't versioned by branch, so it can be
> shared amongst all instances.
> 
> (i.e. put up in file-global scope; might as well)

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:18: 'samples.html',
On 2012/08/09 07:52:36, kalman wrote:
> ... except now we're not testing the extensions either.
> 
> This paths need to be qualified, methinks. Like ['apps/samples.html',
> 'apps/app.html'].

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:38:
logging.getLogger().setLevel(logging.ERROR)
On 2012/08/09 07:52:36, kalman wrote:
> remove logging

This isn't a log, it just sets the logging level so that only ERROR level logs
are shown when we're running this test, so there's not unnecessary output.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:112:
sample_base_path = (url_constants.RAW_GITHUB_BASE + '/' +
On 2012/08/09 07:52:36, kalman wrote:
> nit: line-break like
> 
> sample_base_path = (
>     url_constants.RAW_GITHUB_BASE + '/' + sample_base_path)

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/private/api_reference.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/api_reference.html:33:
<li><strong><a href="{{github_path}}">{{name}}</a></strong> &ndash;
{{description}}</li>
On 2012/08/09 07:52:36, kalman wrote:
> a bit hard to read, so some nits, indent within the {{?is_apps}} and
line-break
> after the &ndash;

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/private/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/samples.html:33:
<span><a href="javascript:void(0)">{{name}}</a>{{^last}} | {{/}}</span>
On 2012/08/09 07:52:36, kalman wrote:
> pull repeated <span>...</span> into a partial?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/samples.html:43: <div
class="sample" tags="{{#api_calls}}{{name}} {{/}}">
On 2012/08/09 07:52:36, kalman wrote:
> I'm sure this can be pulled into a partial with appropriate parameters.

Done.

[not at google - send to devlin, 2012-08-10 06:02:18]

Code looks good, as in, lg.

Apologies to push back on the is_apps changes but I can see this ending up with
basically every template needing an is_apps parameter, and that would be quite
unfortunate. Let's try to compose these templates nicely rather than arrive
there.

http://codereview.chromium.org/10825067/diff/6015/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/integration_test.py (right):

http://codereview.chromium.org/10825067/diff/6015/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/integration_test.py:18: 'samples.html',
On 2012/08/09 19:20:08, cduvall wrote:
> On 2012/08/09 07:52:36, kalman wrote:
> > ... except now we're not testing the extensions either.
> > 
> > This paths need to be qualified, methinks. Like ['apps/samples.html',
> > 'apps/app.html'].
> 
> Done.

It would be nice to actually test this... and for that we'll need to fake out a
few more parts of appengine, set up some fake paths, and check in some abridged
version of the github zip.

Do in a follow-up, though (but file bug and put here).

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:17: the apps samples
page and vice versa.
nice

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source_test.py:37:
test1.pop('samples')
Why not just add the samples to the expected data? You know what it's supposed
to be (since you're controlling it in FakeSamplesDataSource).

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/appengine_blobstore.py (right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/appengine_blobstore.py:45: blob_key =
self._datastore.Get(datastore.BLOBREFERENCE_BLOBSTORE, key)
The way I read this, it's clearing the item from blobstore (since
BlobReferenceStore returns the value of the key it returns), but it doesn't
clear the reference to the key from datastore.

You could change this to call self._datastore.Delete, and make
AppEngineBlobstore.Delete return the value if it successfully deletes the item,
or None otherwise. That's pretty standard for a map API like this.

Then the code would be like

blob_key = self._datastore.Delete(datastore.BLOB_REFERENCE_BLOBSTORE, key)
if blob_key is None:
  return
blob_key.delete()

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/blob_reference_store.py (right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/blob_reference_store.py:8:
BLOBREFERENCE_BLOBSTORE = 'BlobReferenceBlobstore'
should be BLOB_REFERENCE_BLOBSTORE really...

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:111:
manifest_data['github_path'] = (url_constants.GITHUB_BASE + '/' +
See my comment in sample_item.html. Let's just have a single path/url value,
leave the logic in here.

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:142: def
FilterSamples(self, key, api_name):
documentation? what is "key"?

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:143: api_search =
'_' + api_name + '_'
unit test?

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:154: # If we're
testing, the GithubFileSystem can't fetch samples.
see comment about testing the github stuff.... you could mention it here too

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/templates/private/api_reference.html
(right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/private/api_reference.html:42:
{{/is_apps}}
See comment in other review and this review about
 - github_path
 - is_apps and injecting arguments.

If this template accepted two parameters, "title" and "sample_list", this could
be

{{?sample_list}}
  <h2>Sample {{title}} that use chrome.{{api.name}}</h2>
  {{#sample_list}}
  <li>...</li>
  {{/sample_list}}
{{/sample_list}}

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/templates/private/sample_item.html
(right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/private/sample_item.html:3:
<h2><a
href="{{?is_apps}}{{github_path}}{{:}}{{path}}.zip{{/}}">{{name}}</a></h2>
Why do we need github_path at all? Can we just make sure that it's set right
from samples_data_source?

Perhaps call it url.

Then we wouldn't need is_apps. Woo.

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/templates/private/samples.html
(right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/private/samples.html:9: <div
id="gc-container">
So, we should only use is_apps if absolutely necessary. Using is_apps makes the
templates less reusable, generally harder to follow.

Instead, inject
 - the sidenav
 - the title (I will definitely try to write that patch, though if I don't you
can always define "apps_title" and "extensions_title" like you did for
true/false in the dict generator).
 - the API list
 - the samples
 - the footer

At this point you might want to document this template like you would a
function, with a comment at the top explaining what the parameters are.

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/private/samples.html:49:
{{+partials.footer is_apps:is_apps}}
The only difference in the footer is the bootstrapped samples JSON. We used to
do this in a separate <script> tag before including the footer; perhaps we
should have apps_footer and extensions_footer which does this (each would set up
the right bootstrap arguments, then call the generic footer thing).

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
File
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html
(right):

http://codereview.chromium.org/10825067/diff/1044/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:27:
{{+partials.api_reference is_apps:true}}
see previous comments re is_apps

[cduvall, 2012-08-10 21:17:47]

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/6015/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:18: 'samples.html',
On 2012/08/10 06:02:18, kalman wrote:
> On 2012/08/09 19:20:08, cduvall wrote:
> > On 2012/08/09 07:52:36, kalman wrote:
> > > ... except now we're not testing the extensions either.
> > > 
> > > This paths need to be qualified, methinks. Like ['apps/samples.html',
> > > 'apps/app.html'].
> > 
> > Done.
> 
> It would be nice to actually test this... and for that we'll need to fake out
a
> few more parts of appengine, set up some fake paths, and check in some
abridged
> version of the github zip.
> 
> Do in a follow-up, though (but file bug and put here).

Done. Bug is: http://crbug.com/141910

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source_test.py:37:
test1.pop('samples')
On 2012/08/10 06:02:18, kalman wrote:
> Why not just add the samples to the expected data? You know what it's supposed
> to be (since you're controlling it in FakeSamplesDataSource).

The samples isn't actually a SamplesDataSource, its a _LazySamplesGetter. This
makes it so the test will fail, because the expected JSON doesn't have a
_LazySamplesGetter.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/appengine_blobstore.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/appengine_blobstore.py:45: blob_key =
self._datastore.Get(datastore.BLOBREFERENCE_BLOBSTORE, key)
On 2012/08/10 06:02:18, kalman wrote:
> The way I read this, it's clearing the item from blobstore (since
> BlobReferenceStore returns the value of the key it returns), but it doesn't
> clear the reference to the key from datastore.
> 
> You could change this to call self._datastore.Delete, and make
> AppEngineBlobstore.Delete return the value if it successfully deletes the
item,
> or None otherwise. That's pretty standard for a map API like this.
> 
> Then the code would be like
> 
> blob_key = self._datastore.Delete(datastore.BLOB_REFERENCE_BLOBSTORE, key)
> if blob_key is None:
>   return
> blob_key.delete()

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/blob_reference_store.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/blob_reference_store.py:8:
BLOBREFERENCE_BLOBSTORE = 'BlobReferenceBlobstore'
On 2012/08/10 06:02:18, kalman wrote:
> should be BLOB_REFERENCE_BLOBSTORE really...

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:111:
manifest_data['github_path'] = (url_constants.GITHUB_BASE + '/' +
On 2012/08/10 06:02:18, kalman wrote:
> See my comment in sample_item.html. Let's just have a single path/url value,
> leave the logic in here.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:142: def
FilterSamples(self, key, api_name):
On 2012/08/10 06:02:18, kalman wrote:
> documentation? what is "key"?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:143: api_search =
'_' + api_name + '_'
On 2012/08/10 06:02:18, kalman wrote:
> unit test?

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:154: # If we're
testing, the GithubFileSystem can't fetch samples.
On 2012/08/10 06:02:18, kalman wrote:
> see comment about testing the github stuff.... you could mention it here too

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/private/sample_item.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/sample_item.html:3:
<h2><a
href="{{?is_apps}}{{github_path}}{{:}}{{path}}.zip{{/}}">{{name}}</a></h2>
On 2012/08/10 06:02:18, kalman wrote:
> Why do we need github_path at all? Can we just make sure that it's set right
> from samples_data_source?
> 
> Perhaps call it url.
> 
> Then we wouldn't need is_apps. Woo.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/private/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/samples.html:9: <div
id="gc-container">
On 2012/08/10 06:02:18, kalman wrote:
> So, we should only use is_apps if absolutely necessary. Using is_apps makes
the
> templates less reusable, generally harder to follow.
> 
> Instead, inject
>  - the sidenav
>  - the title (I will definitely try to write that patch, though if I don't you
> can always define "apps_title" and "extensions_title" like you did for
> true/false in the dict generator).
>  - the API list
>  - the samples
>  - the footer
> 
> At this point you might want to document this template like you would a
> function, with a comment at the top explaining what the parameters are.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/samples.html:49:
{{+partials.footer is_apps:is_apps}}
On 2012/08/10 06:02:18, kalman wrote:
> The only difference in the footer is the bootstrapped samples JSON. We used to
> do this in a separate <script> tag before including the footer; perhaps we
> should have apps_footer and extensions_footer which does this (each would set
up
> the right bootstrap arguments, then call the generic footer thing).

Done.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:27:
{{+partials.api_reference is_apps:true}}
On 2012/08/10 06:02:18, kalman wrote:
> see previous comments re is_apps

Done.

[not at google - send to devlin, 2012-08-13 01:51:11]

Last round of reviews. lg after this I promise.

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source_test.py:37:
test1.pop('samples')
On 2012/08/10 21:17:47, cduvall wrote:
> On 2012/08/10 06:02:18, kalman wrote:
> > Why not just add the samples to the expected data? You know what it's
supposed
> > to be (since you're controlling it in FakeSamplesDataSource).
> 
> The samples isn't actually a SamplesDataSource, its a _LazySamplesGetter. This
> makes it so the test will fail, because the expected JSON doesn't have a
> _LazySamplesGetter.

Ah I see.

Could you define __eq__ for Future, which compares values? Make
FakeSamplesDataSource return a Future(value={})? It would be really nice to
compare to the real result.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:77: perms =
dict([(model.UnixName(k), v) for k, v in perms.iteritems()])
i think that just dict(...) will work, don't need dict([...]). generators and
whatnot.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:81:
self.assertTrue(len(response.out.getvalue()) > 100000)
Oh is it possible to put a failure message here? Like,

value = response.out.getvalue()
self.assertTrue(len(value) > 1000000,
                "Response is too small, probably empty: %s" % value)

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:15:
{{+partials.table_of_contents toc:intro.toc samples_list:api.samples.apps
title:apps_title}}
IIRC I recently fixed a bug in handlebar which would let you do

{{+partials.tab_of_contents toc:intro.toc
                            samples_list:api.samples.apps
                            title:apps_title}}

if you wanted. Might be neater.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:18:
{{+partials.api_reference samples_list:api.samples.apps title:apps_title}}
ditto

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/private/standard_extensions_api.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_extensions_api.html:15:
{{+partials.table_of_contents toc:intro.toc samples_list:api.samples.extensions
title:extensions_title}}
ditto

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_extensions_api.html:18:
{{+partials.api_reference samples_list:api.samples.extensions
title:extensions_title}}
ditto

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/public/apps/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/apps/samples.html:1:
{{+partials.samples sidenav:partials.apps_sidenav
filter_list:api_list.apps.chrome samples_list:samples.apps
footer:partials.apps_footer title:apps_title}}
ditto

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/public/extensions/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/extensions/samples.html:1:
{{+partials.samples sidenav:partials.extensions_sidenav
filter_list:api_list.extensions.chrome samples_list:samples.extensions
footer:partials.extensions_footer title:extensions_title}}
ditto

[cduvall, 2012-08-13 17:47:06]

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:77: perms =
dict([(model.UnixName(k), v) for k, v in perms.iteritems()])
On 2012/08/13 01:51:11, kalman wrote:
> i think that just dict(...) will work, don't need dict([...]). generators and
> whatnot.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File chrome/common/extensions/docs/server2/integration_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/integration_test.py:81:
self.assertTrue(len(response.out.getvalue()) > 100000)
On 2012/08/13 01:51:11, kalman wrote:
> Oh is it possible to put a failure message here? Like,
> 
> value = response.out.getvalue()
> self.assertTrue(len(value) > 1000000,
>                 "Response is too small, probably empty: %s" % value)

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:15:
{{+partials.table_of_contents toc:intro.toc samples_list:api.samples.apps
title:apps_title}}
On 2012/08/13 01:51:11, kalman wrote:
> IIRC I recently fixed a bug in handlebar which would let you do
> 
> {{+partials.tab_of_contents toc:intro.toc
>                             samples_list:api.samples.apps
>                             title:apps_title}}
> 
> if you wanted. Might be neater.

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:18:
{{+partials.api_reference samples_list:api.samples.apps title:apps_title}}
On 2012/08/13 01:51:11, kalman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/private/standard_extensions_api.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_extensions_api.html:15:
{{+partials.table_of_contents toc:intro.toc samples_list:api.samples.extensions
title:extensions_title}}
On 2012/08/13 01:51:11, kalman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/private/standard_extensions_api.html:18:
{{+partials.api_reference samples_list:api.samples.extensions
title:extensions_title}}
On 2012/08/13 01:51:11, kalman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File chrome/common/extensions/docs/server2/templates/public/apps/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/apps/samples.html:1:
{{+partials.samples sidenav:partials.apps_sidenav
filter_list:api_list.apps.chrome samples_list:samples.apps
footer:partials.apps_footer title:apps_title}}
On 2012/08/13 01:51:11, kalman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
File
chrome/common/extensions/docs/server2/templates/public/extensions/samples.html
(right):

https://chromiumcodereview.appspot.com/10825067/diff/8010/chrome/common/exten...
chrome/common/extensions/docs/server2/templates/public/extensions/samples.html:1:
{{+partials.samples sidenav:partials.extensions_sidenav
filter_list:api_list.extensions.chrome samples_list:samples.extensions
footer:partials.extensions_footer title:extensions_title}}
On 2012/08/13 01:51:11, kalman wrote:
> ditto

Done.

[not at google - send to devlin, 2012-08-13 22:39:08]

lgtm

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://chromiumcodereview.appspot.com/10825067/diff/1044/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source_test.py:37:
test1.pop('samples')
On 2012/08/13 01:51:11, kalman wrote:
> On 2012/08/10 21:17:47, cduvall wrote:
> > On 2012/08/10 06:02:18, kalman wrote:
> > > Why not just add the samples to the expected data? You know what it's
> supposed
> > > to be (since you're controlling it in FakeSamplesDataSource).
> > 
> > The samples isn't actually a SamplesDataSource, its a _LazySamplesGetter.
This
> > makes it so the test will fail, because the expected JSON doesn't have a
> > _LazySamplesGetter.
> 
> Ah I see.
> 
> Could you define __eq__ for Future, which compares values? Make
> FakeSamplesDataSource return a Future(value={})? It would be really nice to
> compare to the real result.

(sorry about this comment, my brain did s/_LazySamplesGetter/Future/ without
warning me)