Extension docs: Include sidenav in 404 pages

Extension docs: Include sidenav in 404 pages

Include sidenav in 404 pages of extension/app docs to make them easy to
navigate away from and look consistent with other pages.

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

[方觉(Fang Jue), 2013-03-26 05:46:04]

This is what it looks like with the patch:
http://goo.gl/5ISgn
http://goo.gl/plk33

I didn't remove public/404.html so that 404 pages won't break before the new
version of server2 is deployed.

[方觉(Fang Jue), 2013-03-26 13:52:11]

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/private/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:9: position: relative;
This fixes problem with scrolling on 404 pages (without it you cannot scroll
down to the bottom of the sidenav on a 404 page).

[not at google - send to devlin, 2013-03-26 16:12:19]

awesome, thanks for looking at this.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:78:
response.out.write(templates.Render('extensions/404'))
As a somewhat separate issue, we need to move away from having just an apps and
an extensions mode for the server, since there's a good chance we're going to
start supporting more classes.

There will be plenty of other work needed to support this - but I don't want to
cause unnecessary friction here.

You should be able to list the files in the directory containing the broken link
via _cache.GetFromFileListing, then step up the hierarchy until a 404 file is
found.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/private/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:6: <base
href="/{{branchInfo.current}}/{{platform}}s/">
<base>? Can we avoid?

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:9: position: relative;
On 2013/03/26 13:52:11, 方觉(Fang Jue) wrote:
> This fixes problem with scrolling on 404 pages (without it you cannot scroll
> down to the bottom of the sidenav on a 404 page).

What do you mean? I patched in this change and it appears to work without it.

(Incidentally, if it fixes something here it probably fixes something elsewhere
too, so it should be in site.css).

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:18: <h1
class="page_title">404 - Not Found</h1>
The 404 here has always irritated me. Let's just make this "Not found".

Also we don't use page_title anymore.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:28:
{{+partials.footer}}
I think if we make this an article (articles/404.html) then we can get some of
this for free.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/public/apps/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/public/apps/404.html:1: {{+partials.404
platform:app title:apps_title sidenav_items:sidenavs.apps}}
see comment about article

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/public/extensions/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/public/extensions/404.html:1:
{{+partials.404 platform:extension title:extensions_title
sidenav_items:sidenavs.extensions}}
ditto

[方觉(Fang Jue), 2013-03-26 23:44:03]

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
> File chrome/common/extensions/docs/templates/private/404.html (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
> chrome/common/extensions/docs/templates/private/404.html:6: <base
> href="/{{branchInfo.current}}/{{platform}}s/">
> <base>? Can we avoid?
> 
Unfortunately no. If you go to localhost:8001/static/auouo/oeeauo.css (for
example), you'll not be able to navigate back to a right page in the sidenav
because all links are relative.

>
https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
> chrome/common/extensions/docs/templates/private/404.html:9: position:
relative;
> On 2013/03/26 13:52:11, 方觉(Fang Jue) wrote:
> > This fixes problem with scrolling on 404 pages (without it you cannot scroll
> > down to the bottom of the sidenav on a 404 page).
> 
> What do you mean? I patched in this change and it appears to work without it.
> 
> (Incidentally, if it fixes something here it probably fixes something
elsewhere
> too, so it should be in site.css).
> 
This happens when the height of content is less than sidenav and the sidenav
doesn't fit in the screen. Maybe you're using a larger screen?

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
> chrome/common/extensions/docs/templates/private/404.html:28:
> {{+partials.footer}}
> I think if we make this an article (articles/404.html) then we can get some of
> this for free.
> 
As explained above, <base> is necessary. So what to do considering this?

[方觉(Fang Jue), 2013-03-27 00:43:46]

Other comments addressed.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/server_instance.py:78:
response.out.write(templates.Render('extensions/404'))
I tried to extract the first component of path and treat it as doc class. Then
try to render the 404 page of that class. If it fails, render extensions/404. Is
that ok?

On 2013/03/26 16:12:19, kalman wrote:
> As a somewhat separate issue, we need to move away from having just an apps
and
> an extensions mode for the server, since there's a good chance we're going to
> start supporting more classes.
> 
> There will be plenty of other work needed to support this - but I don't want
to
> cause unnecessary friction here.
> 
> You should be able to list the files in the directory containing the broken
link
> via _cache.GetFromFileListing, then step up the hierarchy until a 404 file is
> found.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/private/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:18: <h1
class="page_title">404 - Not Found</h1>
On 2013/03/26 16:12:19, kalman wrote:
> The 404 here has always irritated me. Let's just make this "Not found".
> 
> Also we don't use page_title anymore.

Done.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/private/404.html:28:
{{+partials.footer}}
On 2013/03/26 16:12:19, kalman wrote:
> I think if we make this an article (articles/404.html) then we can get some of
> this for free.

Done.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/public/apps/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/public/apps/404.html:1: {{+partials.404
platform:app title:apps_title sidenav_items:sidenavs.apps}}
On 2013/03/26 16:12:19, kalman wrote:
> see comment about article

Done.

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/templates/public/extensions/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/templates/public/extensions/404.html:1:
{{+partials.404 platform:extension title:extensions_title
sidenav_items:sidenavs.extensions}}
On 2013/03/26 16:12:19, kalman wrote:
> ditto

Done.

[not at google - send to devlin, 2013-03-27 03:52:36]

https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
File chrome/common/extensions/docs/templates/articles/404.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
chrome/common/extensions/docs/templates/articles/404.html:2: <p>No API or URL
found with that name.</p>
in more hindsight the message should also say "article" rather than "URL".

https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
File chrome/common/extensions/docs/templates/private/header_head.html (right):

https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
chrome/common/extensions/docs/templates/private/header_head.html:7: {{?is_404}}
I really don't want to add logic like this into shared files. Or any files
really.

https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
chrome/common/extensions/docs/templates/private/header_head.html:8: <base
href="/{{branchInfo.current}}/{{platform}}s/">
I see. Yeah, there are a lot of assumptions everywhere that we're dealing with
paths that look like either /foo or /extensions/foo. This particular issue will
have to wait a little while to fix globally until we don't need to deal with the
differences between dev/beta/trunk (coming soon), but that's beside the point...

Anyway, my preferred way to fix this is to make the sidebar generate correct
absolute paths. See templates/private/sidenav_item.html, line 9.

https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
chrome/common/extensions/docs/templates/private/header_head.html:11: position:
relative;
right, but you can still scroll the sidebar itself.

I agree that it does look a bit bad, though. We're better off fixing this with
more javascript though, in chrome/common/extensions/docs/static/js/scroll.js
itself, by comparing the height of the sidebar to the window height and applying
position:relative rather than fixed if it's bigger.

But that can be a separate change.

[方觉(Fang Jue), 2013-03-27 05:01:30]

On 2013/03/27 03:52:36, kalman wrote:
>
https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
> File chrome/common/extensions/docs/templates/articles/404.html (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
> chrome/common/extensions/docs/templates/articles/404.html:2: <p>No API or URL
> found with that name.</p>
> in more hindsight the message should also say "article" rather than "URL".
> 
It was copied from the original 404.html. Changed to "article".

>
https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
> File chrome/common/extensions/docs/templates/private/header_head.html (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
> chrome/common/extensions/docs/templates/private/header_head.html:7:
{{?is_404}}
> I really don't want to add logic like this into shared files. Or any files
> really.
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
> chrome/common/extensions/docs/templates/private/header_head.html:8: <base
> href="/{{branchInfo.current}}/{{platform}}s/">
> I see. Yeah, there are a lot of assumptions everywhere that we're dealing with
> paths that look like either /foo or /extensions/foo. This particular issue
will
> have to wait a little while to fix globally until we don't need to deal with
the
> differences between dev/beta/trunk (coming soon), but that's beside the
point...
> 
> Anyway, my preferred way to fix this is to make the sidebar generate correct
> absolute paths. See templates/private/sidenav_item.html, line 9.
> 
So it means using absolute paths in sidenav for _every_ page?

>
https://chromiumcodereview.appspot.com/12521030/diff/14001/chrome/common/exte...
> chrome/common/extensions/docs/templates/private/header_head.html:11: position:
> relative;
> right, but you can still scroll the sidebar itself.
> 
> I agree that it does look a bit bad, though. We're better off fixing this with
> more javascript though, in chrome/common/extensions/docs/static/js/scroll.js
> itself, by comparing the height of the sidebar to the window height and
applying
> position:relative rather than fixed if it's bigger.
> 
> But that can be a separate change.
Yes. I did consider fixing this with JavaScript but came up with this trick
first.
I'll remove this and thus removing the need of {{?is_404}}.

[方觉(Fang Jue), 2013-03-27 14:38:10]

After I used absolute paths in sidenav (in patchset 5), another problem arose.
For stable channel, links in sidenav point to /stable/extensions/xxx.html, which
involves a redirect. I tried to work around it in patchset 6 by adding a
`currentUrl` property in branchInfo.

[not at google - send to devlin, 2013-03-28 00:39:13]

hey looks good, but I'm heading on vacation so won't be able to get to this
until early next week.

[not at google - send to devlin, 2013-03-28 00:39:32]

(if I haven't gotten back to you by end of Tuesday pacific time, ping me again)

[not at google - send to devlin, 2013-04-27 01:10:13]

Hey - let's get back to this. I fixed the sidebar scrolling thing separately
(although haven't merged into stable yet - need to do that - also it seems to
have a bug sometimes) - but you should merge that in and I'd like to submit
this.

[方觉(Fang Jue), 2013-04-27 01:12:47]

On 2013/04/27 01:10:13, kalman wrote:
> Hey - let's get back to this. I fixed the sidebar scrolling thing separately
> (although haven't merged into stable yet - need to do that - also it seems to
> have a bug sometimes) - but you should merge that in and I'd like to submit
> this.

This change has involved both server side and template changes. Shouldn't we
wait for https://codereview.chromium.org/14246037/ first?

[not at google - send to devlin, 2013-04-27 01:29:38]

Ok fair enough :)

[not at google - send to devlin, 2013-05-23 17:06:24]

ping

[方觉(Fang Jue), 2013-05-23 23:37:58]

On 2013/05/23 17:06:24, kalman wrote:
> ping

I noticed that there're absolute URLs in *_sidenav.json and simply prepending a
prefix (like /trunk/extensions/) in templates won't work. What about passing it
to sidenav_data_source and let it convert them to absolute paths (and do nothing
if it's an absolute URL)?

And there's another link (Samples) in header_body.html whose href is a relative
path.

[not at google - send to devlin, 2013-05-23 23:39:02]

yep, sidenav_data_source should deal with it.

[方觉(Fang Jue), 2013-05-24 01:30:07]

Extension docs: Include sidenav in 404 pages

Include sidenav in 404 pages of extension/app docs to make them easy to
navigate away from and look consistent with other pages.

BUG=223579

[方觉(Fang Jue), 2013-05-24 02:02:41]

Docs staged at
https://chrome-apps-doc-test.appspot.com/_patch/12521030/extensions/getstarte....
Test 404 pages:
https://chrome-apps-doc-test.appspot.com/_patch/12521030/extensions/aeuoeu.html
https://chrome-apps-doc-test.appspot.com/_patch/12521030/apps/eauoe.html
https://chrome-apps-doc-test.appspot.com/_patch/12521030/euae/oaeueo

[not at google - send to devlin, 2013-05-24 16:58:28]

thanks!

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/render_servlet.py:98: content =
templates.Render('404')
third branch here seems unnecessary. we should always have an extensions/404,
you can assert that it's there.

and delete the top-level 404.html. I believe there is also a reference to it in
the integration test.

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/sidenav_data_source.py:66: fileName =
item.get('fileName')
file_name = ...

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/sidenav_data_source.py:78:
self._ConvertToAbsolutePath(sidenav, key)
it would be more correct to use the path of the request for this, not the key.
As it is, this assumes that code under foo/ will request foo.json which may not
always be the case.

in fact that might make it unnecessary to pass in the base path here at all.

[方觉(Fang Jue), 2013-05-25 00:08:38]

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/render_servlet.py:98: content =
templates.Render('404')
On 2013/05/24 16:58:29, kalman wrote:
> third branch here seems unnecessary. we should always have an extensions/404,
> you can assert that it's there.
> 
> and delete the top-level 404.html. I believe there is also a reference to it
in
> the integration test.

Yes. It's unnecessary. But before the new 404 templates are merged to stable,
beta, dev, it's not yet always true.

Another choice is to increase the mid version in app.yaml. It seems that
revision pinning also works with branches?

The integration test tries to request 'extensions/notfound.html' and there's no
reference to it. (anyway, presubmit passed)

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/sidenav_data_source.py:66: fileName =
item.get('fileName')
On 2013/05/24 16:58:29, kalman wrote:
> file_name = ...

Done.

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/sidenav_data_source.py:78:
self._ConvertToAbsolutePath(sidenav, key)
On 2013/05/24 16:58:29, kalman wrote:
> it would be more correct to use the path of the request for this, not the key.
> As it is, this assumes that code under foo/ will request foo.json which may
not
> always be the case.
> 
> in fact that might make it unnecessary to pass in the base path here at all.

path here is something like 'extensions/index.html', which doesn't contain the
channel. So base path (will be /trunk for example) is necessary.

Use path here seems to be more correct. Done.

[not at google - send to devlin, 2013-05-28 14:53:37]

sorry was a public holiday yesterday.

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/render_servlet.py:98: content =
templates.Render('404')
On 2013/05/25 00:08:39, 方觉(Fang Jue) wrote:
> On 2013/05/24 16:58:29, kalman wrote:
> > third branch here seems unnecessary. we should always have an
extensions/404,
> > you can assert that it's there.
> > 
> > and delete the top-level 404.html. I believe there is also a reference to it
> in
> > the integration test.
> 
> Yes. It's unnecessary. But before the new 404 templates are merged to stable,
> beta, dev, it's not yet always true.
> 
> Another choice is to increase the mid version in app.yaml. It seems that
> revision pinning also works with branches?
> 
> The integration test tries to request 'extensions/notfound.html' and there's
no
> reference to it. (anyway, presubmit passed)

Yes revision pinning works with branches too. Let's do that - remove the third
branch then I can merge this patch through.

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/85001/chrome/common/exte...
chrome/common/extensions/docs/server2/sidenav_data_source.py:78:
self._ConvertToAbsolutePath(sidenav, key)
On 2013/05/25 00:08:39, 方觉(Fang Jue) wrote:
> On 2013/05/24 16:58:29, kalman wrote:
> > it would be more correct to use the path of the request for this, not the
key.
> > As it is, this assumes that code under foo/ will request foo.json which may
> not
> > always be the case.
> > 
> > in fact that might make it unnecessary to pass in the base path here at all.
> 
> path here is something like 'extensions/index.html', which doesn't contain the
> channel. So base path (will be /trunk for example) is necessary.
> 
> Use path here seems to be more correct. Done.

Why is the 'extensions/index.html' example special? I'll comment on the new
code.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/render_servlet.py:94: content =
templates.Render(doc_class + '/404')
'%s/404' % doc_class

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:52: self._base_path
= base_path
perhaps make this all:

self._dir_name, self._file_name = path.rsplit('/', 1)

see below.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:71:
item['fileName'] = '%s/%s/%s' % (self._base_path,
'fileName' is actually wrong, it should have been called 'path' particularly
since they're not all file names (they might be directories, or external URLs).

I suppose it's a bit of make-work to change it though. oh well, you decide.
either way it obviously won't break anything due to the version pinning.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:72:
self._doc_class,
doc class could be empty if there is no branch, which is legitimate. we don't
want to render all sidenav items in 'stable' as '/stable/extensions/foo.html'
because (a) that involves an extra redirect for every sidenav entry and (b) it
looks bad in tooltips and if copy-pasting links.

the logic should be something like:
- if the path is absolute, do nothing
- if the path is relative, prepend it with the "directory" of the requested
path.

I think that |path| is entirely enough to figure this out. basically I think you
need to take out doc_class and replace it with path.rsplit('/', 1)[0].

[方觉(Fang Jue), 2013-05-28 22:53:45]

On 2013/05/28 14:53:37, kalman wrote:
> sorry was a public holiday yesterday.
> 

>
https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
> chrome/common/extensions/docs/server2/sidenav_data_source.py:71:
> item['fileName'] = '%s/%s/%s' % (self._base_path,
> 'fileName' is actually wrong, it should have been called 'path' particularly
> since they're not all file names (they might be directories, or external
URLs).
> 
> I suppose it's a bit of make-work to change it though. oh well, you decide.
> either way it obviously won't break anything due to the version pinning.
> 
What about 'href' (because it can also be a full qualified URL)?

>
https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
> chrome/common/extensions/docs/server2/sidenav_data_source.py:72:
> self._doc_class,
> doc class could be empty if there is no branch, which is legitimate. we don't
> want to render all sidenav items in 'stable' as '/stable/extensions/foo.html'
> because (a) that involves an extra redirect for every sidenav entry and (b) it
> looks bad in tooltips and if copy-pasting links.
> 
> the logic should be something like:
> - if the path is absolute, do nothing
> - if the path is relative, prepend it with the "directory" of the requested
> path.
> 
> I think that |path| is entirely enough to figure this out. basically I think
you
> need to take out doc_class and replace it with path.rsplit('/', 1)[0].
Hey, the path passed to sidenav_data_source doesn't contain the channel at all.

In render_servlet.py, we have:
channel, path = BranchUtility.SplitChannelNameFromPath(path_with_channel)
...
templates = server_instance.template_data_source_factory.Create(self._request,
path)
and in TemplateDataSource.Factory.Create, we have:
self._sidenav_data_source_factory.Create(path)

[方觉(Fang Jue), 2013-05-28 23:37:49]

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/render_servlet.py:94: content =
templates.Render(doc_class + '/404')
On 2013/05/28 14:53:37, kalman wrote:
> '%s/404' % doc_class

Done.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:52: self._base_path
= base_path
On 2013/05/28 14:53:37, kalman wrote:
> perhaps make this all:
> 
> self._dir_name, self._file_name = path.rsplit('/', 1)
> 
> see below.

Suppose we want to add manifest/*.html (with the manifest generation patch) to
sidenav. What path should we use in extensions_sidenav.json?
"manifest/version.html"? If that's the case, this should be
self._doc_class, self._file_name = path.split('/', 1)
If it should be "version.html", the code you suggested is correct.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:71:
item['fileName'] = '%s/%s/%s' % (self._base_path,
On 2013/05/28 14:53:37, kalman wrote:
> 'fileName' is actually wrong, it should have been called 'path' particularly
> since they're not all file names (they might be directories, or external
URLs).
> 
> I suppose it's a bit of make-work to change it though. oh well, you decide.
> either way it obviously won't break anything due to the version pinning.

*sidenav.json, sidenav_data_source and its tests, and private/sidenav* will need
changes I think. But 'fileName' is actually not a good name here. 'path' is not
a good name for external URLs? What about 'href'?

[not at google - send to devlin, 2013-05-28 23:45:27]

lgtm with those last round of comments + rename to href.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:52: self._base_path
= base_path
On 2013/05/28 23:37:50, 方觉(Fang Jue) wrote:
> On 2013/05/28 14:53:37, kalman wrote:
> > perhaps make this all:
> > 
> > self._dir_name, self._file_name = path.rsplit('/', 1)
> > 
> > see below.
> 
> Suppose we want to add manifest/*.html (with the manifest generation patch) to
> sidenav. What path should we use in extensions_sidenav.json?
> "manifest/version.html"? If that's the case, this should be
> self._doc_class, self._file_name = path.split('/', 1)
> If it should be "version.html", the code you suggested is correct.

I see. It should be manifest/version.html, that makes sense. ok.

https://chromiumcodereview.appspot.com/12521030/diff/111001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:71:
item['fileName'] = '%s/%s/%s' % (self._base_path,
On 2013/05/28 23:37:50, 方觉(Fang Jue) wrote:
> On 2013/05/28 14:53:37, kalman wrote:
> > 'fileName' is actually wrong, it should have been called 'path' particularly
> > since they're not all file names (they might be directories, or external
> URLs).
> > 
> > I suppose it's a bit of make-work to change it though. oh well, you decide.
> > either way it obviously won't break anything due to the version pinning.
> 
> *sidenav.json, sidenav_data_source and its tests, and private/sidenav* will
need
> changes I think. But 'fileName' is actually not a good name here. 'path' is
not
> a good name for external URLs? What about 'href'?

href makes sense.

https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
chrome/common/extensions/docs/server2/render_servlet.py:98: content = 'Not
Found.'
we agreed to remove this last check?

https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:52:
self._doc_class, self._file_name = path.split('/', 1)
can we nevertheless call this file_dir and file_name, then include base_path in
the file dir and not store it on self?

such that the code below is posixpath.join('/', self._file_dir, file_name)

[方觉(Fang Jue), 2013-05-28 23:56:55]

>
https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
> chrome/common/extensions/docs/server2/sidenav_data_source.py:52:
> self._doc_class, self._file_name = path.split('/', 1)
> can we nevertheless call this file_dir and file_name, then include base_path
in
> the file dir and not store it on self?
> 
> such that the code below is posixpath.join('/', self._file_dir, file_name)

This looks much better!
But I'll have to run to have breakfast and a class following. I'll do it when
I'm back.

[方觉(Fang Jue), 2013-05-29 05:39:01]

It turned out that we can't simply use path to get doc class. It's passed to
SidenavDataSource when it's created. At that time path can be a page that does
not exist. What do you think if I add a SetPath method and call it in
TemplateDataSource.Render and pass template_name (which is the path being
rendered) to it?

[方觉(Fang Jue), 2013-05-29 14:18:19]

>It turned out that we can't simply use path to get doc class. It's passed to
>SidenavDataSource when it's created. At that time path can be a page that does
>not exist. What do you think if I add a SetPath method and call it in
>TemplateDataSource.Render and pass template_name (which is the path being
>rendered) to it?
I've done it in the latest patchset. It also requires a
_FakeSidenavDataSourceFactory in template_data_source_test. I'll leave it for
you to decide whether it's ok.

>
https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/render_servlet.py (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
> chrome/common/extensions/docs/server2/render_servlet.py:98: content = 'Not
> Found.'
> we agreed to remove this last check?
Done. But I can't assert content here because it will break
instance_servlet_test.

> 
>
https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/125001/chrome/common/ext...
> chrome/common/extensions/docs/server2/sidenav_data_source.py:52:
> self._doc_class, self._file_name = path.split('/', 1)
> can we nevertheless call this file_dir and file_name, then include base_path
in
> the file dir and not store it on self?
> 
> such that the code below is posixpath.join('/', self._file_dir, file_name)
Done.

[not at google - send to devlin, 2013-05-29 17:59:09]

WDYT about that comment? We're done apart from that I think.

https://chromiumcodereview.appspot.com/12521030/diff/147001/chrome/common/ext...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/147001/chrome/common/ext...
chrome/common/extensions/docs/server2/template_data_source.py:113:
self._sidenav_data_source.SetPath(template_name)
I don't understand. the name of the template being renderered is not the same as
the path, it just happens to be in most circumstances.

Your comment about the page not existing is key, though. I commented in the
sidenav data source there. I think the relative-pathness was a red herring and
they should have always been absolute paths - I am really sorry for not catching
that. SidenavDataSource will still need to add the channel name though, and
warns if there's a relative path (and converts it into an absolute path -
redirect logic should fix it, when I submit that patch).

https://chromiumcodereview.appspot.com/12521030/diff/147001/chrome/common/ext...
File chrome/common/extensions/docs/templates/json/apps_sidenav.json (right):

https://chromiumcodereview.appspot.com/12521030/diff/147001/chrome/common/ext...
chrome/common/extensions/docs/templates/json/apps_sidenav.json:7: "href":
"about_apps.html"
Stepping back: the problem may actually be that these need to be qualified paths
after all. E.g. /apps/about_apps.html, /apps/app_architecture.html, etc.
SidenavDataSource still need to prepend the channel name though.

[方觉(Fang Jue), 2013-05-29 23:07:30]

OK. Paths in *_sidenav.json are qualified now. SidenavDataSource only prepends
the channel (or _patch/.../).

[not at google - send to devlin, 2013-05-29 23:16:46]

lgtm, one more change but it's pretty mechanical.

https://chromiumcodereview.appspot.com/12521030/diff/160001/chrome/common/ext...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://chromiumcodereview.appspot.com/12521030/diff/160001/chrome/common/ext...
chrome/common/extensions/docs/server2/sidenav_data_source.py:67: if file_name is
not None and not file_name.startswith(('/',
I think they should *all* start with a '/' and we always prepend the file_dir.

also file_name should be href here (as in href = item.get('href')).

then you can warn if it doesn't start with a '/' and add one.

e.g.

href = item.get('href')
if not href or href.startswith('http://', 'https://'):
  continue
if not href.startswith('/'):
  logging.warn('Sidenav paths must be qualified, %s is not' % href)
  href = '/' + href
item['href'] = '%s%s' % (self._file_dir, href)
if 'items' in item:
  self._ConvertToAbsolutePath(item['items'])

actually _ConvertToAbsolutePath is probably better named _QualifyHrefs or
something.

[方觉(Fang Jue), 2013-05-29 23:40:52]

On 2013/05/29 23:16:46, kalman wrote:
> lgtm, one more change but it's pretty mechanical.
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/160001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):
> 
>
https://chromiumcodereview.appspot.com/12521030/diff/160001/chrome/common/ext...
> chrome/common/extensions/docs/server2/sidenav_data_source.py:67: if file_name
is
> not None and not file_name.startswith(('/',
> I think they should *all* start with a '/' and we always prepend the file_dir.
> 
> also file_name should be href here (as in href = item.get('href')).
> 
> then you can warn if it doesn't start with a '/' and add one.
> 
> e.g.
> 
> href = item.get('href')
> if not href or href.startswith('http://', 'https://'):
>   continue
> if not href.startswith('/'):
>   logging.warn('Sidenav paths must be qualified, %s is not' % href)
>   href = '/' + href
> item['href'] = '%s%s' % (self._file_dir, href)
> if 'items' in item:
>   self._ConvertToAbsolutePath(item['items'])
> 
> actually _ConvertToAbsolutePath is probably better named _QualifyHrefs or
> something.

Done.

[commit-bot: I haz the power, 2013-05-29 23:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/12521030/179003

[commit-bot: I haz the power, 2013-05-30 22:43:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/12521030/179003

[方觉(Fang Jue), 2013-05-31 00:31:15]

Just realized that there will be conflicts when it's merged to stable.
apps_sidenav.json changed after branch 1453.

[not at google - send to devlin, 2013-05-31 16:44:34]

That's ok, it'll be straightforward to merge.

[方觉(Fang Jue), 2013-05-31 17:23:17]

On 2013/05/31 16:44:34, kalman wrote:
> That's ok, it'll be straightforward to merge.

Is it convenient to land this now? You need to merge it to stable/beta/dev
before deploying it to appengine, during which the docs server will stop
updating.

[not at google - send to devlin, 2013-05-31 17:29:07]

Yes it's fine - not a big deal if it stops updating for a while.

[commit-bot: I haz the power, 2013-05-31 17:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fangjue23303@gmail.com/12521030/179003

[commit-bot: I haz the power, 2013-05-31 17:59:14]

Change committed as 203435