Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet

Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet

This is a preparation patch for introducing module script loading for main
thread worklets. This should not change behavior.

In the current implementation, classic script loading code is tied with Worklet
class that is the common class of main thread worklets and threaded worklets.
This is an obstacle to introduce module script loading only in main thread
worklets.

This CL separates the Worklet class into MainThreadWorklet and ThreadedWorklet
as a stopgap. MainThreadWorklet will implement module script loading in
following patches and ThreadedWorklet will keep classic script loading until
module script loading becomes available for ThreadedWorklets.

BUG=627945
Review-Url: https://codereview.chromium.org/2819153003
Cr-Commit-Position: refs/heads/master@{#465151}
Committed: https://chromium.googlesource.com/chromium/src/+/0a5f98358dc00a9959a0b98a88b7e941a6c1f739

[nhiroki, 2017-04-17 03:54:44]

Description was changed from

==========
Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet

This is a preparation patch for introducing module script loading for main
thread worklets, and should not behavior.

In the current implementation, classic script loading code is tied with Worklet
class that is the common class of main thread worklets and threaded worklets.
This is an obstacle to introduce module script loading only in main thread
worklets.

This CL separates the Worklet class into MainThreadWorklet and ThreadedWorklet
as a stopgap. MainThreadWorklet will implement module script loading in
following patches and ThreadedWorklet will keep classic script loading until
module script loading becomes available for ThreadedWorklets.

BUG=627945
==========

to

==========
Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet

This is a preparation patch for introducing module script loading for main
thread worklets. This should not change behavior.

In the current implementation, classic script loading code is tied with Worklet
class that is the common class of main thread worklets and threaded worklets.
This is an obstacle to introduce module script loading only in main thread
worklets.

This CL separates the Worklet class into MainThreadWorklet and ThreadedWorklet
as a stopgap. MainThreadWorklet will implement module script loading in
following patches and ThreadedWorklet will keep classic script loading until
module script loading becomes available for ThreadedWorklets.

BUG=627945
==========

[nhiroki, 2017-04-17 03:54:52]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 03:55:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-17 04:01:00]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org, haraken@chromium.org

[nhiroki, 2017-04-17 04:01:00]

Hi, can you review this? This is a simple separation and should not change the
current behavior. Thanks!

[falken, 2017-04-17 05:35:54]

overall makes sense

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:21: // Worklet class
once threaded worklets are ready to use module loading.
In addition to the TODO, can you have a basic comment about what a main thread
worklet is? "A MainThreadWorklet is a worklet that runs only on the main
thread."

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:47:
loader_and_resolvers_;
nit: this name doesn't really sound like a map. |loader_to_resolver_map_|?

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/ThreadedWorklet.h (right):

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/ThreadedWorklet.h:22: // class once
threaded worklets are also ready to use module loading.
Similar. "A ThreadedWorklet is a worklet that possibly runs off the main
thread."

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/Worklet.h (right):

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/Worklet.h:18: 
nit: add a class-level comment?

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/Worklet.h:33: virtual
WorkletGlobalScopeProxy* GetWorkletGlobalScopeProxy() const = 0;
Can you document this interface?

[commit-bot: I haz the power, 2017-04-17 05:47:26]

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

[commit-bot: I haz the power, 2017-04-17 05:47:26]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-17 07:31:56]

MainThreadWorklet and WorkerWorklet have a bunch of duplicated code. Can we
probably consider sharing more code by moving it to Worklet?

[nhiroki, 2017-04-17 07:51:24]

Thank you for your comments. I haven't update the patcheset yet (will upload the
new one later).

On 2017/04/17 07:31:56, haraken wrote:
> MainThreadWorklet and WorkerWorklet have a bunch of duplicated code. Can we
> probably consider sharing more code by moving it to Worklet?

The duplicated code in MainThreadWorklet will be changed/removed soon in
subsequent CLs, so I'd like to keep them as they are. The purpose of this CL is
to move (copy) the duplicated code into MainThread/ThreadedWorklet so that
subsequent CLs can modify only the parts of MainThreadWorklet.

[haraken, 2017-04-17 07:55:25]

LGTM

[nhiroki, 2017-04-17 11:34:51]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2017-04-17 11:35:05]

Updated!

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right):

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:21: // Worklet class
once threaded worklets are ready to use module loading.
On 2017/04/17 05:35:54, falken wrote:
> In addition to the TODO, can you have a basic comment about what a main thread
> worklet is? "A MainThreadWorklet is a worklet that runs only on the main
> thread."

Done.

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/MainThreadWorklet.h:47:
loader_and_resolvers_;
On 2017/04/17 05:35:54, falken wrote:
> nit: this name doesn't really sound like a map. |loader_to_resolver_map_|?

Done.

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/ThreadedWorklet.h (right):

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/ThreadedWorklet.h:22: // class once
threaded worklets are also ready to use module loading.
On 2017/04/17 05:35:54, falken wrote:
> Similar. "A ThreadedWorklet is a worklet that possibly runs off the main
> thread."

Done.

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/Worklet.h (right):

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/Worklet.h:18: 
On 2017/04/17 05:35:54, falken wrote:
> nit: add a class-level comment?

Done.

https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/Worklet.h:33: virtual
WorkletGlobalScopeProxy* GetWorkletGlobalScopeProxy() const = 0;
On 2017/04/17 05:35:54, falken wrote:
> Can you document this interface?

Done.

[nhiroki, 2017-04-17 11:35:16]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 11:35:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 13:34:08]

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

[commit-bot: I haz the power, 2017-04-17 13:34:09]

Dry run: This issue passed the CQ dry run.

[falken, 2017-04-18 03:37:53]

lgtm

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedWorklet.h (right):

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedWorklet.h:19: // A
ThreadedWorklet is a worklet that runs off the main thread.
I thought threaded worklet could possibly run on the main thread, no?

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/Worklet.h (right):

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/Worklet.h:21: // Regardless of worklet
types, this must be created and destroyed on the main
"Although some worklets run off the main thread, this..."

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/Worklet.h:36: //
https://drafts.css-houdini.org/worklets/#dom-worklet-import
nit: Ah, I see. Maybe this should additionally say

// Worklet.idl
// import() imports an ES6 script module. 

It's not clear that import() is from the IDL and GetWorkletGlobalScopeProxy() is
just our own internal thing otherwise.

[nhiroki, 2017-04-18 04:05:20]

Thank you!

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedWorklet.h (right):

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedWorklet.h:19: // A
ThreadedWorklet is a worklet that runs off the main thread.
On 2017/04/18 03:37:53, falken wrote:
> I thought threaded worklet could possibly run on the main thread, no?

ThreadedWorklet is designed to always run on off-main-thread.

It would be nice if we can unify MainThreadWorklet(GlobalScope) and
ThreadedWorklet(GlobalScope) especially for AnimationWorklet that may run in
both ways, but I'm not sure if it's feasible at this point.

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/Worklet.h (right):

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/Worklet.h:21: // Regardless of worklet
types, this must be created and destroyed on the main
On 2017/04/18 03:37:53, falken wrote:
> "Although some worklets run off the main thread, this..."

Done.

https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/Worklet.h:36: //
https://drafts.css-houdini.org/worklets/#dom-worklet-import
On 2017/04/18 03:37:53, falken wrote:
> nit: Ah, I see. Maybe this should additionally say
> 
> // Worklet.idl
> // import() imports an ES6 script module. 
> 
> It's not clear that import() is from the IDL and GetWorkletGlobalScopeProxy()
is
> just our own internal thing otherwise.

I see. The original code has "// Worklet" but I thoughtlessly removed it :p
Updated the comment.

[nhiroki, 2017-04-18 04:05:25]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-04-18 04:05:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2819153003/#ps60001
(title: "update comments")

[commit-bot: I haz the power, 2017-04-18 04:05:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 05:31:24]

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

[commit-bot: I haz the power, 2017-04-18 05:31:25]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[nhiroki, 2017-04-18 05:35:14]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2017-04-18 05:35:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 06:16:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492493714073380,
"parent_rev": "4e48cbf246d80a5225c8ae709e68966b3032a2c9", "commit_rev":
"0a5f98358dc00a9959a0b98a88b7e941a6c1f739"}

[commit-bot: I haz the power, 2017-04-18 06:16:30]

Description was changed from

==========
Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet

This is a preparation patch for introducing module script loading for main
thread worklets. This should not change behavior.

In the current implementation, classic script loading code is tied with Worklet
class that is the common class of main thread worklets and threaded worklets.
This is an obstacle to introduce module script loading only in main thread
worklets.

This CL separates the Worklet class into MainThreadWorklet and ThreadedWorklet
as a stopgap. MainThreadWorklet will implement module script loading in
following patches and ThreadedWorklet will keep classic script loading until
module script loading becomes available for ThreadedWorklets.

BUG=627945
==========

to

==========
Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet

This is a preparation patch for introducing module script loading for main
thread worklets. This should not change behavior.

In the current implementation, classic script loading code is tied with Worklet
class that is the common class of main thread worklets and threaded worklets.
This is an obstacle to introduce module script loading only in main thread
worklets.

This CL separates the Worklet class into MainThreadWorklet and ThreadedWorklet
as a stopgap. MainThreadWorklet will implement module script loading in
following patches and ThreadedWorklet will keep classic script loading until
module script loading becomes available for ThreadedWorklets.

BUG=627945

Review-Url: https://codereview.chromium.org/2819153003
Cr-Commit-Position: refs/heads/master@{#465151}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a5f98358dc00a9959a0b98a88b7...
==========

[commit-bot: I haz the power, 2017-04-18 06:16:31]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0a5f98358dc00a9959a0b98a88b7...