Rework API reference pages

CLOSING THIS ISSUE without committing, because its Base URL needs to change.

The code has moved to
https://codereview.chromium.org/149673004

and review comments will be addressed there as well.

BUG=340702

[Renato Mangini (chromium), 2014-01-03 13:52:52]

Ready for review. There are minor visual issues, but I want to commit this asap
so people can test and identify them.

[not at google - send to devlin, 2014-01-03 22:04:40]

this CL is a pretty drastic way to solve that BUG :) what I'm saying is, is
there a more generic bug that could describe this? perhaps you could file one
and mark it as a blocker for jeffrey's bug.

[not at google - send to devlin, 2014-01-03 23:30:23]

some nits and other fairly superficial comments. I can't seem to patch in this
change to check it out though:

error: patch failed: chrome/common/extensions/docs/static/css/site.css:1
error: repository lacks the necessary blob to fall back on 3-way merge.
error: chrome/common/extensions/docs/static/css/site.css: patch does not apply
error: chrome/common/extensions/docs/static/sass/site.scss: does not exist in
index
Failed to apply the patch

does it rely on one of your other patches?

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:299: 'is_callback':
True,
you will need to bump the app version due to this change (the 3.0.x -> 3.1.0)

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/static/sass/_api.scss (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/static/sass/_api.scss:6: .apisummary {
nit: style I usually see in chromium is for class names to be separated with a
-, like api-summary, api-reference, inner-table, etc.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/static/sass/_api.scss:7: td, th {
nit: likewise chromium style usually to separate rules on different lines,
though when they're really simple rules it probably doesn't matter so much.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/static/sass/_api.scss:56: 
nit: can the use of blank lines in this file be consistent? I don't really mind
what it is, but it could be
- no blank lines here
- blank line between every {..} block unless they're related (e.g. td, th {...}
th {...})

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/api_property.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/api_property.html:10: <table
class="innerTable">
what is "innerTable" supposed to be? I see it used a bunch but it's not clear
from context why it should be "inner". is there a semantically meaningful name
it could be?

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/api_summary.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/api_summary.html:1: <section
id="toc">
unless you hide the TOC for the main page (which... you're not right, since
there's also potentially an intro on these pages) this means two elements will
have the same ID. why does this need to have "toc" id?

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/callback.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/callback.html:9: <br/>
prefer to separate blocks via block elements rather than <br> if possible. the
<code> block seems semantically separate from the text above.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/function.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/function.html:2:
{{?parentName}}<tr><td>{{/}}
why do you need this? can you do this from where this template is included
instead?

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/function_signature.html
(right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/function_signature.html:5:
|no_link|: if different of null, no link will be shown for types
no_link -> disableLink

also: "if not false, ..."

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/parameter_full.html
(right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/parameter_full.html:16:
innerTable:true
I can't find innerTable used anywhere

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/property.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/property.html:22:
innerTable:true
likewise innerTable

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/ref_link.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/ref_link.html:1: {{^no_link}}<a
href="{{link.href}}">{{/}}
given that no_link is only passed in from one place (variable_type.html) you
could do there

{{?disableLink}}
  {{link.text}}
{{:disableLink}}
  {{+partials.ref_link link:link/}}
{{/disableLink}}

and not need to pass it in here; since passing "no link" into a file called "...
link" looks a bit weird.

[not at google - send to devlin, 2014-01-03 23:31:43]

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/function_signature.html
(right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/function_signature.html:5:
|no_link|: if different of null, no link will be shown for types
On 2014/01/03 23:30:24, kalman wrote:
> no_link -> disableLink
> 
> also: "if not false, ..."

ah i see that it's actually a matter of null vs not-null. in that case, "if not
null"...

[Renato Mangini (chromium), 2014-02-05 01:42:46]

On 2014/01/03 22:04:40, kalman wrote:
> this CL is a pretty drastic way to solve that BUG :) what I'm saying is, is
> there a more generic bug that could describe this? perhaps you could file one
> and mark it as a blocker for jeffrey's bug.

Done, changed referred bug to a more generic one.

[Renato Mangini (chromium), 2014-02-06 03:19:17]

Addressed all comments. The actual commit with the changed CL was moved to a
different CL (https://codereview.chromium.org/149673004/) because this CL had a
bad Base URL.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:299: 'is_callback':
True,
On 2014/01/03 23:30:24, kalman - OOO wrote:
> you will need to bump the app version due to this change (the 3.0.x -> 3.1.0)

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/static/sass/_api.scss (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/static/sass/_api.scss:6: .apisummary {
On 2014/01/03 23:30:24, kalman - OOO wrote:
> nit: style I usually see in chromium is for class names to be separated with a
> -, like api-summary, api-reference, inner-table, etc.

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/static/sass/_api.scss:7: td, th {
On 2014/01/03 23:30:24, kalman - OOO wrote:
> nit: likewise chromium style usually to separate rules on different lines,
> though when they're really simple rules it probably doesn't matter so much.

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/static/sass/_api.scss:56: 
On 2014/01/03 23:30:24, kalman - OOO wrote:
> nit: can the use of blank lines in this file be consistent? I don't really
mind
> what it is, but it could be
> - no blank lines here
> - blank line between every {..} block unless they're related (e.g. td, th
{...}
> th {...})

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/api_property.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/api_property.html:10: <table
class="innerTable">

It is a table that is inside another, and thus has slightly different CSS
properties (mostly margins and padding).

On 2014/01/03 23:30:24, kalman - OOO wrote:
> what is "innerTable" supposed to be? I see it used a bunch but it's not clear
> from context why it should be "inner". is there a semantically meaningful name
> it could be?

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/api_summary.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/api_summary.html:1: <section
id="toc">
On 2014/01/03 23:30:24, kalman - OOO wrote:
> unless you hide the TOC for the main page (which... you're not right, since
> there's also potentially an intro on these pages) this means two elements will
> have the same ID. why does this need to have "toc" id?

I'm not sure if I follow you. This IS the TOC for api.html pages. id="toc"
section is only defined here and in article.html, which is in a different
hierarchy. article.html is used in standard_apps_article.html, and api.html is
used in standard_apps_api.html.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/callback.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/callback.html:9: <br/>
On 2014/01/03 23:30:24, kalman - OOO wrote:
> prefer to separate blocks via block elements rather than <br> if possible. the
> <code> block seems semantically separate from the text above.

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/function.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/function.html:2:
{{?parentName}}<tr><td>{{/}}
On 2014/01/03 23:30:24, kalman - OOO wrote:
> why do you need this? can you do this from where this template is included
> instead?

Not easily. This template is used in three different places, but the main issue
is with type.html: it calls type_item sending this template as one of the
parameters. However, <tr><td> only applies to when type_item runs with
function.html as a parameter, but not to the other situations (parameter_full
and event).

Note that this is required because the current template dependency graph is
complex. We could rework it, but I would prefer to do it on a separate CL.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/function_signature.html
(right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/function_signature.html:5:
|no_link|: if different of null, no link will be shown for types
On 2014/01/03 23:31:43, kalman - OOO wrote:
> On 2014/01/03 23:30:24, kalman wrote:
> > no_link -> disableLink
> > 
> > also: "if not false, ..."
> 
> ah i see that it's actually a matter of null vs not-null. in that case, "if
not
> null"...

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/parameter_full.html
(right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/parameter_full.html:16:
innerTable:true
On 2014/01/03 23:30:24, kalman - OOO wrote:
> I can't find innerTable used anywhere

Good catch. Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/property.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/property.html:22:
innerTable:true
On 2014/01/03 23:30:24, kalman - OOO wrote:
> likewise innerTable

Done.

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/ref_link.html (right):

https://codereview.chromium.org/121713002/diff/30001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/ref_link.html:1: {{^no_link}}<a
href="{{link.href}}">{{/}}
On 2014/01/03 23:30:24, kalman - OOO wrote:
> given that no_link is only passed in from one place (variable_type.html) you
> could do there
> 
> {{?disableLink}}
>   {{link.text}}
> {{:disableLink}}
>   {{+partials.ref_link link:link/}}
> {{/disableLink}}
> 
> and not need to pass it in here; since passing "no link" into a file called
"...
> link" looks a bit weird.

Done.