Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854003002:
Worklet: Make the latter half of addModule() async (Closed)
|
Created:
3 years, 7 months ago by nhiroki Modified:
3 years, 7 months ago CC:
chromium-reviews, shimazu+worker_chromium.org, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorklet: Make the latter half of addModule() async
The Worklet spec requires the step 7 and following steps of the addModule()
algorithm run in parallel.
Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
BUG=627945
Review-Url: https://codereview.chromium.org/2854003002
Cr-Commit-Position: refs/heads/master@{#469918}
Committed: https://chromium.googlesource.com/chromium/src/+/ec1e50e27b3c97347a595e5da762115574111dd1
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #Patch Set 3 : update test expectations #
Total comments: 8
Patch Set 4 : address review comments #
Total comments: 2
Patch Set 5 : change TaskType #Patch Set 6 : WrapWeakPersistent -> WrapPersistent #
Messages
Total messages: 42 (26 generated)
Description was changed from ========== Worker: Make the latter half of addModule() async The Worklet spec requires the step 7 and following steps of the addModule() algorithm run in parallel. Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule BUG=627945 ========== to ========== Worklet: Make the latter half of addModule() async The Worklet spec requires the step 7 and following steps of the addModule() algorithm run in parallel. Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule BUG=627945 ==========
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nhiroki@chromium.org changed reviewers: + falken@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:56: TaskRunnerHelper::Get(TaskType::kUnspecedTimer, script_state) I'm not too familiar with the TaskType. Is there a reason to prefer UnspecedTimer over UnspecedLoading or some other type? https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:60: WrapPersistent(resolver))); What happens if |this| gets destroyed before the task runs? Is |promise| never resolved or rejected? https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:64: // Implementation of the first second of the "addModule(moduleURL, options)" "second half"
Patchset #2 (id:20001) has been deleted
nhiroki@chromium.org changed reviewers: + tzik@chromium.org
+tzik@ and haraken@ for TaskType (can you see my reply comment in MainThreadWorklet.cpp?) falken@, thank you for reviewing this. Updated. https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:56: TaskRunnerHelper::Get(TaskType::kUnspecedTimer, script_state) On 2017/05/08 00:54:42, falken wrote: > I'm not too familiar with the TaskType. Is there a reason to prefer > UnspecedTimer over UnspecedLoading or some other type? Hmmm... that's a tough question. I felt this was an internal non-loading task (i.e., UnspecedTimer) because (1) the spec doesn't define the task source, and (2) this task itself is not a loading task (it just makes the procedure asynchronous). Now I'm confused about how to choose it... kMiscPlatformAPI (because this is a part of the spec)? kUnspecedTimer (because this is an internal task just to make the procedure asynchronous)?? kUnspecedLoading (because this is a part of module loading)??? +tzik@, +haraken@: any thoughts? https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:60: WrapPersistent(resolver))); On 2017/05/08 00:54:41, falken wrote: > What happens if |this| gets destroyed before the task runs? Is |promise| never > resolved or rejected? |this| is destroyed only when the document is destroyed and |resolver| is also detached via ScriptPromiseResolver::ContextDestroyed(). That is, |promise| is never settled and does not have to be settled. https://codereview.chromium.org/2854003002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:64: // Implementation of the first second of the "addModule(moduleURL, options)" On 2017/05/08 00:54:42, falken wrote: > "second half" Done.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nhiroki@chromium.org changed reviewers: + haraken@chromium.org
On 2017/05/08 02:32:05, nhiroki (ooo May 3-7) wrote: > +tzik@ and haraken@ for TaskType (can you see my reply comment in > MainThreadWorklet.cpp?) +haraken@ (I forgot to update the reviewer list)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:56: TaskRunnerHelper::Get(TaskType::kUnspecedTimer, script_state) kUnspecedLoading? https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.h:16: class ScriptPromise; Can we keep ScriptPromise a complete type, as we use it as a pass-by-valued return value?
https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:59: WrapWeakPersistent(this), module_url_record, Why weak?
The test expectation change looks like we're losing information, which I guess is just the trade-off for making this async? Is there a more explicit test possible for this spec conformance change? I guess it's difficult because GetWorkletGlobalScopeProxy()->FetchAndInvokeScript is already async.
On 2017/05/08 04:39:33, falken wrote: > The test expectation change looks like we're losing information, which I guess > is just the trade-off for making this async? I think so. > Is there a more explicit test possible for this spec conformance change? I guess > it's difficult because GetWorkletGlobalScopeProxy()->FetchAndInvokeScript is > already async. We could check it in wpt/layout tests if there is an API to access the state of a returned promise (pending/rejected/resolved), but AFAICS it doesn't exist.
Thank you for your comments. https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:56: TaskRunnerHelper::Get(TaskType::kUnspecedTimer, script_state) On 2017/05/08 04:23:46, tzik wrote: > kUnspecedLoading? OK, changed. Also added a comment about why we chose the type. https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:59: WrapWeakPersistent(this), module_url_record, On 2017/05/08 04:27:14, haraken wrote: > > Why weak? Because it's meaningless to run the following sequence after Document that owns |this| is destroyed. Is it preferable to check GetExecutionContext() at the beginning of FetchAndInvokeScript() instead of making this weak like line 29? https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.h:16: class ScriptPromise; On 2017/05/08 04:23:46, tzik wrote: > Can we keep ScriptPromise a complete type, as we use it as a pass-by-valued > return value? Done.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm if others' comments are resolved https://codereview.chromium.org/2854003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:58: TaskRunnerHelper::Get(TaskType::kUnspecedTimer, script_state) I think you meant to change the comment and the task type.
https://codereview.chromium.org/2854003002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:58: TaskRunnerHelper::Get(TaskType::kUnspecedTimer, script_state) On 2017/05/08 05:10:52, falken wrote: > I think you meant to change the comment and the task type. Oh... thanks!
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:59: WrapWeakPersistent(this), module_url_record, On 2017/05/08 05:05:46, nhiroki (maybe slow) wrote: > On 2017/05/08 04:27:14, haraken wrote: > > > > Why weak? > > Because it's meaningless to run the following sequence after Document that owns > |this| is destroyed. > > Is it preferable to check GetExecutionContext() at the beginning of > FetchAndInvokeScript() instead of making this weak like line 29? Yes. Note that the weak persistent is cleared when the Document gets destructed (by Oilpan's GC), which happens later than when the Document gets destroyed. You need to explicitly check the validity of the Document.
Thank you. Updated. https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp (right): https://codereview.chromium.org/2854003002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp:59: WrapWeakPersistent(this), module_url_record, On 2017/05/08 05:17:03, haraken wrote: > On 2017/05/08 05:05:46, nhiroki (maybe slow) wrote: > > On 2017/05/08 04:27:14, haraken wrote: > > > > > > Why weak? > > > > Because it's meaningless to run the following sequence after Document that > owns > > |this| is destroyed. > > > > Is it preferable to check GetExecutionContext() at the beginning of > > FetchAndInvokeScript() instead of making this weak like line 29? > > Yes. > > Note that the weak persistent is cleared when the Document gets destructed (by > Oilpan's GC), which happens later than when the Document gets destroyed. You > need to explicitly check the validity of the Document. I see. Thank you for the clarification!
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by nhiroki@chromium.org
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2854003002/#ps120001 (title: "WrapWeakPersistent -> WrapPersistent")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494221383077830,
"parent_rev": "325fa4c7d89214f89c3ff66432276130aeb414fb", "commit_rev":
"ec1e50e27b3c97347a595e5da762115574111dd1"}
Message was sent while issue was closed.
Description was changed from ========== Worklet: Make the latter half of addModule() async The Worklet spec requires the step 7 and following steps of the addModule() algorithm run in parallel. Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule BUG=627945 ========== to ========== Worklet: Make the latter half of addModule() async The Worklet spec requires the step 7 and following steps of the addModule() algorithm run in parallel. Spec: https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule BUG=627945 Review-Url: https://codereview.chromium.org/2854003002 Cr-Commit-Position: refs/heads/master@{#469918} Committed: https://chromium.googlesource.com/chromium/src/+/ec1e50e27b3c97347a595e5da762... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ec1e50e27b3c97347a595e5da762... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
