Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(145)

Issue 1535943005: Initial implementation of bindings and basic classes for worklets. (Closed)

Created:
5 years ago by ikilpatrick
Modified:
4 years, 11 months ago
Reviewers:
haraken, kinuko, Ian Vollick
CC:
chromium-reviews, kinuko+worker_chromium.org, sof, eae+blinkwatch, falken, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, blink-reviews, horo+watch_chromium.org, blink-worker-reviews_chromium.org, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Initial implementation of bindings and basic classes for worklets. https://drafts.css-houdini.org/worklets/ WorkerScriptController renamed to GlobalScopeScriptController to share code with WorkletScriptController. Additional super-class (AbstractGlobalScope) created for WorkerGlobalScope and WorkletGlobalScope to share ExecutionContext related implementation. Notable pieces left to implement: - support for Worklet::importScript - devtools integration - off-main-thread implementation for compositorworklet & audioworklet Intent to implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/kl8ct3ub3a8/-WfzBTkCAgAJ BUG=567358 Committed: https://crrev.com/eb26e4f31ee245612a4a901d7bb366606cd91f1e Cr-Commit-Position: refs/heads/master@{#369323}

Patch Set 1 #

Patch Set 2 : remove workerconsole from idl. #

Patch Set 3 : forgot to save file. #

Patch Set 4 : change toExecutionContextForModules #

Patch Set 5 : . #

Patch Set 6 : .. #

Patch Set 7 : make component build work. #

Patch Set 8 : . #

Patch Set 9 : .. #

Patch Set 10 : fix memory leak. #

Total comments: 8

Patch Set 11 : remove AbstractGlobalScope. #

Total comments: 19

Patch Set 12 : #

Patch Set 13 : removed Work*ScriptController #

Patch Set 14 : added comments. #

Total comments: 13

Patch Set 15 : . #

Total comments: 46

Patch Set 16 : rebase on top of renamed scriptcontroller #

Patch Set 17 : address (most) comments. #

Total comments: 4

Patch Set 18 : added WorkerOrWorkletGlobalScope #

Patch Set 19 : . #

Patch Set 20 : .. #

Patch Set 21 : fix windows build. #

Total comments: 12

Patch Set 22 : comments. #

Total comments: 9

Patch Set 23 : small comments. #

Patch Set 24 : rebase #

Patch Set 25 : absolute paths for DEPS file. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+382 lines, -57 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/ToV8.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ToV8.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8Binding.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 5 chunks +5 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +12 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h View 1 2 3 4 5 6 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +10 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 8 chunks +28 lines, -31 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/ModuleBindingsInitializer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.h View 1 2 3 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/scripts/v8_utilities.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ExecutionContext.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/workers/WorkerGlobalScope.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +7 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/core/workers/WorkerOrWorkletGlobalScope.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +26 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/modules.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/worklet/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +9 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/worklet/Worklet.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +34 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/worklet/Worklet.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +27 lines, -0 lines 0 comments Download
A + third_party/WebKit/Source/modules/worklet/Worklet.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +5 lines, -4 lines 0 comments Download
A third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +94 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +72 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/worklet/WorkletGlobalScope.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (11 generated)
ikilpatrick
5 years ago (2015-12-21 21:43:07 UTC) #3
haraken
I'm happy to review the implementation details if kinuko@ and vollick@ are happy with the ...
5 years ago (2015-12-21 23:34:05 UTC) #4
ikilpatrick
On 2015/12/21 23:34:05, haraken wrote: > I'm happy to review the implementation details if kinuko@ ...
5 years ago (2015-12-22 00:02:15 UTC) #5
kinuko
Thanks for sharing this! While I think this looks sane and something like this would ...
5 years ago (2015-12-22 08:39:04 UTC) #6
ikilpatrick
On 2015/12/22 08:39:04, kinuko wrote: > Thanks for sharing this! While I think this looks ...
5 years ago (2015-12-22 23:40:04 UTC) #7
ikilpatrick
https://codereview.chromium.org/1535943005/diff/180001/third_party/WebKit/Source/bindings/core/v8/GlobalScopeScriptController.h File third_party/WebKit/Source/bindings/core/v8/GlobalScopeScriptController.h (right): https://codereview.chromium.org/1535943005/diff/180001/third_party/WebKit/Source/bindings/core/v8/GlobalScopeScriptController.h#newcode125 third_party/WebKit/Source/bindings/core/v8/GlobalScopeScriptController.h:125: GlobalScopeExecutionState* m_globalScopeExecutionState; On 2015/12/22 08:39:03, kinuko wrote: > nit: ...
5 years ago (2015-12-22 23:40:23 UTC) #8
kinuko
https://codereview.chromium.org/1535943005/diff/200001/third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.cpp File third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.cpp (right): https://codereview.chromium.org/1535943005/diff/200001/third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.cpp#newcode59 third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.cpp:59: GlobalScopeScriptController* scriptController = toWorkerGlobalScope(context)->script(); Here we're checking isWorkerGlobalScope and ...
4 years, 12 months ago (2015-12-24 04:45:51 UTC) #9
kinuko
On 2015/12/22 23:40:04, ikilpatrick wrote: > Removed AbstractGlobalScope (see below), change: > GlobalScopeScriptController->IsolatedScriptController? sgtm (though ...
4 years, 12 months ago (2015-12-24 04:46:08 UTC) #10
ikilpatrick
Much simpler now, renamed to IsolatedScriptController (a better name would be nice). Dropped the other ...
4 years, 12 months ago (2015-12-27 20:22:40 UTC) #11
kinuko
Sorry for belated (year-crossing!) review. I think this now lgtm to start with. For binding ...
4 years, 11 months ago (2016-01-05 04:58:32 UTC) #12
haraken
https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.cpp File third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.cpp (left): https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.cpp#oldcode307 third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.cpp:307: visitor->trace(m_workerGlobalScope); You need to keep this code, otherwise the ...
4 years, 11 months ago (2016-01-05 06:08:06 UTC) #13
kinuko
https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h File third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h (right): https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h#newcode54 third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h:54: class CORE_EXPORT IsolatedScriptController : public NoBaseWillBeGarbageCollectedFinalized<IsolatedScriptController> { On 2016/01/05 ...
4 years, 11 months ago (2016-01-05 10:08:31 UTC) #14
haraken
https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h File third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h (right): https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h#newcode54 third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h:54: class CORE_EXPORT IsolatedScriptController : public NoBaseWillBeGarbageCollectedFinalized<IsolatedScriptController> { On 2016/01/05 ...
4 years, 11 months ago (2016-01-05 12:12:40 UTC) #15
Ian Vollick
This seems very reasonable to me, but as I'm not an expert, I defer to ...
4 years, 11 months ago (2016-01-05 14:43:00 UTC) #16
ikilpatrick
Sorry for the delay, in seattle today and tomorrow. I think I've addressed most comments, ...
4 years, 11 months ago (2016-01-07 22:47:35 UTC) #17
haraken
https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h File third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h (right): https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h#newcode106 third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h:106: RawPtrWillBeMember<ScriptWrappable> m_scriptWrappable; On 2016/01/07 22:47:35, ikilpatrick wrote: > On ...
4 years, 11 months ago (2016-01-08 04:59:08 UTC) #18
ikilpatrick
https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h File third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h (right): https://codereview.chromium.org/1535943005/diff/280001/third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h#newcode106 third_party/WebKit/Source/bindings/core/v8/IsolatedScriptController.h:106: RawPtrWillBeMember<ScriptWrappable> m_scriptWrappable; On 2016/01/08 04:59:07, haraken wrote: > On ...
4 years, 11 months ago (2016-01-11 04:13:20 UTC) #19
haraken
Thanks for being persistent on this! LGTM. https://codereview.chromium.org/1535943005/diff/400001/third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/1535943005/diff/400001/third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp#newcode719 third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:719: RELEASE_ASSERT(s_toExecutionContextForModules); This ...
4 years, 11 months ago (2016-01-12 00:38:49 UTC) #20
ikilpatrick
No problem! Thanks for the review. :) WorkerOrWorkletGlobalScope doesn't inherit from ScriptWrappable, which is why ...
4 years, 11 months ago (2016-01-12 04:11:24 UTC) #21
kinuko
still lgtm, some nits https://codereview.chromium.org/1535943005/diff/420001/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp File third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp (right): https://codereview.chromium.org/1535943005/diff/420001/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp#newcode157 third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:157: v8::Local<v8::Function> isolatedGlobalScopeConstructor = m_scriptState->perContextData()->constructorForType(wrapperTypeInfo); remnant ...
4 years, 11 months ago (2016-01-13 02:59:20 UTC) #22
ikilpatrick
Great, thanks all for the reviews! I'll submit tomorrow MTV morning. https://codereview.chromium.org/1535943005/diff/420001/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp File third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp (right): ...
4 years, 11 months ago (2016-01-13 04:12:29 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535943005/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535943005/460001
4 years, 11 months ago (2016-01-13 18:43:28 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/135083)
4 years, 11 months ago (2016-01-13 18:54:14 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535943005/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535943005/480001
4 years, 11 months ago (2016-01-13 21:40:36 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on ...
4 years, 11 months ago (2016-01-13 23:46:43 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535943005/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535943005/480001
4 years, 11 months ago (2016-01-14 02:23:35 UTC) #35
commit-bot: I haz the power
Committed patchset #25 (id:480001)
4 years, 11 months ago (2016-01-14 04:28:31 UTC) #37
commit-bot: I haz the power
4 years, 11 months ago (2016-01-14 04:29:44 UTC) #39
Message was sent while issue was closed.
Patchset 25 (id:??) landed as
https://crrev.com/eb26e4f31ee245612a4a901d7bb366606cd91f1e
Cr-Commit-Position: refs/heads/master@{#369323}

Powered by Google App Engine
This is Rietveld 408576698