Doc Server Manifest Generation Followup

Doc Server Manifest Generation Followup

Properties of the example manifest json that are children of other properties are now displayed as sub-json objects. Ex. app.background is now displayed as a property of the app property.

Some invalid URLs were created in the last patch and have now be removed.

There is now a second issue associated with this patch that is relative to the
previous manifest patch: https://codereview.chromium.org/16410002

The previous patch is here: https://codereview.chromium.org/14273041/

[jshumway, 2013-05-31 03:25:13]

Properties are now put into their parent json object on the manifest example
page.

Invalid URLs have been removed.

I was having some trouble uploading this CL and accidentally pulled the entirety
of my previous patch into this one.

[not at google - send to devlin, 2013-05-31 18:27:21]

It's good to upload a version of this patch against master - easier for me to
patch it in and test it out myself.

While I'm on the topic, I patched this in and the nesting under app/ looks
*awesome*, exactly what I had in mind, though it's missing "isolation" and
"launch". However it doesn't seem to be working for background.persistent?
Likewise for extensions. And by the way, extensions manifest has "app". It's
almost like the auto generation isn't being applies to it properly.

Perhaps it will be easier to see what's going on in review - speaking of which,
the diff agains the existing patch is better (you should maintain 2 separate
codereviews if you can):

git cl upload <other branch>

then it will diff against the other branch. that will be much easier to review.

Sorry about the extra pain this is causing. You can switch issues with "git cl
issue <issue>" and "git cl issue 0" to start a new one.

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

https://codereview.chromium.org/15842014/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:20:
{{+partials.manifest_example manifest_source:manifest_source.apps}}
is your existing patch like this?

This template should just be called manifest.html and take the manifest_source
directly from the templates in public. No need for both an extensions and apps
manifest file.

[jshumway, 2013-06-05 03:02:39]

I was looking into some of the properties and found some interesting things:

First, I was displaying background.persistent incorrectly, which lead me to find
that I was displaying quite a few properties wrong. In the template I do a check
{{?example}} to see if an example key is present and if it is, render it as
literal json. The only problem is that {{?example}} cannot distinguish between a
property missing and the value of that property being logically false. Examples
with the values 'false' and '[]' were being omitted improperly. I decided to
wrap each example value in an object with the single key 'value' which would
contain the actual example data. I can now test if example exists, then render
example.value if it does.

I dug around through _manifest_features.json and the properties like 'isolation'
and 'launch' under app, and 'app' for extensions, are not being displayed
because the required 'extension_types' values are missing. 'isolation' and
'launch' do not apply to platform_app, the only type whose values we are
rendering.

There is now a second issue associated with this patch that is relative to the
previous manifest patch: https://codereview.chromium.org/16410002

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

https://codereview.chromium.org/15842014/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/templates/articles/apps_manifest.html:20:
{{+partials.manifest_example manifest_source:manifest_source.apps}}
On 2013/05/31 18:27:21, kalman wrote:
> is your existing patch like this?
> 
> This template should just be called manifest.html and take the manifest_source
> directly from the templates in public. No need for both an extensions and apps
> manifest file.

My existing patch is like this. When I was creating distinct manifest pages in
the first patch, I wasn't sure if the few paragraphs before the manifest example
were going to be unique between the two pages, so I kept these templates
separate.

[not at google - send to devlin, 2013-06-11 19:00:08]

Ok I'm burnt out on reviews today :( but it looks like you have a lot of stuff
to go on with anyway. I will do it asap tomorrow.

[jshumway, 2013-06-19 00:58:38]

Quick change to add /apps/contentSecurityPolicy.html as a valid URL that directs
to the same page as /apps/app_csp.html, thus including the page that was added
to the old manifest document using an {{?is_app}} wrapper.

[not at google - send to devlin, 2013-06-19 15:45:09]

I would rather we actually do a 302 redirect than just shoving the content into
that doc. It has an all-but-identical effect, but maintains the single canonical
location for those CSP docs, which is something to aim for.

I.e. let's land your redirect patch first then do that.

In any case - could you point me at this code review rebased against your first
patch? Maybe we should just land it in one lump - easier for me to cherry-pick.

Ping me when it's done.

[jshumway, 2013-06-20 23:53:18]

On 2013/06/19 15:45:09, kalman wrote:
> I would rather we actually do a 302 redirect than just shoving the content
into
> that doc. It has an all-but-identical effect, but maintains the single
canonical
> location for those CSP docs, which is something to aim for.
> 
> I.e. let's land your redirect patch first then do that.
> 
> In any case - could you point me at this code review rebased against your
first
> patch? Maybe we should just land it in one lump - easier for me to
cherry-pick.
> 
> Ping me when it's done.

Here is the link to this patch vs the previous manifest patch:
https://codereview.chromium.org/16410002