Add a basic version of Worklet#import.

Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a 'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358
Committed: https://crrev.com/3931071f1feca0ed359ab392b1a4a48ac135e830
Cr-Commit-Position: refs/heads/master@{#376271}

[ikilpatrick, 2016-02-10 22:02:20]

Description was changed from

==========
Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of
the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a
'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the
future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

BUG=567358
==========

to

==========
Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of
the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a
'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the
future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358
==========

[ikilpatrick, 2016-02-10 22:02:20]

ikilpatrick@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-02-12 07:25:45]

Sorry for slow review, was holiday in Japan yesterday... I think this looks good
overall, I have some questions around worklet lifetime though.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/worklet/import.html (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/worklet/import.html:10:
async_test(function(t) {
nit: you could also use promise_test to save some lines

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:50:
bind(&Worklet::onFinished, this, m_scriptLoaders.last().get(), resolver));
In some error cases onFinished could run before returning promise.  Looks like
it's ok but might be good to note that in a comment.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:66:
m_workletGlobalScope->script()->evaluate(ScriptSourceCode(scriptLoader->script(),
scriptLoader->url()));
Should we catch error event here?

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:67: resolver->resolve();
Maybe check resolver->executionContext() ||
resolver->executionContext()->activeDOMObjectsAreStopped() before rejecting or
resolving if we want to extend this object's lifetime longer than the parent
document. (Please see my comments around ActiveDOMObject too)

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/worklet/Worklet.h (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.h:35: bool
hasPendingActivity() const final;
nit: this method is now moved to ScriptWrappable as of
https://codereview.chromium.org/1609343002.

A bit unsure how much implementing these are useful?  Worklet is a member of
DOMWindowWorklet so we probably don't need to worry too much about this one's
deleted, and when a parent document is detached this one could probably just
deleted (and then ongoing loaders are cancelled)?  Since worklet runs on the
same script signaling termination doesn't seem very necessary either, but I
could be missing some corner cases...

[ikilpatrick, 2016-02-12 22:59:57]

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/worklet/import.html (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/worklet/import.html:10:
async_test(function(t) {
On 2016/02/12 07:25:44, kinuko wrote:
> nit: you could also use promise_test to save some lines

Done.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:50:
bind(&Worklet::onFinished, this, m_scriptLoaders.last().get(), resolver));
On 2016/02/12 07:25:44, kinuko wrote:
> In some error cases onFinished could run before returning promise.  Looks like
> it's ok but might be good to note that in a comment.

Done.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:66:
m_workletGlobalScope->script()->evaluate(ScriptSourceCode(scriptLoader->script(),
scriptLoader->url()));
On 2016/02/12 07:25:45, kinuko wrote:
> Should we catch error event here?

Added note for the current thinking at the moment. Basically if a script has a
SyntaxError - reject, otherwise resolve (even if a script error was thrown).

The reason for this is that a script may have successfully registered classes in
the worklet (which could be used) even if a script throws.

To support this behaviour will need to change WorkerOrWorkletScriptController a
little to support passing back an error code. So will do it in a follow up.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:67: resolver->resolve();
On 2016/02/12 07:25:45, kinuko wrote:
> Maybe check resolver->executionContext() ||
> resolver->executionContext()->activeDOMObjectsAreStopped() before rejecting or
> resolving if we want to extend this object's lifetime longer than the parent
> document. (Please see my comments around ActiveDOMObject too)

Added guard for the moment.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/worklet/Worklet.h (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.h:35: bool
hasPendingActivity() const final;
On 2016/02/12 07:25:45, kinuko wrote:
> nit: this method is now moved to ScriptWrappable as of
> https://codereview.chromium.org/1609343002.
> 
> A bit unsure how much implementing these are useful?  Worklet is a member of
> DOMWindowWorklet so we probably don't need to worry too much about this one's
> deleted, and when a parent document is detached this one could probably just
> deleted (and then ongoing loaders are cancelled)?  Since worklet runs on the
> same script signaling termination doesn't seem very necessary either, but I
> could be missing some corner cases...

I'm unsure as well. I had a hunch that this wasn't required, but wasn't sure.

Who would be a good person to check with?

[kinuko, 2016-02-15 04:58:27]

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/worklet/Worklet.h (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.h:35: bool
hasPendingActivity() const final;
On 2016/02/12 22:59:57, ikilpatrick wrote:
> On 2016/02/12 07:25:45, kinuko wrote:
> > nit: this method is now moved to ScriptWrappable as of
> > https://codereview.chromium.org/1609343002.
> > 
> > A bit unsure how much implementing these are useful?  Worklet is a member of
> > DOMWindowWorklet so we probably don't need to worry too much about this
one's
> > deleted, and when a parent document is detached this one could probably just
> > deleted (and then ongoing loaders are cancelled)?  Since worklet runs on the
> > same script signaling termination doesn't seem very necessary either, but I
> > could be missing some corner cases...
> 
> I'm unsure as well. I had a hunch that this wasn't required, but wasn't sure.
> 
> Who would be a good person to check with?

I _think_ we should be fine without implementing hasPendingActivity. We could
explicitly keep the wrapper alive this way to avoid to have it gc'ed and
recreated but we don't seem doing this usually for window members.

cc-ing haraken@ to check with...

https://codereview.chromium.org/1684303002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/worklet/import.html (right):

https://codereview.chromium.org/1684303002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/worklet/import.html:11: return
renderWorklet.import('resources/empty-worklet-script.js').then(function(undefined_arg)
{
fyi, for promise chaining we used to use following style in SW code which has
been working well, might be helpful as well. (Up to you)

https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

[haraken, 2016-02-15 05:16:44]

On 2016/02/15 04:58:27, kinuko wrote:
>
https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/worklet/Worklet.h (right):
> 
>
https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/worklet/Worklet.h:35: bool
> hasPendingActivity() const final;
> On 2016/02/12 22:59:57, ikilpatrick wrote:
> > On 2016/02/12 07:25:45, kinuko wrote:
> > > nit: this method is now moved to ScriptWrappable as of
> > > https://codereview.chromium.org/1609343002.
> > > 
> > > A bit unsure how much implementing these are useful?  Worklet is a member
of
> > > DOMWindowWorklet so we probably don't need to worry too much about this
> one's
> > > deleted, and when a parent document is detached this one could probably
just
> > > deleted (and then ongoing loaders are cancelled)?  Since worklet runs on
the
> > > same script signaling termination doesn't seem very necessary either, but
I
> > > could be missing some corner cases...
> > 
> > I'm unsure as well. I had a hunch that this wasn't required, but wasn't
sure.
> > 
> > Who would be a good person to check with?
> 
> I _think_ we should be fine without implementing hasPendingActivity. We could
> explicitly keep the wrapper alive this way to avoid to have it gc'ed and
> recreated but we don't seem doing this usually for window members.
> 
> cc-ing haraken@ to check with...

If the wrapper gets gced and then recreated, expandos on the wrapper will be
lost. In common cases this is just fine because losing expandos on the wrapper
that is already unreachable on the JS side shouldn't be a problem. It becomes
problematic only when the wrapper is unreachable on the JS side but reachable on
the DOM side (e.g., XHR's wrapper must be kept alive and thus the expandos on
the XHR's wrapper must be alive while the XHR is loading something).
hasPendingActivity is a way to say to V8, "even if the wrapper is unreachable on
your side, please don't collect it and its expandos -- I'm doing something
important right now".

If Worklet doesn't involve such a DOM-side reachability issue,
hasPendingActivity wouldn't be needed.

[ikilpatrick, 2016-02-16 19:34:14]

Thanks :)

Lifetimes are making more sense now. Just a few more questions.

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/worklet/Worklet.h (right):

https://codereview.chromium.org/1684303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/worklet/Worklet.h:35: bool
hasPendingActivity() const final;
On 2016/02/15 04:58:27, kinuko wrote:
> On 2016/02/12 22:59:57, ikilpatrick wrote:
> > On 2016/02/12 07:25:45, kinuko wrote:
> > > nit: this method is now moved to ScriptWrappable as of
> > > https://codereview.chromium.org/1609343002.
> > > 
> > > A bit unsure how much implementing these are useful?  Worklet is a member
of
> > > DOMWindowWorklet so we probably don't need to worry too much about this
> one's
> > > deleted, and when a parent document is detached this one could probably
just
> > > deleted (and then ongoing loaders are cancelled)?  Since worklet runs on
the
> > > same script signaling termination doesn't seem very necessary either, but
I
> > > could be missing some corner cases...
> > 
> > I'm unsure as well. I had a hunch that this wasn't required, but wasn't
sure.
> > 
> > Who would be a good person to check with?
> 
> I _think_ we should be fine without implementing hasPendingActivity. We could
> explicitly keep the wrapper alive this way to avoid to have it gc'ed and
> recreated but we don't seem doing this usually for window members.
> 
> cc-ing haraken@ to check with...

Makes sense, removed hasPendingActivity.

I can theoretically also remove stop() as the WorkerScriptLoader destructor will
cancel any pending requests?

https://codereview.chromium.org/1684303002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/worklet/import.html (right):

https://codereview.chromium.org/1684303002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/worklet/import.html:11: return
renderWorklet.import('resources/empty-worklet-script.js').then(function(undefined_arg)
{
On 2016/02/15 04:58:27, kinuko wrote:
> fyi, for promise chaining we used to use following style in SW code which has
> been working well, might be helpful as well. (Up to you)
> 
> https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

Done. (I think).

https://codereview.chromium.org/1684303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/1684303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:66: if
(resolver->executionContext() ||
resolver->executionContext()->activeDOMObjectsAreStopped()) {
This guard this guard still required now?

[kinuko, 2016-02-17 05:14:32]

lgtm

https://codereview.chromium.org/1684303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/1684303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:66: if
(resolver->executionContext() ||
resolver->executionContext()->activeDOMObjectsAreStopped()) {
On 2016/02/16 19:34:14, ikilpatrick wrote:
> This guard this guard still required now?

As we cancel the loading in stop() I think we're fine not doing this..
(otherwise we'll know by crashes :))

[ikilpatrick, 2016-02-17 22:41:43]

https://codereview.chromium.org/1684303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/1684303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:66: if
(resolver->executionContext() ||
resolver->executionContext()->activeDOMObjectsAreStopped()) {
On 2016/02/17 05:14:32, kinuko wrote:
> On 2016/02/16 19:34:14, ikilpatrick wrote:
> > This guard this guard still required now?
> 
> As we cancel the loading in stop() I think we're fine not doing this..
> (otherwise we'll know by crashes :))

Removed guard.

[ikilpatrick, 2016-02-17 23:50:36]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-02-17 23:50:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1684303002/#ps80001
(title: "remove guard.")

[commit-bot: I haz the power, 2016-02-17 23:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684303002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684303002/80001

[commit-bot: I haz the power, 2016-02-18 00:06:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 00:06:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ikilpatrick, 2016-02-18 00:41:05]

ikilpatrick@chromium.org changed reviewers:
+ leviw@chromium.org

[ikilpatrick, 2016-02-18 00:41:06]

+levi for RuntimeEnabledFeatures.in OWNERS :)

[leviw_travelin_and_unemployed, 2016-02-18 02:36:25]

lgtm

[ikilpatrick, 2016-02-18 18:53:01]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-02-18 18:53:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1684303002/#ps120001
(title: "Fix non-oilpan build.")

[commit-bot: I haz the power, 2016-02-18 18:57:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684303002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684303002/120001

[commit-bot: I haz the power, 2016-02-18 21:41:31]

Description was changed from

==========
Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of
the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a
'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the
future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358
==========

to

==========
Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of
the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a
'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the
future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358
==========

[commit-bot: I haz the power, 2016-02-18 21:41:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-18 21:42:39]

Description was changed from

==========
Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of
the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a
'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the
future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358
==========

to

==========
Add a basic version of Worklet#import.

Worklets are now spec'd to use the 'module' loading infrastructure instead of
the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a
'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the
future,
(see: https://github.com/whatwg/html/pull/608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358

Committed: https://crrev.com/3931071f1feca0ed359ab392b1a4a48ac135e830
Cr-Commit-Position: refs/heads/master@{#376271}
==========

[commit-bot: I haz the power, 2016-02-18 21:42:40]

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