Caterpillar now generates app.info.js, a metadata script. Resolves #12.

Caterpillar now generates app.info.js, a metadata script. Resolves #12.

app.info.js is used by the runtime polyfill and possibly in future
polyfills. It includes the manifest file and the app ID.

https://github.com/chromium/caterpillar/issues/12

R=mgiuca@chromium.org, raymes@chromium.org

Committed: 122627e4dfaf7e35e439b1f6a6250b56527b2496

[Matthew Alger, 2016-01-28 06:38:16]

alger@google.com changed reviewers:
+ mgiuca@chromium.org, raymes@chromium.org

[Matthew Alger, 2016-01-28 06:38:17]

mgiuca: Could you please look at caterpillar.py, caterpillar_test.py, and
configuration.py? These are the main changes in the CL.

raymes: I've updated the runtime polyfill to make use of app.info.js, and also
updated the tests accordingly. Could you please look at runtime.polyfill.js,
runtime.polyfill.test.js, and app.info.js?

Thanks.

[Matt Giuca, 2016-01-28 07:33:23]

Hmm, I have a general feeling that we shouldn't do this.

The user is migrating away from a Chrome app. While we want our polyfills to
provide all the existing services (to make conversion less painful), I think we
can make an exception here: it doesn't make sense to give the web app knowledge
about the app ID that it used to be back when it was a Chrome app. By
definition, any app relying on this will now be wrong.

I think this should just return null (and the report says "hey, you used the
appId, that isn't a thing any more"). And the user should not put the app ID
into the config.

https://codereview.chromium.org/1644913002/diff/1/src/caterpillar_test.py
File src/caterpillar_test.py (right):

https://codereview.chromium.org/1644913002/diff/1/src/caterpillar_test.py#new...
src/caterpillar_test.py:638: self.assertEqual(app_info_js,
"""chrome.caterpillar.manifest = {
"""\
Start on the next line.

https://codereview.chromium.org/1644913002/diff/1/src/configuration.py
File src/configuration.py (right):

https://codereview.chromium.org/1644913002/diff/1/src/configuration.py#newcode37
src/configuration.py:37: 'id': ('Chrome App ID',
'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),
Does it have to be aaaaaaaaaaaaa or can it just be the empty string or
something?

[Matthew Alger, 2016-01-28 23:23:18]

Patchset #3 (id:40001) has been deleted

[Matthew Alger, 2016-01-28 23:24:45]

I agree with not having the IDs; I've removed them.

I think having the manifest available is still important though.

https://codereview.chromium.org/1644913002/diff/1/src/caterpillar_test.py
File src/caterpillar_test.py (right):

https://codereview.chromium.org/1644913002/diff/1/src/caterpillar_test.py#new...
src/caterpillar_test.py:638: self.assertEqual(app_info_js,
"""chrome.caterpillar.manifest = {
On 2016/01/28 07:33:22, Matt Giuca wrote:
> """\
> Start on the next line.

Done.

https://codereview.chromium.org/1644913002/diff/1/src/configuration.py
File src/configuration.py (right):

https://codereview.chromium.org/1644913002/diff/1/src/configuration.py#newcode37
src/configuration.py:37: 'id': ('Chrome App ID',
'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'),
On 2016/01/28 07:33:22, Matt Giuca wrote:
> Does it have to be aaaaaaaaaaaaa or can it just be the empty string or
> something?

This is the sample App ID used in the manifest docs
(https://developer.chrome.com/apps/manifest). If there's going to be a default I
think it should be of the same form as actual inputs.

Removed this line anyway as part of removing IDs.

[Matthew Alger, 2016-01-28 23:25:32]

https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:455: separators=(',', ': ')).replace('"', "'")
Changed this to drop trailing spaces by explicitly specifying separators.
Otherwise, the tests have trailing whitespace on the ends of some lines...

[Matt Giuca, 2016-01-29 02:34:27]

> I think having the manifest available is still important though.

What is it for, though? Now that the runtime module doesn't access the app ID,
the manifest is not being used. Do you have plans to use it?

https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:455: separators=(',', ': ')).replace('"', "'")
On 2016/01/28 23:25:32, Matthew Alger wrote:
> Changed this to drop trailing spaces by explicitly specifying separators.
> Otherwise, the tests have trailing whitespace on the ends of some lines...

Fair enough.

Why is " being replaced with '? Isn't that invalid JSON?

[Matthew Alger, 2016-01-29 02:44:03]

On 2016/01/29 02:34:27, Matt Giuca wrote:
> > I think having the manifest available is still important though.
> 
> What is it for, though? Now that the runtime module doesn't access the app ID,
> the manifest is not being used. Do you have plans to use it?

runtime.polyfill.js:144 - getManifest

> https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py
> File src/caterpillar.py (right):
> 
>
https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py#newc...
> src/caterpillar.py:455: separators=(',', ': ')).replace('"', "'")
> On 2016/01/28 23:25:32, Matthew Alger wrote:
> > Changed this to drop trailing spaces by explicitly specifying separators.
> > Otherwise, the tests have trailing whitespace on the ends of some lines...
> 
> Fair enough.
> 
> Why is " being replaced with '? Isn't that invalid JSON?

Because I'm not generating JSON, I'm generating JavaScript (using the JSON
generator...). The rest of the code uses ' for strings.

[Matt Giuca, 2016-01-29 03:06:53]

Ah, I didn't see the chrome.app.manifest part.

I think the same principle applies to that as to appId (which I argued for
above). Either the app should have access to its original Chrome App ID and
manifest, or we should be getting away from that world and neither should be
available.

I think in this case, I would remove both and then remove this whole app.info.js
thing. Do you know of a specific need for the converted app to access its Chrome
App manifest?

https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644913002/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:455: separators=(',', ': ')).replace('"', "'")
On 2016/01/29 02:34:27, Matt Giuca wrote:
> On 2016/01/28 23:25:32, Matthew Alger wrote:
> > Changed this to drop trailing spaces by explicitly specifying separators.
> > Otherwise, the tests have trailing whitespace on the ends of some lines...
> 
> Fair enough.
> 
> Why is " being replaced with '? Isn't that invalid JSON?

Acknowledged.

[Matthew Alger, 2016-01-29 03:21:17]

On 2016/01/29 03:06:53, Matt Giuca wrote:
> Ah, I didn't see the chrome.app.manifest part.
> 
> I think the same principle applies to that as to appId (which I argued for
> above). Either the app should have access to its original Chrome App ID and
> manifest, or we should be getting away from that world and neither should be
> available.
> 
> I think in this case, I would remove both and then remove this whole
app.info.js
> thing. Do you know of a specific need for the converted app to access its
Chrome
> App manifest?

It's hard to tell whether getManifest is actually used. We *can* support it,
though, and one of the aims of the polyfills is to support as much as possible.
If that's not a sufficient reason we can drop this CL.

[Matt Giuca, 2016-01-29 03:24:04]

On 2016/01/29 03:21:17, Matthew Alger wrote:
> On 2016/01/29 03:06:53, Matt Giuca wrote:
> > Ah, I didn't see the chrome.app.manifest part.
> > 
> > I think the same principle applies to that as to appId (which I argued for
> > above). Either the app should have access to its original Chrome App ID and
> > manifest, or we should be getting away from that world and neither should be
> > available.
> > 
> > I think in this case, I would remove both and then remove this whole
> app.info.js
> > thing. Do you know of a specific need for the converted app to access its
> Chrome
> > App manifest?
> 
> It's hard to tell whether getManifest is actually used. We *can* support it,
> though, and one of the aims of the polyfills is to support as much as
possible.
> If that's not a sufficient reason we can drop this CL.

Let's ask Raymes. I am ambivalent about this.

[raymes, 2016-01-31 23:54:41]

.js lgtm

[Matt Giuca, 2016-02-01 00:05:59]

OK lgtm, I am convinced (as discussed, I think it is useful for a web app to
continue accessing info about itself, such as its name, version, etc).

[Matthew Alger, 2016-02-01 06:22:38]

Description was changed from

==========
Caterpillar now generates app.info.js, a metadata script. Resolves #12.

app.info.js is used by the runtime polyfill and possibly in future
polyfills. It includes the manifest file and the app ID.

https://github.com/chromium/caterpillar/issues/12
==========

to

==========
Caterpillar now generates app.info.js, a metadata script. Resolves #12.

app.info.js is used by the runtime polyfill and possibly in future
polyfills. It includes the manifest file and the app ID.

https://github.com/chromium/caterpillar/issues/12

R=mgiuca@chromium.org, raymes@chromium.org

Committed: 122627e4dfaf7e35e439b1f6a6250b56527b2496
==========

[Matthew Alger, 2016-02-01 06:22:42]

Committed patchset #3 (id:60001) manually as
122627e4dfaf7e35e439b1f6a6250b56527b2496 (presubmit successful).