Correct $ref links in extensions documentation server relative location.

Correct $ref links in extensions documentation server relative location.

The links are now prefixed as needed with enough '../'s as appropriate.
This fixes $ref links from manifest articles.

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

[benwells, 2013-11-21 05:03:23]

This doesn't fix the partial things coming from manifest.json, I'm not sure how
to fix those.

If this is OK and lands I will update all the manifest articles to use $ref as
the links are a bit of a mess there at the moment.

Note two tests failed in the presubmit but I am thinking maybe they were already
broken (content_provider_test.py and render_servlet_test.py).

[not at google - send to devlin, 2013-11-21 05:26:49]

Being extra hard on you, sorry.

Yeah the paths in manifest.json (etc) are trickier and we can worry about that
later. It will probably require doing the $ref substitution post-render rather
than pre-render, something I'm working on for a different reason.

Sorry if the presubmits are failing, that's probably my fault? So those tests
are failing on a clean tree?

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:57: return prefix
could be written as

'../' * (path.count('/') - 1)

and for that matter perhaps that's better just inlined with a comment, since a
comment in context of where this method is called from would be easier to
explain, I think.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:184: def
ResolveAllLinks(self, text, request, namespace=None):
Rather than introduce a whole Request as a (conceptual) dependency, just pass in
the path, and make it explicitly named/optional.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:227: 'rest': rest })
we should kill this confusing %(prefix)s stuff and just use %s with an array.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (right):

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:173: None,
if the request/path/prefix whatever is optional then you don't need to add None
everywhere

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:176: # If a
request is provided it should construct an approapriate relative link.
appropriate

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:179: request =
FakeRequest()
use
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
though if you only pass in the path or prefix it's unnecessary anyway

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:184: '<a
href="../../../bar.bon.html#property-bar_bon_p3">bar_bon_p3</a>',
keep < 80 chars

[not at google - send to devlin, 2013-11-21 15:36:44]

Btw content_provider_test and render_servlet_test both pass for me on my current
checkout (r236527).

[benwells, 2013-11-22 05:44:01]

The failures are occurring for me on Windows without my changes, but not on
Linux.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:57: return prefix
On 2013/11/21 05:26:50, kalman wrote:
> could be written as
> 
> '../' * (path.count('/') - 1)
> 
> and for that matter perhaps that's better just inlined with a comment, since a
> comment in context of where this method is called from would be easier to
> explain, I think.

holy cow, I can multiple strings with numbers !?!?!

Done

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:184: def
ResolveAllLinks(self, text, request, namespace=None):
On 2013/11/21 05:26:50, kalman wrote:
> Rather than introduce a whole Request as a (conceptual) dependency, just pass
in
> the path, and make it explicitly named/optional.

Done.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver.py:227: 'rest': rest })
On 2013/11/21 05:26:50, kalman wrote:
> we should kill this confusing %(prefix)s stuff and just use %s with an array.

Done.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (right):

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:173: None,
On 2013/11/21 05:26:50, kalman wrote:
> if the request/path/prefix whatever is optional then you don't need to add
None
> everywhere

Done.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:176: # If a
request is provided it should construct an approapriate relative link.
On 2013/11/21 05:26:50, kalman wrote:
> appropriate

Done.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:179: request =
FakeRequest()
On 2013/11/21 05:26:50, kalman wrote:
> use
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> though if you only pass in the path or prefix it's unnecessary anyway

Done.

https://codereview.chromium.org/80183002/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/reference_resolver_test.py:184: '<a
href="../../../bar.bon.html#property-bar_bon_p3">bar_bon_p3</a>',
On 2013/11/21 05:26:50, kalman wrote:
> keep < 80 chars

Done.

[not at google - send to devlin, 2013-11-22 20:16:09]

lgtm but a few more nits.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:99: request_path =
self._request.path
nit: ternary:

request_path = self._request.path if self._request or None

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:101: intro,
requestPath=request_path, namespace=api_name)
python style requires request_path not requestPath

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver.py (left):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:47: 
python style is actually to have 2 blank lines between top-level declarations.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:172: def
ResolveAllLinks(self, text, requestPath='', namespace=None):
requestPath -> request_path

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:177: inspecting
request.path.
Perhaps just "Links will be generated relative to |request_path|".

For that matter, perhaps "request_path" is too specific; the abstract concept
here is "relative_to", so perhaps that?

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (left):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver_test.py:15: 
ditto blank line

[not at google - send to devlin, 2013-11-22 20:16:23]

oh and you also need to bump the app.yaml and cron.yaml versions.

[benwells, 2013-11-25 01:12:36]

Would this be a major yaml bump or a minor yaml bump?

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:99: request_path =
self._request.path
On 2013/11/22 20:16:10, kalman wrote:
> nit: ternary:
> 
> request_path = self._request.path if self._request or None

I was thinking it might be better to make sure that self._request is never None.
This change was just needed for tests which leave it blank.

I'll take a look into that.

[benwells, 2013-11-27 07:28:31]

Ben - please check I've made these changes properly ...

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:99: request_path =
self._request.path
On 2013/11/25 01:12:37, benwells wrote:
> On 2013/11/22 20:16:10, kalman wrote:
> > nit: ternary:
> > 
> > request_path = self._request.path if self._request or None
> 
> I was thinking it might be better to make sure that self._request is never
None.
> This change was just needed for tests which leave it blank.
> 
> I'll take a look into that.

Removed the code by making sure self._request was not None in tests.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:101: intro,
requestPath=request_path, namespace=api_name)
On 2013/11/22 20:16:10, kalman wrote:
> python style requires request_path not requestPath

Done.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver.py (left):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:47: 
On 2013/11/22 20:16:10, kalman wrote:
> python style is actually to have 2 blank lines between top-level declarations.

Done.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:172: def
ResolveAllLinks(self, text, requestPath='', namespace=None):
On 2013/11/22 20:16:10, kalman wrote:
> requestPath -> request_path

Done.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:177: inspecting
request.path.
On 2013/11/22 20:16:10, kalman wrote:
> Perhaps just "Links will be generated relative to |request_path|".
> 
> For that matter, perhaps "request_path" is too specific; the abstract concept
> here is "relative_to", so perhaps that?

Done.

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (left):

https://codereview.chromium.org/80183002/diff/70001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver_test.py:15: 
On 2013/11/22 20:16:10, kalman wrote:
> ditto blank line

Done.

[not at google - send to devlin, 2013-11-27 21:46:08]

lgtm with a couple more things.

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
chrome/common/extensions/docs/server2/reference_resolver.py:186: if relative_to
is not '':
"is not" is for object identity. I don't know whether that works for ''. It
would be !=. In any case this branch isn't quite right? My logic ... not quite
right either? If relative_to is "foo" then .count('/') is 0 so you'll be
multiplying '../' by -1.

... I leave it to you to fix it, but I would imagine that there wouldn't be a
need to do any actual string comparison for |relative_to|.

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (right):

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
chrome/common/extensions/docs/server2/reference_resolver_test.py:181:
namespace='bar.bon'))
you might also want to test relative_to of '', 'foo', and 'foo/' in some way.

[benwells, 2013-12-02 06:32:32]

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
chrome/common/extensions/docs/server2/reference_resolver.py:186: if relative_to
is not '':
On 2013/11/27 21:46:09, kalman wrote:
> "is not" is for object identity. I don't know whether that works for ''. It
> would be !=. In any case this branch isn't quite right? My logic ... not quite
> right either? If relative_to is "foo" then .count('/') is 0 so you'll be
> multiplying '../' by -1.
> 
> ... I leave it to you to fix it, but I would imagine that there wouldn't be a
> need to do any actual string comparison for |relative_to|.

Nice catch. Turns out multiplying a sequence by a negative number is equivalent
to multiplying by 0 (see note 2 under the table at
http://docs.python.org/2/library/stdtypes.html#typesseq). So we're all good here
by just removing the broken if statement.

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (right):

https://codereview.chromium.org/80183002/diff/180001/chrome/common/extensions...
chrome/common/extensions/docs/server2/reference_resolver_test.py:181:
namespace='bar.bon'))
On 2013/11/27 21:46:09, kalman wrote:
> you might also want to test relative_to of '', 'foo', and 'foo/' in some way.

Done.

[commit-bot: I haz the power, 2013-12-02 06:33:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/80183002/210001

[commit-bot: I haz the power, 2013-12-02 06:33:11]

Failed to apply patch for chrome/common/extensions/docs/server2/app.yaml:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/docs/server2/app.yaml
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/docs/server2/app.yaml.rej

Patch:       chrome/common/extensions/docs/server2/app.yaml
Index: chrome/common/extensions/docs/server2/app.yaml
diff --git a/chrome/common/extensions/docs/server2/app.yaml
b/chrome/common/extensions/docs/server2/app.yaml
index
3eebce785081db4c3a2aa72d730d0c52a48ab818..69bbee3b3ad706e7906c80e918abe26dcd712d15
100644
--- a/chrome/common/extensions/docs/server2/app.yaml
+++ b/chrome/common/extensions/docs/server2/app.yaml
@@ -1,5 +1,5 @@
 application: chrome-apps-doc
-version: 2-42-7
+version: 2-43-0
 runtime: python27
 api_version: 1
 threadsafe: false

[commit-bot: I haz the power, 2013-12-03 03:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/80183002/230001

[commit-bot: I haz the power, 2013-12-03 10:37:50]

Change committed as 238337