Docserver: Modify DocumentRenderer to generate absolute links

Docserver: Modify DocumentRenderer to generate absolute links

BUG=400775
NOTRY=True

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

[not at google - send to devlin, 2014-08-06 23:00:49]

I think this will become a lot simpler without the host?

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer.py:82:
new_document.append('<a href=%s/%s/%s>%s</a>' % (host,
you shouldn't need a host, just /some/link will default to
http[s]://<host>/some/link

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer_test.py:14: def
_ConstructString(*strs):
do you need this? you can have multi-line string in python by just listing them
next to each other. besides that seems fine to inline the ''.join call

[ahernandez, 2014-08-07 00:56:12]

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer.py:82:
new_document.append('<a href=%s/%s/%s>%s</a>' % (host,
On 2014/08/06 23:00:48, kalman wrote:
> you shouldn't need a host, just /some/link will default to
> http[s]://<host>/some/link

This complains about links with '/' at the
front:https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/docs/server2/patch_servlet_test.py&q=patch_servlet_test.py&sq=package:chromium&type=cs&l=34

This check only happens in the test file, but I'm a bit wary of changing the
check, particularly because I'm not sure why it checks for '/' at the beginning
of the link in the first place. There's no documentation explaining why a link
of that form is considered unqualified.

[not at google - send to devlin, 2014-08-07 15:21:18]

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer.py:82:
new_document.append('<a href=%s/%s/%s>%s</a>' % (host,
On 2014/08/07 00:56:12, ahernandez wrote:
> On 2014/08/06 23:00:48, kalman wrote:
> > you shouldn't need a host, just /some/link will default to
> > http[s]://<host>/some/link
> 
> This complains about links with '/' at the
>
front:https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/docs/server2/patch_servlet_test.py&q=patch_servlet_test.py&sq=package:chromium&type=cs&l=34
> 
> This check only happens in the test file, but I'm a bit wary of changing the
> check, particularly because I'm not sure why it checks for '/' at the
beginning
> of the link in the first place. There's no documentation explaining why a link
> of that form is considered unqualified.

yeah, the leading / validity check is to catch bugs caused by inconsistencies.
you're right not to change it - but it should be possible to generate an
absolute link without triggering that? we're just generating HTML at this point,
not valid links.

what I mean is - if you take the |host| argument out of this phrase and just
change it to '<a href=/%s/%s>%s</a>' % (platform, ...) - will that work?

[ahernandez, 2014-08-07 17:21:31]

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer.py:82:
new_document.append('<a href=%s/%s/%s>%s</a>' % (host,
On 2014/08/07 15:21:18, kalman wrote:
> On 2014/08/07 00:56:12, ahernandez wrote:
> > On 2014/08/06 23:00:48, kalman wrote:
> > > you shouldn't need a host, just /some/link will default to
> > > http[s]://<host>/some/link
> > 
> > This complains about links with '/' at the
> >
>
front:https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/docs/server2/patch_servlet_test.py&q=patch_servlet_test.py&sq=package:chromium&type=cs&l=34
> > 
> > This check only happens in the test file, but I'm a bit wary of changing the
> > check, particularly because I'm not sure why it checks for '/' at the
> beginning
> > of the link in the first place. There's no documentation explaining why a
link
> > of that form is considered unqualified.
> 
> yeah, the leading / validity check is to catch bugs caused by inconsistencies.
> you're right not to change it - but it should be possible to generate an
> absolute link without triggering that? we're just generating HTML at this
point,
> not valid links.
> 
> what I mean is - if you take the |host| argument out of this phrase and just
> change it to '<a href=/%s/%s>%s</a>' % (platform, ...) - will that work?

Changing it to '<a href=/%s/%s>%s</a>' is what I'm talking about: that produces
hrefs with a leading '/'.

[not at google - send to devlin, 2014-08-07 17:37:09]

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/441053004/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer.py:82:
new_document.append('<a href=%s/%s/%s>%s</a>' % (host,
On 2014/08/07 17:21:31, ahernandez wrote:
> On 2014/08/07 15:21:18, kalman wrote:
> > On 2014/08/07 00:56:12, ahernandez wrote:
> > > On 2014/08/06 23:00:48, kalman wrote:
> > > > you shouldn't need a host, just /some/link will default to
> > > > http[s]://<host>/some/link
> > > 
> > > This complains about links with '/' at the
> > >
> >
>
front:https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/docs/server2/patch_servlet_test.py&q=patch_servlet_test.py&sq=package:chromium&type=cs&l=34
> > > 
> > > This check only happens in the test file, but I'm a bit wary of changing
the
> > > check, particularly because I'm not sure why it checks for '/' at the
> > beginning
> > > of the link in the first place. There's no documentation explaining why a
> link
> > > of that form is considered unqualified.
> > 
> > yeah, the leading / validity check is to catch bugs caused by
inconsistencies.
> > you're right not to change it - but it should be possible to generate an
> > absolute link without triggering that? we're just generating HTML at this
> point,
> > not valid links.
> > 
> > what I mean is - if you take the |host| argument out of this phrase and just
> > change it to '<a href=/%s/%s>%s</a>' % (platform, ...) - will that work?
> 
> Changing it to '<a href=/%s/%s>%s</a>' is what I'm talking about: that
produces
> hrefs with a leading '/'.

such links are valid, at least on the web. I should have read your comment first
- that the patch servlet test is failing. i think you should fix the patch
servlet test to not crash :)

[ahernandez, 2014-08-07 23:24:57]

Apologies for the delay, I was waiting on another patch to go through so I could
upload these changes. PTAL.

[not at google - send to devlin, 2014-08-07 23:59:31]

this still doesn't quite work - go to
http://localhost:8000/apps/manifest/sockets and then click on sockets.tcp. It
should link to http://localhost:8000/apps/sockets_tcp but it links to itself.

https://codereview.chromium.org/441053004/diff/140001/chrome/common/extension...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/441053004/diff/140001/chrome/common/extension...
chrome/common/extensions/docs/server2/document_renderer.py:83:
self._platform_bundle._base_path + platform,
nit - but perhaps use %s%s and split up base_path/platform rather than %s and
adding them together? the mix just looks a little odd.

[ahernandez, 2014-08-08 00:04:46]

On 2014/08/07 23:59:31, kalman wrote:
> this still doesn't quite work - go to
> http://localhost:8000/apps/manifest/sockets and then click on sockets.tcp. It
> should link to http://localhost:8000/apps/sockets_tcp but it links to itself.
> 
>
https://codereview.chromium.org/441053004/diff/140001/chrome/common/extension...
> File chrome/common/extensions/docs/server2/document_renderer.py (right):
> 
>
https://codereview.chromium.org/441053004/diff/140001/chrome/common/extension...
> chrome/common/extensions/docs/server2/document_renderer.py:83:
> self._platform_bundle._base_path + platform,
> nit - but perhaps use %s%s and split up base_path/platform rather than %s and
> adding them together? the mix just looks a little odd.

The links are working properly on my local preview.py, so I'm not sure what the
discrepancy is from. I'm running on Ubuntu.

[not at google - send to devlin, 2014-08-08 00:10:54]

I'm very silly, and was running against an unpatched version of the server.
lgtm.

[ahernandez, 2014-08-08 00:19:16]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-08-08 00:54:48]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/44105300...

[commit-bot: I haz the power, 2014-08-08 09:54:00]

Change committed as 288290