Stubbed runtime and app.runtime in caterpillar.js. Resolves #8.

Stubbed runtime and app.runtime in caterpillar.js. Resolves #8.

Currently, caterpillar.py stubs app.runtime so that the initial
call to onLaunched for window creation won't fail. If that call
fails, then the service worker won't install, as it has a
script error. This CL moves this stubbing to caterpillar.js,
which is always a dependency. This is an improvement over the
existing location of the stub, since the stub is common code
and static, and so should not be included in a generated file.

Also, currently the runtime polyfill is required as a
dependency for caterpillar.js, since caterpillar.setError needs
to access the runtime namespace. This CL adds a runtime stub to
caterpillar.js so runtime is no longer needed as a dependency.
This resolves issue #8.

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

R=raymes@chromium.org

Committed: a2d90e3ad21309465a3eaf945a5da5f8c22a992c

[Matthew Alger, 2016-01-28 04:51:41]

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

[Matthew Alger, 2016-01-28 04:51:41]

This CL stubs both runtime and app.runtime in the common Caterpillar JS. This
involves a move of some code from the generated service worker into
caterpillar.js.

raymes: Could you please take a look at the changes in both caterpillar.js and
caterpillar.py? For context, the changed part of caterpillar.py is just the
static part of the Caterpillar-generated service worker, so the only changes in
that file are JavaScript.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:36: if (!chrome.app)
Note that Chrome exports chrome.app, so we can't just check if chrome.app exists
and only stub then.

[raymes, 2016-01-31 23:44:37]

Could you expand the CL description? You should always explain why you are
making a change in the CL description (not just relying the on bug report for
context)

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:35: // in Chrome Apps.
I'm not sure what this comment means? Why is it stubbed out? Is it unsupported?

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:36: if (!chrome.app)
On 2016/01/28 04:51:41, Matthew Alger wrote:
> Note that Chrome exports chrome.app, so we can't just check if chrome.app
exists
> and only stub then.

Could we just always override onLaunched? (do we really have to check whether it
exists?)

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:44: }
Why did this code need to be moved?

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:57: console.warn('Error set in lastError:', message);
Do we need to log warnings every time?

[Matthew Alger, 2016-02-01 00:14:34]

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:35: // in Chrome Apps.
On 2016/01/31 23:44:37, raymes wrote:
> I'm not sure what this comment means? Why is it stubbed out? Is it
unsupported? 

chrome.app.runtime.onLaunched.addListener is used in the boilerplate of every
Chrome App to launch the main window. The fact we're moving the app from the
Chrome Apps platform to the web means this line is never necessary (unless you
are using multiple windows, which we don't support anyway). We don't support the
listener itself, so it needs to be stubbed out, or the service worker (which
imports all background scripts) will break (as undefined has no property
onLaunched).

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:36: if (!chrome.app)
On 2016/01/31 23:44:37, raymes wrote:
> On 2016/01/28 04:51:41, Matthew Alger wrote:
> > Note that Chrome exports chrome.app, so we can't just check if chrome.app
> exists
> > and only stub then.
> 
> Could we just always override onLaunched? (do we really have to check whether
it
> exists?)

It's not obvious to me that Chrome doesn't/won't export chrome.app.runtime, and
if it does, we don't want to override that.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:44: }
On 2016/01/31 23:44:37, raymes wrote:
> Why did this code need to be moved?

- It's static, so I think it should go in a static file.
- This is common JS code for Caterpillar, so it should go in the common
Caterpillar JS file.
- It doesn't make sense to have this in caterpillar.py, since it isn't really
related to service worker generation.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:57: console.warn('Error set in lastError:', message);
On 2016/01/31 23:44:37, raymes wrote:
> Do we need to log warnings every time?

Hmm, I'm not sure. I don't think it really matters either way - do you have an
opinion on this?

[raymes, 2016-02-01 00:18:44]

Please also see my comment above about expanding the CL description :)

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:35: // in Chrome Apps.
Could you make the comment say something like this? I don't think it's
necessarily intuitive.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:36: if (!chrome.app)
Why not override it? :) I think we had a similar discussion previously about
chrome.runtime?

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:44: }
On 2016/02/01 00:14:33, Matthew Alger wrote:
> On 2016/01/31 23:44:37, raymes wrote:
> > Why did this code need to be moved?
> 
> - It's static, so I think it should go in a static file.
> - This is common JS code for Caterpillar, so it should go in the common
> Caterpillar JS file.
> - It doesn't make sense to have this in caterpillar.py, since it isn't really
> related to service worker generation.

Sounds good - this would be useful context to have in the CL description.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:49: * Sets an error message in lastError if chrome.runtime
is loaded.
nit: this is no longer accurate, nor the @throws below.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:57: console.warn('Error set in lastError:', message);
On 2016/02/01 00:14:33, Matthew Alger wrote:
> On 2016/01/31 23:44:37, raymes wrote:
> > Do we need to log warnings every time?
> 
> Hmm, I'm not sure. I don't think it really matters either way - do you have an
> opinion on this?

I think removing it is ok.

[Matthew Alger, 2016-02-01 00:22:59]

Description was changed from

==========
Stubbed runtime and app.runtime in caterpillar.js. Resolves #8.

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

to

==========
Stubbed runtime and app.runtime in caterpillar.js. Resolves #8.

Currently, caterpillar.py stubs app.runtime so that the initial
call to onLaunched for window creation won't fail. If that call
fails, then the service worker won't install, as it has a
script error. This CL moves this stubbing to caterpillar.js,
which is always a dependency. This is an improvement over the
existing location of the stub, since the stub is common code
and static, and so should not be included in a generated file.

Also, currently the runtime polyfill is required as a
dependency for caterpillar.js, since caterpillar.setError needs
to access the runtime namespace. This CL adds a runtime stub to
caterpillar.js so runtime is no longer needed as a dependency.
This resolves issue #8.

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

[Matthew Alger, 2016-02-01 00:30:37]

On 2016/02/01 00:18:44, raymes wrote:
> Please also see my comment above about expanding the CL description :)

Ah, yep, sorry. Done.

[Matthew Alger, 2016-02-01 00:36:52]

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:35: // in Chrome Apps.
On 2016/02/01 00:18:44, raymes wrote:
> Could you make the comment say something like this? I don't think it's
> necessarily intuitive.

Done.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:36: if (!chrome.app)
On 2016/02/01 00:18:44, raymes wrote:
> Why not override it? :) I think we had a similar discussion previously about
> chrome.runtime?

Hmmm, okay. Done.

(Though if a chrome.app.runtime polyfill gets written, this would need to
change)

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:44: }
On 2016/02/01 00:18:44, raymes wrote:
> On 2016/02/01 00:14:33, Matthew Alger wrote:
> > On 2016/01/31 23:44:37, raymes wrote:
> > > Why did this code need to be moved?
> > 
> > - It's static, so I think it should go in a static file.
> > - This is common JS code for Caterpillar, so it should go in the common
> > Caterpillar JS file.
> > - It doesn't make sense to have this in caterpillar.py, since it isn't
really
> > related to service worker generation.
> 
> Sounds good - this would be useful context to have in the CL description.

Done.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:49: * Sets an error message in lastError if chrome.runtime
is loaded.
On 2016/02/01 00:18:44, raymes wrote:
> nit: this is no longer accurate, nor the @throws below.

! Done.

https://codereview.chromium.org/1642833002/diff/1/src/js/caterpillar.js#newco...
src/js/caterpillar.js:57: console.warn('Error set in lastError:', message);
On 2016/02/01 00:18:44, raymes wrote:
> On 2016/02/01 00:14:33, Matthew Alger wrote:
> > On 2016/01/31 23:44:37, raymes wrote:
> > > Do we need to log warnings every time?
> > 
> > Hmm, I'm not sure. I don't think it really matters either way - do you have
an
> > opinion on this?
> 
> I think removing it is ok.

Done.

[raymes, 2016-02-01 01:51:10]

lgtm

https://codereview.chromium.org/1642833002/diff/40001/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/40001/src/js/caterpillar.js#n...
src/js/caterpillar.js:35: // All Chrome Apps call this method to launch the main
window. Stubbing it means
nit: fill 80 chars on the previous line

[Matthew Alger, 2016-02-02 22:18:26]

https://codereview.chromium.org/1642833002/diff/40001/src/js/caterpillar.js
File src/js/caterpillar.js (right):

https://codereview.chromium.org/1642833002/diff/40001/src/js/caterpillar.js#n...
src/js/caterpillar.js:35: // All Chrome Apps call this method to launch the main
window. Stubbing it means
On 2016/02/01 01:51:10, raymes wrote:
> nit: fill 80 chars on the previous line

Done.

[Matthew Alger, 2016-02-02 22:18:32]

Description was changed from

==========
Stubbed runtime and app.runtime in caterpillar.js. Resolves #8.

Currently, caterpillar.py stubs app.runtime so that the initial
call to onLaunched for window creation won't fail. If that call
fails, then the service worker won't install, as it has a
script error. This CL moves this stubbing to caterpillar.js,
which is always a dependency. This is an improvement over the
existing location of the stub, since the stub is common code
and static, and so should not be included in a generated file.

Also, currently the runtime polyfill is required as a
dependency for caterpillar.js, since caterpillar.setError needs
to access the runtime namespace. This CL adds a runtime stub to
caterpillar.js so runtime is no longer needed as a dependency.
This resolves issue #8.

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

to

==========
Stubbed runtime and app.runtime in caterpillar.js. Resolves #8.

Currently, caterpillar.py stubs app.runtime so that the initial
call to onLaunched for window creation won't fail. If that call
fails, then the service worker won't install, as it has a
script error. This CL moves this stubbing to caterpillar.js,
which is always a dependency. This is an improvement over the
existing location of the stub, since the stub is common code
and static, and so should not be included in a generated file.

Also, currently the runtime polyfill is required as a
dependency for caterpillar.js, since caterpillar.setError needs
to access the runtime namespace. This CL adds a runtime stub to
caterpillar.js so runtime is no longer needed as a dependency.
This resolves issue #8.

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

R=raymes@chromium.org

Committed: a2d90e3ad21309465a3eaf945a5da5f8c22a992c
==========

[Matthew Alger, 2016-02-02 22:18:35]

Committed patchset #4 (id:60001) manually as
a2d90e3ad21309465a3eaf945a5da5f8c22a992c (presubmit successful).