[DocServer] Fixed Sample Links

[DocServer] Fixed Sample Links

BUG=295502
NOTRY=true

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

[François Beaufort, 2013-09-20 09:39:50]

Hello Vincent,

You'll be able to test this CL by going to
https://chrome-apps-doc.appspot.com/_patch/23526077/extensions/index.html and
click on the new Google Mail Checker extension link.
It should redirect you to the samples pages and show only the Google Mail
Checker extension.

[scheib, 2013-09-20 18:26:43]

I like the readability of the links -- but kalman is more suitable to review
changes in doc server behavior.

[not at google - send to devlin, 2013-09-20 18:31:22]

I like the feature too, however,
- this should be done server-side (see samples_data_source I think for where
these IDs get generated)
- don't have spaces in URLs; make it #google-mail-checker, etc.

When you do this, you can check for broken links by running
"integration_tests.py -a" and grepping for samples.html. There are lots of
broken links already, but you should be able to find any new ones.

[François Beaufort, 2013-09-23 10:32:50]

IDs are now generated by the samples_data_source.py python script. 
There are no spaces in friendly URLs anymore.

And thanks to the integration_test script, I found some issues introduced by my
CL that I've fixed. Thank you!

On 2013/09/20 18:31:22, kalman wrote:
> I like the feature too, however,
> - this should be done server-side (see samples_data_source I think for where
> these IDs get generated)
> - don't have spaces in URLs; make it #google-mail-checker, etc.
> 
> When you do this, you can check for broken links by running
> "integration_tests.py -a" and grepping for samples.html. There are lots of
> broken links already, but you should be able to find any new ones.

[not at google - send to devlin, 2013-09-23 15:59:50]

lgtm without the JS code.

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/samples_data_source.py:234:
logging.error(e)
incremental cleanup:

except:
  logging.error(traceback.format_exc())
  ...

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/docs/static/js/samples.js (right):

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
chrome/common/extensions/docs/static/js/samples.js:40:
window.addEventListener('DOMContentLoaded', onLoad);
the onLoad is hopefully unnecessary since the relevant DOM will have loaded by
the time this script executes.

however, I don't think we should hide the samples this way anyway: I don't like
executing JS on the client (keep it to an absolute minimum) and if we're loading
all this content anyway we might as well just show it. anchor is fine. If you
want to only show a single sample (and that sounds good to me actually) then we
should make it part of the actual URL in some way, so that the server only need
to render that particular sample.

could you file a but on that? there are a couple of different ways to do it and
a quick look on a bug would resolve it rather than doing it here.

[François Beaufort, 2013-09-24 09:24:49]

I've fixed the Python exception code and removed the JS OnLoad code.

The issue I have now is that when I go to
https://chrome-apps-doc.appspot.com/_patch/23526077/extensions/samples.html#d...
from
https://chrome-apps-doc.appspot.com/_patch/23526077/extensions/devtools.html, it
doesn't show up the samples related to devtools (containing devtools tags or
having devtools in their name).

The looseness of this hashtag was that you could either link to a specific
sample (eg. #google-mail-checker) or either to a range of samples (eg.
#devtools).

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/samples_data_source.py:234:
logging.error(e)
On 2013/09/23 15:59:51, kalman wrote:
> incremental cleanup:
> 
> except:
>   logging.error(traceback.format_exc())
>   ...

Done.

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/docs/static/js/samples.js (right):

https://codereview.chromium.org/23526077/diff/11001/chrome/common/extensions/...
chrome/common/extensions/docs/static/js/samples.js:40:
window.addEventListener('DOMContentLoaded', onLoad);
On 2013/09/23 15:59:51, kalman wrote:
> the onLoad is hopefully unnecessary since the relevant DOM will have loaded by
> the time this script executes.
> 
> however, I don't think we should hide the samples this way anyway: I don't
like
> executing JS on the client (keep it to an absolute minimum) and if we're
loading
> all this content anyway we might as well just show it. anchor is fine. If you
> want to only show a single sample (and that sounds good to me actually) then
we
> should make it part of the actual URL in some way, so that the server only
need
> to render that particular sample.
> 
> could you file a but on that? there are a couple of different ways to do it
and
> a quick look on a bug would resolve it rather than doing it here.

Done.

[not at google - send to devlin, 2013-09-24 18:16:29]

lgtm

I don't mind losing the #devtools thing. We already have a mechanism for pulling
in sample for a specific API; see api_reference.html down the bottom; it just
doesn't seem to work for devtools. We can fix that later.

https://codereview.chromium.org/23526077/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/static/js/samples.js (right):

https://codereview.chromium.org/23526077/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/static/js/samples.js:13: var sample_title =
samples[i].getElementsByTagName('h2')[0].innerText;
nice, I like it. nits:
- s/sample_title/sampleTitle/ or just |title|.
  - oops all of those other variable names should be camel case as well... hm.
- use |sample| not |samples[i]|.
- use textContent not innerText.

[François Beaufort, 2013-09-25 10:10:30]

I've changed all variables names to camel case to be consistent and addressed
all your feedback

https://codereview.chromium.org/23526077/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/docs/static/js/samples.js (right):

https://codereview.chromium.org/23526077/diff/25001/chrome/common/extensions/...
chrome/common/extensions/docs/static/js/samples.js:13: var sample_title =
samples[i].getElementsByTagName('h2')[0].innerText;
On 2013/09/24 18:16:30, kalman wrote:
> nice, I like it. nits:
> - s/sample_title/sampleTitle/ or just |title|.
>   - oops all of those other variable names should be camel case as well... hm.
> - use |sample| not |samples[i]|.
> - use textContent not innerText.

Done.

[not at google - send to devlin, 2013-09-25 17:33:08]

lgtm

https://codereview.chromium.org/23526077/diff/37001/chrome/common/extensions/...
File chrome/common/extensions/docs/static/js/samples.js (right):

https://codereview.chromium.org/23526077/diff/37001/chrome/common/extensions/...
chrome/common/extensions/docs/static/js/samples.js:13: var sampleTitle =
sample.getElementsByTagName('h2')[0].textContent;
I sure hope that sample.getElementsByTagName('h2') is non-empty.

[François Beaufort, 2013-09-26 10:27:28]

https://codereview.chromium.org/23526077/diff/37001/chrome/common/extensions/...
File chrome/common/extensions/docs/static/js/samples.js (right):

https://codereview.chromium.org/23526077/diff/37001/chrome/common/extensions/...
chrome/common/extensions/docs/static/js/samples.js:13: var sampleTitle =
sample.getElementsByTagName('h2')[0].textContent;
You're right. I should check it's not empty.

On 2013/09/25 17:33:08, kalman wrote:
> I sure hope that sample.getElementsByTagName('h2') is non-empty.

[commit-bot: I haz the power, 2013-09-27 08:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/23526077/4...

[commit-bot: I haz the power, 2013-09-27 08:48: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
aec513807d090b8d9c058c060b39635cad329272..bc42d42ee8e1ac15525f76d3179bff8120aefffc
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-28-2
+version: 2-28-3
 runtime: python27
 api_version: 1
 threadsafe: false

[François Beaufort, 2013-09-27 09:00:44]

I bumped Server's version.

[not at google - send to devlin, 2013-09-27 16:49:11]

lgtm, go for it

[commit-bot: I haz the power, 2013-09-27 19:08:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/23526077/5...

[commit-bot: I haz the power, 2013-09-27 19:26:06]

Change committed as 225757