<appview> documentation.

Adding <appview> documentation.

Committed: https://crrev.com/7de34a730a9541db1e58d142091551795bf1b889
Cr-Commit-Position: refs/heads/master@{#319150}

[lfg, 2015-02-12 22:28:19]

lfg@chromium.org changed reviewers:
+ fsamuel@chromium.org

[lfg, 2015-02-12 22:28:19]

PTAL.

Thanks!

[Fady Samuel, 2015-03-02 22:59:25]

lgtm

[lfg, 2015-03-02 23:00:50]

lfg@chromium.org changed reviewers:
+ kalman@chromium.org

[lfg, 2015-03-02 23:00:51]

Hi kalman@, PTAL.

Thanks!

[not at google - send to devlin, 2015-03-02 23:08:10]

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/appview_tag.json (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/appview_tag.json:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/appview_tag.json:7: "namespace": "appviewTag",
Consider writing this in IDL instead of JSON.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/intros/appview_tag.html (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/intros/appview_tag.html:13: embedded.
I'm confused whether this documentation is for apps that want to embed other
apps, or apps that want to be embedded, or both. I think the latter? Please have
clearer sections than just "Example".

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/intros/appview_tag.html:23:
<pre>chrome.app.runtime.onEmbedRequested.addListener(function(request) {<br> 
request.allow("foo.html");<br>});</pre>
<pre data-filename="background.js">

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/intros/appview_tag.html:32: <pre>var
appview =
document.createElement("appview");<br>document.body.appendChild(appview);<br>appview.connect(appToEmbed,
{}, function(success) {});</pre>
Ditto.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/json/chrome_sidenav.json (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/json/chrome_sidenav.json:490: },
It makes more sense for this to be under the Webview Tag entry.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/public/apps/redirects.json (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/public/apps/redirects.json:5:
"appview_tag": "tags/appview",
You don't need this, the webview entries are because that's where the docs used
to be.

[lfg, 2015-03-03 20:48:48]

Please, take another look.


Thanks!

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/appview_tag.json (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/appview_tag.json:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/03/02 23:08:10, kalman wrote:
> 2015

Done.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/appview_tag.json:7: "namespace": "appviewTag",
On 2015/03/02 23:08:10, kalman wrote:
> Consider writing this in IDL instead of JSON.

Done.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/intros/appview_tag.html (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/intros/appview_tag.html:13: embedded.
On 2015/03/02 23:08:10, kalman wrote:
> I'm confused whether this documentation is for apps that want to embed other
> apps, or apps that want to be embedded, or both. I think the latter? Please
have
> clearer sections than just "Example".

Done.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/intros/appview_tag.html:23:
<pre>chrome.app.runtime.onEmbedRequested.addListener(function(request) {<br> 
request.allow("foo.html");<br>});</pre>
On 2015/03/02 23:08:10, kalman wrote:
> <pre data-filename="background.js">

Done.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/intros/appview_tag.html:32: <pre>var
appview =
document.createElement("appview");<br>document.body.appendChild(appview);<br>appview.connect(appToEmbed,
{}, function(success) {});</pre>
On 2015/03/02 23:08:10, kalman wrote:
> Ditto.

Done.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/json/chrome_sidenav.json (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/json/chrome_sidenav.json:490: },
On 2015/03/02 23:08:10, kalman wrote:
> It makes more sense for this to be under the Webview Tag entry.

I put it below the Webview tag, can't put it under it because Webview tag entry
isn't an article, and the api pages don't have the sidenav.

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/public/apps/redirects.json (right):

https://codereview.chromium.org/921793004/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/public/apps/redirects.json:5:
"appview_tag": "tags/appview",
On 2015/03/02 23:08:10, kalman wrote:
> You don't need this, the webview entries are because that's where the docs
used
> to be.

Done.

[not at google - send to devlin, 2015-03-03 23:50:59]

https://codereview.chromium.org/921793004/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/appview_tag.html (right):

https://codereview.chromium.org/921793004/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/appview_tag.html:23: <pre
data-filename="background.js">chrome.app.runtime.onEmbedRequested.addListener(function(request)
{<br>  request.allow("foo.html");<br>});</pre>
If this is in <pre> you don't need the <br>s, just write it plain.

<pre data-filename="background.js">
chrome.app.runtime.omEmbedRequested.addListener(function(request) {
  request.allow("foo.html");
});
</pre>

and comment what allow() does.

Same below.

https://codereview.chromium.org/921793004/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/appview_tag.html:34: <pre
data-filename="background.js">var appview =
document.createElement("appview");<br>document.body.appendChild(appview);<br>appview.connect(appToEmbed,
{}, function(success) {});</pre>
What is the {} after appToEmbed? It looks odd. It should either be optional in
the API and you leave it out, or you should give it some realistic value, or you
should comment what it is. Comment stuff either way.

https://codereview.chromium.org/921793004/diff/60001/tools/json_schema_compil...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/921793004/diff/60001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema.py:376: if self.compiler_options is not
None:
We should write both of these like:

compiler_options = self.compiler_options or {}
documentation_options = self.documentation_options or {}

https://codereview.chromium.org/921793004/diff/60001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema.py:462:
documentation_options['documented_in'] = node.value
Let's make this a little less wordy:
documentation_options_title --> documentation_title
documentation_options_namespace --> documentation_namespace
documentation_options_documented_in --> documented_in

[lfg, 2015-03-04 21:13:02]

https://codereview.chromium.org/921793004/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/appview_tag.html (right):

https://codereview.chromium.org/921793004/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/appview_tag.html:23: <pre
data-filename="background.js">chrome.app.runtime.onEmbedRequested.addListener(function(request)
{<br>  request.allow("foo.html");<br>});</pre>
On 2015/03/03 23:50:58, kalman wrote:
> If this is in <pre> you don't need the <br>s, just write it plain.
> 
> <pre data-filename="background.js">
> chrome.app.runtime.omEmbedRequested.addListener(function(request) {
>   request.allow("foo.html");
> });
> </pre>
> 
> and comment what allow() does.
> 
> Same below.

Done.

https://codereview.chromium.org/921793004/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/appview_tag.html:34: <pre
data-filename="background.js">var appview =
document.createElement("appview");<br>document.body.appendChild(appview);<br>appview.connect(appToEmbed,
{}, function(success) {});</pre>
On 2015/03/03 23:50:58, kalman wrote:
> What is the {} after appToEmbed? It looks odd. It should either be optional in
> the API and you leave it out, or you should give it some realistic value, or
you
> should comment what it is. Comment stuff either way.

Done.

https://codereview.chromium.org/921793004/diff/60001/tools/json_schema_compil...
File tools/json_schema_compiler/idl_schema.py (right):

https://codereview.chromium.org/921793004/diff/60001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema.py:376: if self.compiler_options is not
None:
On 2015/03/03 23:50:59, kalman wrote:
> We should write both of these like:
> 
> compiler_options = self.compiler_options or {}
> documentation_options = self.documentation_options or {}

Done.

https://codereview.chromium.org/921793004/diff/60001/tools/json_schema_compil...
tools/json_schema_compiler/idl_schema.py:462:
documentation_options['documented_in'] = node.value
On 2015/03/03 23:50:59, kalman wrote:
> Let's make this a little less wordy:
> documentation_options_title --> documentation_title
> documentation_options_namespace --> documentation_namespace
> documentation_options_documented_in --> documented_in

Done.

[not at google - send to devlin, 2015-03-04 21:22:57]

lgtm

https://codereview.chromium.org/921793004/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/appview_tag.html (right):

https://codereview.chromium.org/921793004/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/appview_tag.html:39: var appview
= document.createElement("appview"); // Creates an <appview> element.
Keep in mind that the amount of space you're given to write code is quite small,
because if screen size isn't too large it will be < 80 characters.

In this case (and above), I would suggest putting the comments on the line
above, rather than at the end of lines.

I would also check that
"chrome.app.runtime.onEmbedRequested.addListener(function(request) {" doesn't
get wrapped. You may want to change it to

chrome.app.runtime.onEmbedRequested.addListener(
    function(request) {
  // Allows the embedder to embed foo.html.
  request.allow("foo.html");
});

... *if* it wraps. Or, whatever style you choose. You can test the documentation
by running chrome/common/extensions/docs/server2/preview.py

[lfg, 2015-03-04 21:36:14]

https://codereview.chromium.org/921793004/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/intros/appview_tag.html (right):

https://codereview.chromium.org/921793004/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/templates/intros/appview_tag.html:39: var appview
= document.createElement("appview"); // Creates an <appview> element.
On 2015/03/04 21:22:57, kalman wrote:
> Keep in mind that the amount of space you're given to write code is quite
small,
> because if screen size isn't too large it will be < 80 characters.
> 
> In this case (and above), I would suggest putting the comments on the line
> above, rather than at the end of lines.

I've changed this so that the comments are above the lines. It still wraps on
very small screens, but even when it does, the color-coding still makes it
distinguished.

> I would also check that
> "chrome.app.runtime.onEmbedRequested.addListener(function(request) {" doesn't
> get wrapped. You may want to change it to
> 
> chrome.app.runtime.onEmbedRequested.addListener(
>     function(request) {
>   // Allows the embedder to embed foo.html.
>   request.allow("foo.html");
> });
> 
> ... *if* it wraps. Or, whatever style you choose. You can test the
documentation
> by running chrome/common/extensions/docs/server2/preview.py

This one doesn't wrap unless the screen is small, and it doesn't look very good
when there's enough space to display everything. Again, the color-coding already
helps, so I've left this one as-is.

[lfg, 2015-03-04 21:36:35]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-03-04 21:36:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/921793004/#ps100001
(title: "wrapping")

[commit-bot: I haz the power, 2015-03-04 21:38:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/921793004/100001

[lfg, 2015-03-04 21:38:33]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2015-03-04 21:46:17]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-03-04 21:46:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/921793004/100001

[commit-bot: I haz the power, 2015-03-04 22:49:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-04 22:50:57]

Patchset 6 (id:??) landed as
https://crrev.com/7de34a730a9541db1e58d142091551795bf1b889
Cr-Commit-Position: refs/heads/master@{#319150}