Clean patch with DCC static content

Clean patch with DCC static content

NOTRY=true

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

[not at google - send to devlin, 2013-12-13 21:20:14]

Before I review: could you bump the versions of app.yaml and cron.yaml in the
server2 directory? That will make the server stop updating so that this new
content won't go live.

... and now, I look.

[mkearney, 2013-12-13 21:35:27]

Hey, Ben

Renato is best person to bump the versions (I don't want to upload, as
binary files keep given me trouble).

Renato is on the shuttle and will be home in 30 minutes to make the push.
So you should be good to review.

I'm going to test this now too. We may have some merge stuff going on with
the table of contents fix you put through. And that css might still need to
go in.

Testing now.

Me


On Fri, Dec 13, 2013 at 1:20 PM, <kalman@chromium.org> wrote:

> Before I review: could you bump the versions of app.yaml and cron.yaml in
> the
> server2 directory? That will make the server stop updating so that this new
> content won't go live.
>
> ... and now, I look.
>
> https://codereview.chromium.org/102593005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkearney1, 2013-12-13 22:23:30]

** Table of contents for reference pages is broken (extensions/storage.html is a
good one to look at).

** Need to map Ben's table of contents fix to the changes made in this cl.
Here's Ben's cl: https://codereview.chromium.org/99703004/

** Check css changes in Ben's cl and make sure these are in this cl.

Other than that-- this is a good first go to deliver.

[mkearney1, 2013-12-13 22:31:28]

lgtm... Renato brings up a good point. We are better to land this cl and then
fix the TOC rendering for reference, rather than adding more fixes to this
already mega cl. Let's get this through and then iterate.

[mkearney1, 2013-12-13 22:33:14]

I'm the owner of this cl, so I can't lgtm :)

[Renato Mangini (chromium), 2013-12-13 22:41:42]

lgtm

[not at google - send to devlin, 2013-12-13 23:03:46]

lgtm with comments + that caveat. I didn't really review sidenav_data_source
that closely given I hope that code will be backed out.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:119:
_AddSelected(sidenav, self._server_instance.base_path + self._request.path)
The change I'd rather see to this file is that the _AddSelected becomes
_AddAnnotations, in which we both annotate the selected and the related items,
then it's up to the templates to render that correctly.

That would also make testing this easier.

If you really want to land this CL now then I don't mind you landing it, but,
please make that change asap.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/table_of_contents_renderer.py
(right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/table_of_contents_renderer.py:39:
'hideInTOC':   entry.attributes.get('hideintoc', None)!=None,
this CL is pretty large and I can't patch it in, so I can't tell when this is
used, but you should be able to specify title="" rather than hideintoc.

I.e. I specifically added title="" for hiding it in the TOC.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/templates/json/content_providers.json
(right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/templates/json/content_providers.json:50:
"supportsZip": true
why does this support zip?

[not at google - send to devlin, 2013-12-13 23:05:12]

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:107: for nav_type
in ('top', 'related')]
this should be just 'chrome' not ('top', 'related').

[not at google - send to devlin, 2013-12-13 23:05:47]

btw I didn't look at the actual content in the slightest, leaving that to
everybody else.

[not at google - send to devlin, 2013-12-13 23:07:23]

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-45-0
when you sync you'll find this has reached 2-46-0, so make sure you bump it to
2-47-0.

Actually is this change the big new look change? Perhaps it's time to bump the
server to 3-0-0.

[not at google - send to devlin, 2013-12-14 02:41:04]

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:115:
self._server_instance.base_path + self._request.path)
Also: it looks like _ProcessRelated can return None here, which will crash.

[Renato Mangini (chromium), 2013-12-17 20:32:32]

addressed kalman's comments

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-45-0
On 2013/12/13 23:07:24, kalman wrote:
> when you sync you'll find this has reached 2-46-0, so make sure you bump it to
> 2-47-0.
> 
> Actually is this change the big new look change? Perhaps it's time to bump the
> server to 3-0-0.

Done. Yep, 3.0.0 sounds better.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:107: for nav_type
in ('top', 'related')]
On 2013/12/13 23:05:12, kalman wrote:
> this should be just 'chrome' not ('top', 'related').

Done.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:115:
self._server_instance.base_path + self._request.path)
On 2013/12/14 02:41:05, kalman wrote:
> Also: it looks like _ProcessRelated can return None here, which will crash.

Done.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:119:
_AddSelected(sidenav, self._server_instance.base_path + self._request.path)
On 2013/12/13 23:03:47, kalman wrote:
> The change I'd rather see to this file is that the _AddSelected becomes
> _AddAnnotations, in which we both annotate the selected and the related items,
> then it's up to the templates to render that correctly.
> 
> That would also make testing this easier.
> 
> If you really want to land this CL now then I don't mind you landing it, but,
> please make that change asap.

Done, although the template is more complicated and slower now. Although the
related pages is a flat list, now the template has to navigate through the tree
to get the parent title and the related pages. But I agree that the server code
is simpler and more elegant.

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/server2/table_of_contents_renderer.py
(right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/server2/table_of_contents_renderer.py:39:
'hideInTOC':   entry.attributes.get('hideintoc', None)!=None,
On 2013/12/13 23:03:47, kalman wrote:
> this CL is pretty large and I can't patch it in, so I can't tell when this is
> used, but you should be able to specify title="" rather than hideintoc.
> 
> I.e. I specifically added title="" for hiding it in the TOC.

Done. I wasn't aware of the title="" trick

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
File chrome/common/extensions/docs/templates/json/content_providers.json
(right):

https://codereview.chromium.org/102593005/diff/350001/chrome/common/extension...
chrome/common/extensions/docs/templates/json/content_providers.json:50:
"supportsZip": true
On 2013/12/13 23:03:47, kalman wrote:
> why does this support zip?

Done.

[not at google - send to devlin, 2013-12-18 05:10:13]

lgtm with some very minor nits, though could you also update
sidenav_data_source_test.py with those new "related" attributes?

I see your point about efficiency now... I think in practice it doesn't make
much of a difference since we're looping over the TOC anyway, and this is a
pretty small amount of work, but it's still not great. However, the cleaner code
in the Python is a bigger win.

Side note: hopefully I will add profiling to the templates so we can actually
figure out what rendering operations take the most time. That will settle it.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:23: 
need an extra \n here

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:35: # depth first
search
Comment is ok to leave out I think.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:46: # navigate on
siblings, marking them as related
the docstring on this function adequately covers this; no comment needed

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:55: 
need 1 less \n here

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:97: return future
inline this (no need for the extra |future| variable, return directly)

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:99: def get(self,
key):
add:

# TODO(mangini/kalman): Use |key| to decide which sidenav to use, which will
require a more complex Cron method.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:104:
self._server_instance.base_path + self._request.path)
indent -= 3 to align with the (

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:105: return sidenav
no trailing space

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/templates/private/article.html (right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/article.html:13: {{+partials.cc
/}}
should the license be at the bottom of the page? (even though the TOC is
floated, so in practice makes no difference, this looks a bit odd)

I would also do away with the blank lines.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/templates/private/relatedpages_item.html
(right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/relatedpages_item.html:1:
{{#r:items}}{{?r.related}}
put the {{?r.related}} on the next line; apologies for not documenting this
anywhere, but the template rendering will look better (though again it makes no
practical difference given this is all HTML anyway)

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/relatedpages_item.html:6:
{{?r.items}}{{+partials.relatedpages_item items:r.items /}}{{/r.items}}
you should be able to do

{{?r.items +partials.relatedpages_item items:r.items /}}

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/templates/private/relatedpages_title.html
(right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/relatedpages_title.html:5:
{{?t.items}}{{+partials.relatedpages_title items:t.items /}}{{/t.items}}
same comment as before; {{?t.items +partials.relatedpages_title items:t.items
/}}

[Renato Mangini (chromium), 2013-12-18 13:25:29]

addressed all comments, except the one re license footer (see inline comments
for more)

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:23: 
On 2013/12/18 05:10:14, kalman wrote:
> need an extra \n here

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:35: # depth first
search
On 2013/12/18 05:10:14, kalman wrote:
> Comment is ok to leave out I think.

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:46: # navigate on
siblings, marking them as related
On 2013/12/18 05:10:14, kalman wrote:
> the docstring on this function adequately covers this; no comment needed

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:55: 
On 2013/12/18 05:10:14, kalman wrote:
> need 1 less \n here

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:97: return future
On 2013/12/18 05:10:14, kalman wrote:
> inline this (no need for the extra |future| variable, return directly)

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:99: def get(self,
key):
On 2013/12/18 05:10:14, kalman wrote:
> add:
> 
> # TODO(mangini/kalman): Use |key| to decide which sidenav to use, which will
> require a more complex Cron method.

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:104:
self._server_instance.base_path + self._request.path)
On 2013/12/18 05:10:14, kalman wrote:
> indent -= 3 to align with the (

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/server2/sidenav_data_source.py:105: return sidenav
On 2013/12/18 05:10:14, kalman wrote:
> no trailing space

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/templates/private/article.html (right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/article.html:13: {{+partials.cc
/}}
On 2013/12/18 05:10:14, kalman wrote:
> should the license be at the bottom of the page? (even though the TOC is
> floated, so in practice makes no difference, this looks a bit odd)

TOC does not float on narrow devices, and changing license to the bottom
effectively moves it to below TOC. I prefer the way it is (TOC always the last
element in page, when not floating), and since this is what came from designers,
I'd rather not change it.

> 
> I would also do away with the blank lines.

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/templates/private/relatedpages_item.html
(right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/relatedpages_item.html:1:
{{#r:items}}{{?r.related}}
On 2013/12/18 05:10:14, kalman wrote:
> put the {{?r.related}} on the next line; apologies for not documenting this
> anywhere, but the template rendering will look better (though again it makes
no
> practical difference given this is all HTML anyway)

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/relatedpages_item.html:6:
{{?r.items}}{{+partials.relatedpages_item items:r.items /}}{{/r.items}}
On 2013/12/18 05:10:14, kalman wrote:
> you should be able to do
> 
> {{?r.items +partials.relatedpages_item items:r.items /}}

Done.

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
File chrome/common/extensions/docs/templates/private/relatedpages_title.html
(right):

https://codereview.chromium.org/102593005/diff/370001/chrome/common/extension...
chrome/common/extensions/docs/templates/private/relatedpages_title.html:5:
{{?t.items}}{{+partials.relatedpages_title items:t.items /}}{{/t.items}}
On 2013/12/18 05:10:14, kalman wrote:
> same comment as before; {{?t.items +partials.relatedpages_title items:t.items
> /}}

Done.

[Renato Mangini (chromium), 2013-12-18 14:24:36]

lgtm


... and fixed the tests for sidenav_data_source. Commiting now.

[commit-bot: I haz the power, 2013-12-18 14:24:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkearney@chromium.org/102593005/410001

[commit-bot: I haz the power, 2013-12-18 15:38:29]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-18 18:20:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkearney@chromium.org/102593005/410001

[commit-bot: I haz the power, 2013-12-18 19:46:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-18 22:04:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkearney@chromium.org/102593005/410001

[commit-bot: I haz the power, 2013-12-18 22:09:34]

Change committed as 241673