Extensions Docs Server: Internationalized samples

Extensions Docs Server: Internationalized samples

The samples page can now handle names and descriptions of the form '__MSG_...',
and the locale is chosen based on the Accept-Languages HTTP header.

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

[cduvall, 2012-07-19 21:44:12]

This requires that Handlebar uses unicode() instead of str() to convert data.
Try setting your language to Spanish, and Handlebar will fail on special
characters.

[not at google - send to devlin, 2012-07-20 01:28:21]

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

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:8: class
SamplesDataSource(object):
See comment in template_data_source... this needs a Factory to get injected into
TDS and then TDS creates one of these bound to a request too.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:15: self._headers =
{}
let's just bind this to the request. Easier to understand what that is than
"headers".

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:28: return
self._fetcher.Read([filename]).Get()[filename]
Seen this pattern in a couple of places now.

Would it work to add a method to FileSystem called ReadSingleFile which just
returns that one file calling the interface methods? If the subclasses implement
it properly then should all work?

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:35: if
name.startswith('__MSG_') or description.startswith('__MSG_'):
Won't this cache only the language in the header? We need to cache all the
languages. Algorithm should be like...

manifest = self._FetchFile(path + '/manfiest.json')
manifest_json = manifest_json.loads(manifest)

data = {
  name: manifest_json.get('name', ''),
  description: manifest_json.get('description', ''),
  default_locale: manifest_json.get('default_local', None)
}

locale_dirs = self._fetcher.Read(path + '/_locales').Get()
if local_dirs:
  locales = {}
  for locale in locale_dirs:
    locales[locale] = manifest_json.loads(
        self._FetchFile(path + '/_locales/' + locale))
  data.locales = locales

then in get(), when setting name/description, detect if there's a __MSG_ and
internationalise it (based on the headers).

if that makes sense.

in any case, once this cache moves into the Factory you won't be able to access
the request anymore, anyway.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:37: if
'Accept-Language' in self._headers:
for thsi kind of thing, would prefer to see somethign like

accepted_languages = _GetAcceptedLanguages(
    self._request.headers['Accept-Languages'])
accepted_languages.add(manifest.default_locale)
... etc

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:41: languages =
[l.replace('-', '_') for l in languages]
l as a short variable is like the most confusing short variable name ever. Or is
that a 1?

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/template_data_source.py:80:
self._samples_data_source.SetHeaders(request.headers)
This isn't safe with multiple requests needing to access it at once.
SamplesDataSource needs the same treatment that TemplateDataSource got, with the
Factory.

[cduvall, 2012-07-20 19:40:50]

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

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:8: class
SamplesDataSource(object):
On 2012/07/20 01:28:21, kalman wrote:
> See comment in template_data_source... this needs a Factory to get injected
into
> TDS and then TDS creates one of these bound to a request too.

Done.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:15: self._headers =
{}
On 2012/07/20 01:28:21, kalman wrote:
> let's just bind this to the request. Easier to understand what that is than
> "headers".

Done.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:28: return
self._fetcher.Read([filename]).Get()[filename]
On 2012/07/20 01:28:21, kalman wrote:
> Seen this pattern in a couple of places now.
> 
> Would it work to add a method to FileSystem called ReadSingleFile which just
> returns that one file calling the interface methods? If the subclasses
implement
> it properly then should all work?

Done.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:35: if
name.startswith('__MSG_') or description.startswith('__MSG_'):
On 2012/07/20 01:28:21, kalman wrote:
> Won't this cache only the language in the header? We need to cache all the
> languages. Algorithm should be like...
> 
> manifest = self._FetchFile(path + '/manfiest.json')
> manifest_json = manifest_json.loads(manifest)
> 
> data = {
>   name: manifest_json.get('name', ''),
>   description: manifest_json.get('description', ''),
>   default_locale: manifest_json.get('default_local', None)
> }
> 
> locale_dirs = self._fetcher.Read(path + '/_locales').Get()
> if local_dirs:
>   locales = {}
>   for locale in locale_dirs:
>     locales[locale] = manifest_json.loads(
>         self._FetchFile(path + '/_locales/' + locale))
>   data.locales = locales
> 
> then in get(), when setting name/description, detect if there's a __MSG_ and
> internationalise it (based on the headers).
> 
> if that makes sense.
> 
> in any case, once this cache moves into the Factory you won't be able to
access
> the request anymore, anyway.

Done.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:37: if
'Accept-Language' in self._headers:
On 2012/07/20 01:28:21, kalman wrote:
> for thsi kind of thing, would prefer to see somethign like
> 
> accepted_languages = _GetAcceptedLanguages(
>     self._request.headers['Accept-Languages'])
> accepted_languages.add(manifest.default_locale)
> ... etc

Done.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:41: languages =
[l.replace('-', '_') for l in languages]
On 2012/07/20 01:28:21, kalman wrote:
> l as a short variable is like the most confusing short variable name ever. Or
is
> that a 1?

Very true, sorry about that.

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/template_data_source.py:80:
self._samples_data_source.SetHeaders(request.headers)
On 2012/07/20 01:28:21, kalman wrote:
> This isn't safe with multiple requests needing to access it at once.
> SamplesDataSource needs the same treatment that TemplateDataSource got, with
the
> Factory.

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/third_party/handle...
File third_party/handlebar/handlebar.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/10001/third_party/handle...
third_party/handlebar/handlebar.py:71: string = unicode(obj)
I'll take these out before I commit, just showing what needs to be changed.

[not at google - send to devlin, 2012-07-23 13:25:34]

Seeing the samples there, in French... kinda surreal, and very funny. In a good
way.

I was trying to verify that it worked for the subversion-backed stuff, but I
can't make it work without timing out. Neither the samples page nor normal pages
(not even trunk/index.html). Has something changed/broken along the way?

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

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/file_system.py:21: def
ReadSingleFile(self, path):
oh this works for directories as well as files. Oops my bad, "ReadSingle" or
something would be more appropriate.

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:9: """ Constructs a
list of samples and their respective files and api calls.
Heh, I think these leading spaces are just me being silly. Apparently you're not
supposed to be leading spaces in docstrings :\ so I'll stop doing it.

Looks better though. Whatever...

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:64: sample_files =
filter(lambda x: x.startswith(sample_path + '/'), files)
randomly browsing code here, but maybe this should be a list comprehension like

[path for path in files if path.startswith(sample_path + '/')]

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:106:
self._request.headers['Accept-Language']))
Using regex is kind of unnecessary here? And almost certainly not compliant.

I think we want to do something along the lines of

accept_language = self._request.headers.get('Accept-Language', None)
if accept_language is None:
  return []
return [lang_with_q.split(';')[0].strip()
        for lang_with_q in accept_language.split(',')]

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:116:
accepted_languages = self._GetAcceptedLanguages()
small optimisation: inline this where it's used (don't need to parse header
unless we... actually need to)

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:119: sample_data =
dict_.copy()
small optimisation: can avoid copying this dict until after the "if
name.startswith('__MSG__')" etc check.

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:123: name_key =
name[len('__MSG_'):-2]
nit: instead of -2 have -len('__') perhaps? Makes it clearer that that's what
we're stripping out.

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:133:
sample_data['locales'][locale][description_key]['message'])
nit: 
locale_data = sample_data['locales'][locale]
sample_data['name'] = local_data[name_key]['message']
...

http://codereview.chromium.org/10804036/diff/10001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:134:
return_list.append(sample_data)
This is all super cool.

However, there seems to be a bunch of stuff that could go wrong, namely 
examples that have been misconfigured (having an internationalised description
but not name and vice versa, not having a internationalised key for some locale,
etc). These will throw exceptions and get turned into 500s presumably, but it
might be nice to not break the whole samples page because somebody accidentally
misspelled some __MSG_BLAH.

http://codereview.chromium.org/10804036/diff/10001/third_party/handlebar/hand...
File third_party/handlebar/handlebar.py (right):

http://codereview.chromium.org/10804036/diff/10001/third_party/handlebar/hand...
third_party/handlebar/handlebar.py:71: string = unicode(obj)
On 2012/07/20 19:40:51, cduvall wrote:
> I'll take these out before I commit, just showing what needs to be changed.

Several hours later, and I know more than I really wanted about Python's unicode
system.

But I've pushed the "real" way to fix this problem into github now (after a
false start).

[cduvall, 2012-07-23 18:10:37]

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

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/file_system.py:21: def
ReadSingleFile(self, path):
On 2012/07/23 13:25:37, kalman wrote:
> oh this works for directories as well as files. Oops my bad, "ReadSingle" or
> something would be more appropriate.

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:9: """ Constructs a
list of samples and their respective files and api calls.
On 2012/07/23 13:25:37, kalman wrote:
> Heh, I think these leading spaces are just me being silly. Apparently you're
not
> supposed to be leading spaces in docstrings :\ so I'll stop doing it.
> 
> Looks better though. Whatever...

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:64: sample_files =
filter(lambda x: x.startswith(sample_path + '/'), files)
On 2012/07/23 13:25:37, kalman wrote:
> randomly browsing code here, but maybe this should be a list comprehension
like
> 
> [path for path in files if path.startswith(sample_path + '/')]

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:106:
self._request.headers['Accept-Language']))
On 2012/07/23 13:25:37, kalman wrote:
> Using regex is kind of unnecessary here? And almost certainly not compliant.
> 
> I think we want to do something along the lines of
> 
> accept_language = self._request.headers.get('Accept-Language', None)
> if accept_language is None:
>   return []
> return [lang_with_q.split(';')[0].strip()
>         for lang_with_q in accept_language.split(',')]

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:116:
accepted_languages = self._GetAcceptedLanguages()
On 2012/07/23 13:25:37, kalman wrote:
> small optimisation: inline this where it's used (don't need to parse header
> unless we... actually need to)

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:119: sample_data =
dict_.copy()
On 2012/07/23 13:25:37, kalman wrote:
> small optimisation: can avoid copying this dict until after the "if
> name.startswith('__MSG__')" etc check.

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:123: name_key =
name[len('__MSG_'):-2]
On 2012/07/23 13:25:37, kalman wrote:
> nit: instead of -2 have -len('__') perhaps? Makes it clearer that that's what
> we're stripping out.

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:133:
sample_data['locales'][locale][description_key]['message'])
On 2012/07/23 13:25:37, kalman wrote:
> nit: 
> locale_data = sample_data['locales'][locale]
> sample_data['name'] = local_data[name_key]['message']
> ...

Done.

https://chromiumcodereview.appspot.com/10804036/diff/10001/chrome/common/exte...
chrome/common/extensions/docs/server2/samples_data_source.py:134:
return_list.append(sample_data)
On 2012/07/23 13:25:37, kalman wrote:
> This is all super cool.
> 
> However, there seems to be a bunch of stuff that could go wrong, namely 
> examples that have been misconfigured (having an internationalised description
> but not name and vice versa, not having a internationalised key for some
locale,
> etc). These will throw exceptions and get turned into 500s presumably, but it
> might be nice to not break the whole samples page because somebody
accidentally
> misspelled some __MSG_BLAH.

Done. I just caught an exception and reverted the samples_dict to the
un-localized version.

https://chromiumcodereview.appspot.com/10804036/diff/10001/third_party/handle...
File third_party/handlebar/handlebar.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/10001/third_party/handle...
third_party/handlebar/handlebar.py:71: string = unicode(obj)
On 2012/07/23 13:25:37, kalman wrote:
> On 2012/07/20 19:40:51, cduvall wrote:
> > I'll take these out before I commit, just showing what needs to be changed.
> 
> Several hours later, and I know more than I really wanted about Python's
unicode
> system.
> 
> But I've pushed the "real" way to fix this problem into github now (after a
> false start).

Nice, I had some fun exploring Python unicode too. I committed the Handlebar
changes.

[cduvall, 2012-07-23 19:39:41]

On 2012/07/23 13:25:34, kalman wrote:
> Seeing the samples there, in French... kinda surreal, and very funny. In a
good
> way.
> 
> I was trying to verify that it worked for the subversion-backed stuff, but I
> can't make it work without timing out. Neither the samples page nor normal
pages
> (not even trunk/index.html). Has something changed/broken along the way?

Were you trying to view it on the local server? for some reason the dev server
doesn't handle async fetches very well. It seems to be working at
http://chrome-apps-doc2.appspot.com/trunk/ .

Also, I just saw that the new Handlebar changes broke templates that have
unicode characters. For example, the footer won't render anymore because of the
copyright symbol.

[cduvall, 2012-07-23 21:04:20]

https://chromiumcodereview.appspot.com/10804036/diff/2005/third_party/handleb...
File third_party/handlebar/handlebar.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/2005/third_party/handleb...
third_party/handlebar/handlebar.py:595: self.source = unicode(template, 'utf-8')
This fix makes it render the footer, but this might not be the best way to do
it.

[not at google - send to devlin, 2012-07-24 01:23:30]

what you have lg, though there will presumably be a bit more work to get the
encodings going properly before you can actually submit this.

(or is the encoding problem separate? in which case, lgtm and submit)

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

https://chromiumcodereview.appspot.com/10804036/diff/2005/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:117: # Copy the
sample dict so we don't change the dict in the cache.
comment needs to be moved...

https://chromiumcodereview.appspot.com/10804036/diff/2005/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:126:
logging.info(self._GetAcceptedLanguages())
remove

https://chromiumcodereview.appspot.com/10804036/diff/2005/chrome/common/exten...
chrome/common/extensions/docs/server2/samples_data_source.py:136: # Revert the
sample to the original dict.
Pretty heavy-handed way to do this :\ oh well. So Exception catches...
everything? I guess when it does happen we can be notified (via logging) and go
and fix the sample, so no biggie.

logging should definitely be stronger than info though, at least warning if not
error. Error preferably.

https://chromiumcodereview.appspot.com/10804036/diff/2005/third_party/handleb...
File third_party/handlebar/handlebar.py (right):

https://chromiumcodereview.appspot.com/10804036/diff/2005/third_party/handleb...
third_party/handlebar/handlebar.py:595: self.source = unicode(template, 'utf-8')
On 2012/07/23 21:04:20, cduvall wrote:
> This fix makes it render the footer, but this might not be the best way to do
> it.

Yeah; as discussed in IRC, let's fix this at the FileSystem level.

[cduvall, 2012-07-24 21:22:46]

This isn't handling encodings yet. I'm still researching the best way to handle
that.

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

http://codereview.chromium.org/10804036/diff/2005/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:117: # Copy the
sample dict so we don't change the dict in the cache.
On 2012/07/24 01:23:31, kalman wrote:
> comment needs to be moved...

Done.

http://codereview.chromium.org/10804036/diff/2005/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:126:
logging.info(self._GetAcceptedLanguages())
On 2012/07/24 01:23:31, kalman wrote:
> remove

Done.

http://codereview.chromium.org/10804036/diff/2005/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:136: # Revert the
sample to the original dict.
On 2012/07/24 01:23:31, kalman wrote:
> Pretty heavy-handed way to do this :\ oh well. So Exception catches...
> everything? I guess when it does happen we can be notified (via logging) and
go
> and fix the sample, so no biggie.
> 
> logging should definitely be stronger than info though, at least warning if
not
> error. Error preferably.

Done.

http://codereview.chromium.org/10804036/diff/2005/third_party/handlebar/handl...
File third_party/handlebar/handlebar.py (right):

http://codereview.chromium.org/10804036/diff/2005/third_party/handlebar/handl...
third_party/handlebar/handlebar.py:595: self.source = unicode(template, 'utf-8')
On 2012/07/24 01:23:31, kalman wrote:
> On 2012/07/23 21:04:20, cduvall wrote:
> > This fix makes it render the footer, but this might not be the best way to
do
> > it.
> 
> Yeah; as discussed in IRC, let's fix this at the FileSystem level.

Done.

[not at google - send to devlin, 2012-07-24 23:49:12]

Cool. lmk when encodings are working.

[cduvall, 2012-07-25 01:04:37]

This is what I have for encodings. I went for the
try-utf8-and-if-that-doesn't-work-use-latin1 approach. I think this will work
fine, and might be the best we can do without knowing the encodings of the
files.

[not at google - send to devlin, 2012-07-25 01:44:14]

lgtm, let's submit this.

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/intro_data_source.py:50: self.page_title
+= data
see previous comment about this?

(i.e. remove else clause)

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/static/css/samples.css (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/static/css/samples.css:37: min-height:
700px;
why? this kind of stuff assumes things about screen sizes. what does it achieve?

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/subversion_file_system.py:49: if
os.path.splitext(path)[-1] in ['.js', '.html', '.json']:
This is the same code as in LocalFileSystem. Pull into FileSystem?

[cduvall, 2012-07-25 03:19:25]

Just responding to the comments, I'll commit tomorrow morning.

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/intro_data_source.py:50: self.page_title
+= data
On 2012/07/25 01:44:14, kalman wrote:
> see previous comment about this?
> 
> (i.e. remove else clause)

The else clause is needed because sometimes the data is split up in weird ways
by the HTMLParser. Only the first h1 will be taken as the page title though (see
line 27). All the other h1 tags are ignored once the title is set.

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/static/css/samples.css (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/static/css/samples.css:37: min-height:
700px;
On 2012/07/25 01:44:14, kalman wrote:
> why? this kind of stuff assumes things about screen sizes. what does it
achieve?

I did this so the footer text doesn't come up into the sidebar when there are no
samples showing. I can take this out, and we can deal with minor formatting
things like that later though.

[cduvall, 2012-07-25 18:21:33]

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/static/css/samples.css (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/static/css/samples.css:37: min-height:
700px;
On 2012/07/25 03:19:26, cduvall wrote:
> On 2012/07/25 01:44:14, kalman wrote:
> > why? this kind of stuff assumes things about screen sizes. what does it
> achieve?
> 
> I did this so the footer text doesn't come up into the sidebar when there are
no
> samples showing. I can take this out, and we can deal with minor formatting
> things like that later though.

Used float left like you said in your email.

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

http://codereview.chromium.org/10804036/diff/28002/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/subversion_file_system.py:49: if
os.path.splitext(path)[-1] in ['.js', '.html', '.json']:
On 2012/07/25 01:44:14, kalman wrote:
> This is the same code as in LocalFileSystem. Pull into FileSystem?

Done.