Added dependency script injection and fixed other script injection.

Added dependency script injection and fixed other script injection.

This CL makes script injection more clear by splitting up the
required script list into multiple lists that are added together when
necessary. It also adds dependency script injection for dependencies
that have been installed with npm or bower.

R=mgiuca@chromium.org

Committed: 2b09d2efbd381700bfc26b7fe2b641283facaaa9

[Matthew Alger, 2016-01-20 04:10:12]

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

[Matt Giuca, 2016-01-21 00:23:40]

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

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:572: if 'runtime' not in polyfillable:
why?

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:589: required_always_paths = [
Why the name change?

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:635: + required_polyfill_paths)  # Order significant here.
nit: Don't squeeze this comment here; put it above.

[Matthew Alger, 2016-01-21 00:29:30]

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

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:572: if 'runtime' not in polyfillable:
On 2016/01/21 00:23:40, Matt Giuca wrote:
> why?

I don't want to add it twice, and polyfillable is a list.

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:589: required_always_paths = [
On 2016/01/21 00:23:40, Matt Giuca wrote:
> Why the name change?

Added three different lists with a similar name - calling this required_js_paths
would be misleading, since it's not all the JS. It's just the files that are
always required.

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:635: + required_polyfill_paths)  # Order significant here.
On 2016/01/21 00:23:40, Matt Giuca wrote:
> nit: Don't squeeze this comment here; put it above.

Done.

[Matt Giuca, 2016-01-27 00:35:25]

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

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:572: if 'runtime' not in polyfillable:
On 2016/01/21 00:29:30, Matthew Alger wrote:
> On 2016/01/21 00:23:40, Matt Giuca wrote:
> > why?
> 
> I don't want to add it twice, and polyfillable is a list.

I mean why do you special-case runtime here? (Why does it need to be added even
if it isn't used?)

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:589: required_always_paths = [
On 2016/01/21 00:29:30, Matthew Alger wrote:
> On 2016/01/21 00:23:40, Matt Giuca wrote:
> > Why the name change?
> 
> Added three different lists with a similar name - calling this
required_js_paths
> would be misleading, since it's not all the JS. It's just the files that are
> always required.

Acknowledged.

[Matthew Alger, 2016-01-27 00:36:39]

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

https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
src/caterpillar.py:572: if 'runtime' not in polyfillable:
On 2016/01/27 00:35:25, Matt Giuca wrote:
> On 2016/01/21 00:29:30, Matthew Alger wrote:
> > On 2016/01/21 00:23:40, Matt Giuca wrote:
> > > why?
> > 
> > I don't want to add it twice, and polyfillable is a list.
> 
> I mean why do you special-case runtime here? (Why does it need to be added
even
> if it isn't used?)

All Chrome Apps errors are stored in chrome.runtime.lastError.

Perhaps a better solution would be to stub chrome.runtime in chrome.caterpillar,
so the special case isn't needed?

[Matt Giuca, 2016-01-27 00:44:21]

On 2016/01/27 00:36:39, Matthew Alger wrote:
> https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py
> File src/caterpillar.py (right):
> 
>
https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
> src/caterpillar.py:572: if 'runtime' not in polyfillable:
> On 2016/01/27 00:35:25, Matt Giuca wrote:
> > On 2016/01/21 00:29:30, Matthew Alger wrote:
> > > On 2016/01/21 00:23:40, Matt Giuca wrote:
> > > > why?
> > > 
> > > I don't want to add it twice, and polyfillable is a list.
> > 
> > I mean why do you special-case runtime here? (Why does it need to be added
> even
> > if it isn't used?)
> 
> All Chrome Apps errors are stored in chrome.runtime.lastError.
> 
> Perhaps a better solution would be to stub chrome.runtime in
chrome.caterpillar,
> so the special case isn't needed?

Can you explain what you mean by that? Does caterpillar need access to
chrome.runtime.lastError? What would stubbing chrome.runtime in
chrome.caterpillar entail?

[Matthew Alger, 2016-01-27 00:47:24]

On 2016/01/27 00:44:21, Matt Giuca wrote:
> On 2016/01/27 00:36:39, Matthew Alger wrote:
> > https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py
> > File src/caterpillar.py (right):
> > 
> >
>
https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
> > src/caterpillar.py:572: if 'runtime' not in polyfillable:
> > On 2016/01/27 00:35:25, Matt Giuca wrote:
> > > On 2016/01/21 00:29:30, Matthew Alger wrote:
> > > > On 2016/01/21 00:23:40, Matt Giuca wrote:
> > > > > why?
> > > > 
> > > > I don't want to add it twice, and polyfillable is a list.
> > > 
> > > I mean why do you special-case runtime here? (Why does it need to be added
> > even
> > > if it isn't used?)
> > 
> > All Chrome Apps errors are stored in chrome.runtime.lastError.
> > 
> > Perhaps a better solution would be to stub chrome.runtime in
> chrome.caterpillar,
> > so the special case isn't needed?
> 
> Can you explain what you mean by that? Does caterpillar need access to
> chrome.runtime.lastError? What would stubbing chrome.runtime in
> chrome.caterpillar entail?

Every API stores all errors it encounters in chrome.runtime.lastError. This
means at minimum that every API needs to be able to access the chrome.runtime
object and set its lastError property. Since the error objects aren't just
strings, there's a function chrome.caterpillar.setError that takes a string and
sets the error. So chrome.caterpillar needs at very least "if (!chrome.runtime)
chrome.runtime = {}" somewhere.

[Matt Giuca, 2016-01-27 01:44:14]

On 2016/01/27 00:47:24, Matthew Alger wrote:
> On 2016/01/27 00:44:21, Matt Giuca wrote:
> > On 2016/01/27 00:36:39, Matthew Alger wrote:
> > > https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py
> > > File src/caterpillar.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
> > > src/caterpillar.py:572: if 'runtime' not in polyfillable:
> > > On 2016/01/27 00:35:25, Matt Giuca wrote:
> > > > On 2016/01/21 00:29:30, Matthew Alger wrote:
> > > > > On 2016/01/21 00:23:40, Matt Giuca wrote:
> > > > > > why?
> > > > > 
> > > > > I don't want to add it twice, and polyfillable is a list.
> > > > 
> > > > I mean why do you special-case runtime here? (Why does it need to be
added
> > > even
> > > > if it isn't used?)
> > > 
> > > All Chrome Apps errors are stored in chrome.runtime.lastError.
> > > 
> > > Perhaps a better solution would be to stub chrome.runtime in
> > chrome.caterpillar,
> > > so the special case isn't needed?
> > 
> > Can you explain what you mean by that? Does caterpillar need access to
> > chrome.runtime.lastError? What would stubbing chrome.runtime in
> > chrome.caterpillar entail?
> 
> Every API stores all errors it encounters in chrome.runtime.lastError. This
> means at minimum that every API needs to be able to access the chrome.runtime
> object and set its lastError property. Since the error objects aren't just
> strings, there's a function chrome.caterpillar.setError that takes a string
and
> sets the error. So chrome.caterpillar needs at very least "if
(!chrome.runtime)
> chrome.runtime = {}" somewhere.

I see, so you need the runtime polyfill to be included regardless of whether it
is used by the user's code, because your other polyfills need to write into it.

In that case, lgtm but please add a comment explaining the above sentence.

[Matthew Alger, 2016-01-27 01:50:53]

On 2016/01/27 01:44:14, Matt Giuca wrote:
> On 2016/01/27 00:47:24, Matthew Alger wrote:
> > On 2016/01/27 00:44:21, Matt Giuca wrote:
> > > On 2016/01/27 00:36:39, Matthew Alger wrote:
> > > > https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py
> > > > File src/caterpillar.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1604873003/diff/60001/src/caterpillar.py#newc...
> > > > src/caterpillar.py:572: if 'runtime' not in polyfillable:
> > > > On 2016/01/27 00:35:25, Matt Giuca wrote:
> > > > > On 2016/01/21 00:29:30, Matthew Alger wrote:
> > > > > > On 2016/01/21 00:23:40, Matt Giuca wrote:
> > > > > > > why?
> > > > > > 
> > > > > > I don't want to add it twice, and polyfillable is a list.
> > > > > 
> > > > > I mean why do you special-case runtime here? (Why does it need to be
> added
> > > > even
> > > > > if it isn't used?)
> > > > 
> > > > All Chrome Apps errors are stored in chrome.runtime.lastError.
> > > > 
> > > > Perhaps a better solution would be to stub chrome.runtime in
> > > chrome.caterpillar,
> > > > so the special case isn't needed?
> > > 
> > > Can you explain what you mean by that? Does caterpillar need access to
> > > chrome.runtime.lastError? What would stubbing chrome.runtime in
> > > chrome.caterpillar entail?
> > 
> > Every API stores all errors it encounters in chrome.runtime.lastError. This
> > means at minimum that every API needs to be able to access the
chrome.runtime
> > object and set its lastError property. Since the error objects aren't just
> > strings, there's a function chrome.caterpillar.setError that takes a string
> and
> > sets the error. So chrome.caterpillar needs at very least "if
> (!chrome.runtime)
> > chrome.runtime = {}" somewhere.
> 
> I see, so you need the runtime polyfill to be included regardless of whether
it
> is used by the user's code, because your other polyfills need to write into
it.
> 
> In that case, lgtm but please add a comment explaining the above sentence.

The more I think about it, the less it seems a good idea to handle this by
including runtime as a polyfill dependency. This means that platform needs to be
included as a dependency and the generated report will be less clear (as it will
say that the code depends on runtime when it really does not). I think it would
be better to revert the runtime dependency addition, and stub it in
caterpillar.js instead. Can I add that to this CL?

[Matthew Alger, 2016-01-28 04:31:54]

Reverted runtime dependency. I'll add a stub in another CL.

[Matthew Alger, 2016-01-28 04:32:05]

Description was changed from

==========
Added dependency script injection and fixed other script injection.

This CL makes script injection more clear by splitting up the
required script list into multiple lists that are added together when
necessary. It also adds dependency script injection for dependencies
that have been installed with npm or bower, and ensures that runtime is
always considered a dependency.
==========

to

==========
Added dependency script injection and fixed other script injection.

This CL makes script injection more clear by splitting up the
required script list into multiple lists that are added together when
necessary. It also adds dependency script injection for dependencies
that have been installed with npm or bower.
==========

[Matt Giuca, 2016-01-28 06:58:02]

That's a valid argument. slgtm.

(That means... "still lgtm").

[Matthew Alger, 2016-01-28 22:31:28]

Description was changed from

==========
Added dependency script injection and fixed other script injection.

This CL makes script injection more clear by splitting up the
required script list into multiple lists that are added together when
necessary. It also adds dependency script injection for dependencies
that have been installed with npm or bower.
==========

to

==========
Added dependency script injection and fixed other script injection.

This CL makes script injection more clear by splitting up the
required script list into multiple lists that are added together when
necessary. It also adds dependency script injection for dependencies
that have been installed with npm or bower.

R=mgiuca@chromium.org

Committed: 2b09d2efbd381700bfc26b7fe2b641283facaaa9
==========

[Matthew Alger, 2016-01-28 22:31:30]

Committed patchset #7 (id:120001) manually as
2b09d2efbd381700bfc26b7fe2b641283facaaa9 (presubmit successful).