Add documentation for the chrome.sockets API and manifest key.

Add documentation for
the chrome.sockets API and manifest key.

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

[rpaquay, 2014-01-02 19:07:01]

Ben,

I am trying to add documentation for the new sockets.* APIs. I have a few
issues:

* On the doc server running locally, I don't know how to properly add a
documentation page for the
"http://localhost:8000/apps/manifest/manifestTypes.html#type-SocketHostPatterns"
link, referenced at the bottom of the
"http://localhost:8000/apps/manifest/sockets.html" page.

* On the doc server running off the patch, none of my changes seem to be taken
into account. For example,
"https://chrome-apps-doc.appspot.com/_patch/94093013/apps/api_index.html" does
not show any chrome.sockets.* API entry. I am not sure if this is a known
infrastructure issue, as this is working as expected on my local doc server
running off the enlistment.

[not at google - send to devlin, 2014-01-03 00:00:25]

Reviewing now, responding to comments first:

On 2014/01/02 19:07:01, rpaquay wrote:
> Ben,
> 
> I am trying to add documentation for the new sockets.* APIs. I have a few
> issues:
> 
> * On the doc server running locally, I don't know how to properly add a
> documentation page for the
>
"http://localhost:8000/apps/manifest/manifestTypes.html#type-SocketHostPatterns"
> link, referenced at the bottom of the
> "http://localhost:8000/apps/manifest/sockets.html" page.

Ah right. This is generated by some $ref right? There are a couple of ways we
could fix this, but the best would probably be to allow an annotation like
"documentedAt": "manifest/sockets/sockets" in that SocketHostPatterns type in
sockets.json.

> 
> * On the doc server running off the patch, none of my changes seem to be taken
> into account. For example,
> "https://chrome-apps-doc.appspot.com/_patch/94093013/apps/api_index.html" does
> not show any chrome.sockets.* API entry. I am not sure if this is a known
> infrastructure issue, as this is working as expected on my local doc server
> running off the enlistment.

It seems we don't document trunk APIs at the moment, that's fine, though we
should (perhaps under "private.html"? eh, later).

[rpaquay, 2014-01-03 00:15:49]

On 2014/01/03 00:00:25, kalman wrote:
> Reviewing now, responding to comments first:
> 
> On 2014/01/02 19:07:01, rpaquay wrote:
> > Ben,
> > 
> > I am trying to add documentation for the new sockets.* APIs. I have a few
> > issues:
> > 
> > * On the doc server running locally, I don't know how to properly add a
> > documentation page for the
> >
>
"http://localhost:8000/apps/manifest/manifestTypes.html#type-SocketHostPatterns"
> > link, referenced at the bottom of the
> > "http://localhost:8000/apps/manifest/sockets.html" page.
> 
> Ah right. This is generated by some $ref right?

Not by the doc I wrote, but maybe indirectly? The whole section at the bottom of
the page (the "sockets" manifest entry reference) is generated with a single
line:

{{+partials.type type:apis.manifestTypes.byName.sockets/}}


> There are a couple of ways we
> could fix this, but the best would probably be to allow an annotation like
> "documentedAt": "manifest/sockets/sockets" in that SocketHostPatterns type in
> sockets.json.

I see. So you mean I would need to update the doc server to support this new
syntax, correct?

> 
> > 
> > * On the doc server running off the patch, none of my changes seem to be
taken
> > into account. For example,
> > "https://chrome-apps-doc.appspot.com/_patch/94093013/apps/api_index.html"
does
> > not show any chrome.sockets.* API entry. I am not sure if this is a known
> > infrastructure issue, as this is working as expected on my local doc server
> > running off the enlistment.
> 
> It seems we don't document trunk APIs at the moment, that's fine, though we
> should (perhaps under "private.html"? eh, later).

Not sure what you mean by "trunk API"? You mean you look only in the "stable"
branch?

In anyways, it sounds like looking at the doc generated by the local server is
currently a reliable way for me to figure out how the doc will look once
everything is in the "right" branch, yes?

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

> 
> Not by the doc I wrote, but maybe indirectly? The whole section at the bottom
of
> the page (the "sockets" manifest entry reference) is generated with a single
> line:
> 
> {{+partials.type type:apis.manifestTypes.byName.sockets/}}

Yeah indirectly, like here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

> 
> 
> > There are a couple of ways we
> > could fix this, but the best would probably be to allow an annotation like
> > "documentedAt": "manifest/sockets/sockets" in that SocketHostPatterns type
in
> > sockets.json.
> 
> I see. So you mean I would need to update the doc server to support this new
> syntax, correct?

Well we'd need to actually support the new syntax (actually it'd probably be
"documentedIn" similar to how we already have "implementedIn") but I can get
somebody else to do that. Or... you can do it if you feel inspired :) I would
just submit what you have here and then worry about fixing the broken link in a
later patch.

> 
> Not sure what you mean by "trunk API"? You mean you look only in the "stable"
> branch?

Ah, so, some background: the api_index page is generated by stepping back
through svn history and finding the revision that an API was added at (and how
it appears in the features file). When you add an API and it hasn't reached even
dev channel yet (as is the case here obviously, because the patch is only
applied on the trunk branch) the server decides that it's a trunk API. Since
trunk APIs aren't listed in api_index.html, it doesn't get listed.

If we were to list the trunk APIs somewhere, for example in the private API
section (seems reasonable) then at least it'd be linked to, but it's not very
important really for the purpose of previewing docs.

> 
> In anyways, it sounds like looking at the doc generated by the local server is
> currently a reliable way for me to figure out how the doc will look once
> everything is in the "right" branch, yes?

Yep. With the preview server it doesn't try to look up the branch history (it
would be way too slow; also preview.py should work offline) and just assumes
everything is stable, which is why you see it there.

[not at google - send to devlin, 2014-01-03 00:24:41]

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (left):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:764: "sockets.udp": {
Could you submit these non-documentation changes separately? It's easier for
patch history, dcommitting, sherrifs, figuring out breakages, etc.

[not at google - send to devlin, 2014-01-03 03:02:13]

The api_index (and intro table for sockets) has the overview:

"Use the chrome.sockets.udp API to send and receive data over the network using
UDP connections. This API supersedes the UDP functionality previously found in
the "socket" API. Note that the socket ids created from this namespace are not
compatible with ids created in other namespaces."

That last sentence looks odd particularly in the api index, and should be moved
elsewhere.

CL description nits:
- needs BUG
- don't mention docserver problems
- when you pull the code changes out that line won't be relevant

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/articles/manifest/sockets.html
(right):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/manifest/sockets.html:5:
available for the <a href="../sockets_udp.html">chrome.sockets.udp</a>, <a
href="../sockets_tcp.html">chrome.sockets.tcp</a> and <a
href="../sockets_tcp_server.html">chrome.sockets.tcpServer</a> APIs.
ben wells did some work to make $ref's work from directories other than the
current one. you should be able to do $ref:socket.url $ref:socket.tcp etc.

also: try to keep < 80 characters. easier to edit + make review comments :)

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/manifest/sockets.html:35: 
this should probably go in its own <section>, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/apps/manifest/sockets.html
(right):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/apps/manifest/sockets.html:1:
{{+partials.standard_apps_article
1 line

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/apps/sockets_tcp_server.html
(right):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/apps/sockets_tcp_server.html:1:
{{+partials.standard_apps_api api:apis.sockets_tcp_server/}}
you shouldn't need this file. the docserver will know how to redirect from this
to tcpServer, and nothing should be linking to it; is anything linking to it?

[rpaquay, 2014-01-06 22:28:29]

ptal.

> The api_index (and intro table for sockets) has the overview:
> 
> "Use the chrome.sockets.udp API to send and receive data over the network
using
> UDP connections. This API supersedes the UDP functionality previously found in
> the "socket" API. Note that the socket ids created from this namespace are not
> compatible with ids created in other namespaces."
> 
> That last sentence looks odd particularly in the api index, and should be
moved
> elsewhere.

Done. I moved the text to the "creatInfo.socketId" property.

> 
> CL description nits:
> - needs BUG
> - don't mention docserver problems
> - when you pull the code changes out that line won't be relevant

Done.

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (left):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:764: "sockets.udp": {
On 2014/01/03 00:24:41, kalman wrote:
> Could you submit these non-documentation changes separately? It's easier for
> patch history, dcommitting, sherrifs, figuring out breakages, etc.

Done.

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/articles/manifest/sockets.html
(right):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/manifest/sockets.html:5:
available for the <a href="../sockets_udp.html">chrome.sockets.udp</a>, <a
href="../sockets_tcp.html">chrome.sockets.tcp</a> and <a
href="../sockets_tcp_server.html">chrome.sockets.tcpServer</a> APIs.
On 2014/01/03 03:02:14, kalman wrote:
> ben wells did some work to make $ref's work from directories other than the
> current one. you should be able to do $ref:socket.url $ref:socket.tcp etc.
> 
> also: try to keep < 80 characters. easier to edit + make review comments :)

Done.

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/manifest/sockets.html:35: 
On 2014/01/03 03:02:14, kalman wrote:
> this should probably go in its own <section>, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

This article follows the same pattern as "externally_connectable":
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/templates/public/apps/sockets_tcp_server.html
(right):

https://codereview.chromium.org/94093013/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/public/apps/sockets_tcp_server.html:1:
{{+partials.standard_apps_api api:apis.sockets_tcp_server/}}
On 2014/01/03 03:02:14, kalman wrote:
> you shouldn't need this file. the docserver will know how to redirect from
this
> to tcpServer, and nothing should be linking to it; is anything linking to it?

My bad. This was a leftover from a previous local commit.

[not at google - send to devlin, 2014-01-07 22:46:28]

lgtm, but 2 more comments:

1. could you change the CL description to be more like "Add documentation for
the chrome.sockets API and manifest key".

2. consider adding an article at /sockets.html which briefly explains the
difference between the socket APIs with links to each.

[not at google - send to devlin, 2014-01-07 22:46:41]

(2) as a follow-up

[rpaquay, 2014-01-08 17:38:09]

On 2014/01/07 22:46:28, kalman wrote:
> lgtm, but 2 more comments:
> 
> 1. could you change the CL description to be more like "Add documentation for
> the chrome.sockets API and manifest key".
Done.

> 2. consider adding an article at /sockets.html which briefly explains the
> difference between the socket APIs with links to each.

I have been working on updating "app_network.html", which is the intro to the
socket API. It is coming in a follow up cl.

[commit-bot: I haz the power, 2014-01-08 17:39:59]

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

[commit-bot: I haz the power, 2014-01-08 22:44:25]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-08 23:33:58]

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

[commit-bot: I haz the power, 2014-01-09 01:36:38]

Change committed as 243728