Summary: | Update Worklet.addModule() to actually fetch and evaluate the module script | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | Web Audio | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | annulen, beidson, cdumez, darin, eric.carlson, ews-watchlist, ggaren, glenn, gyuyoung.kim, japhet, jer.noble, mkwst, peng.liu6, philipj, ryuan.choi, sam, sergio, toyoshim, webkit-bug-importer, youennf, yutak | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 217194 | ||||||||||||||||
Bug Blocks: | 217058, 217251 | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2020-10-02 12:08:22 PDT
Created attachment 410357 [details]
Patch
Created attachment 410360 [details]
Patch
Created attachment 410365 [details]
Patch
Created attachment 410392 [details]
Patch
Comment on attachment 410392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410392&action=review > Source/WebCore/loader/ThreadableLoader.cpp:115 > + if (is<WorkerGlobalScope>(context) || (is<WorkletGlobalScope>(context) && downcast<WorkletGlobalScope>(context).workerOrWorkletThread())) > + return WorkerThreadableLoader::create(static_cast<WorkerOrWorkletGlobalScope&>(context), client, WorkerRunLoop::defaultMode(), WTFMove(request), options, WTFMove(referrer)); > + > + Document* document = nullptr; > + if (is<WorkletGlobalScope>(context)) > + document = downcast<WorkletGlobalScope>(context).responsibleDocument(); > + else > + document = &downcast<Document>(context); > > - return DocumentThreadableLoader::create(downcast<Document>(context), client, WTFMove(request), options, WTFMove(referrer)); > + return DocumentThreadableLoader::create(*document, client, WTFMove(request), options, WTFMove(referrer)); I don't quite understand this logic. When do you have a WorkletGlobalScope but not a workerOrWorkletThread? (In reply to Sam Weinig from comment #5) > Comment on attachment 410392 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410392&action=review > > > Source/WebCore/loader/ThreadableLoader.cpp:115 > > + if (is<WorkerGlobalScope>(context) || (is<WorkletGlobalScope>(context) && downcast<WorkletGlobalScope>(context).workerOrWorkletThread())) > > + return WorkerThreadableLoader::create(static_cast<WorkerOrWorkletGlobalScope&>(context), client, WorkerRunLoop::defaultMode(), WTFMove(request), options, WTFMove(referrer)); > > + > > + Document* document = nullptr; > > + if (is<WorkletGlobalScope>(context)) > > + document = downcast<WorkletGlobalScope>(context).responsibleDocument(); > > + else > > + document = &downcast<Document>(context); > > > > - return DocumentThreadableLoader::create(downcast<Document>(context), client, WTFMove(request), options, WTFMove(referrer)); > > + return DocumentThreadableLoader::create(*document, client, WTFMove(request), options, WTFMove(referrer)); > > I don't quite understand this logic. When do you have a WorkletGlobalScope > but not a workerOrWorkletThread? A Worklet does not necessarily use threading. PaintWorklets do not use threading, AudioWorklets do. (In reply to Chris Dumez from comment #6) > (In reply to Sam Weinig from comment #5) > > Comment on attachment 410392 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=410392&action=review > > > > > Source/WebCore/loader/ThreadableLoader.cpp:115 > > > + if (is<WorkerGlobalScope>(context) || (is<WorkletGlobalScope>(context) && downcast<WorkletGlobalScope>(context).workerOrWorkletThread())) > > > + return WorkerThreadableLoader::create(static_cast<WorkerOrWorkletGlobalScope&>(context), client, WorkerRunLoop::defaultMode(), WTFMove(request), options, WTFMove(referrer)); > > > + > > > + Document* document = nullptr; > > > + if (is<WorkletGlobalScope>(context)) > > > + document = downcast<WorkletGlobalScope>(context).responsibleDocument(); > > > + else > > > + document = &downcast<Document>(context); > > > > > > - return DocumentThreadableLoader::create(downcast<Document>(context), client, WTFMove(request), options, WTFMove(referrer)); > > > + return DocumentThreadableLoader::create(*document, client, WTFMove(request), options, WTFMove(referrer)); > > > > I don't quite understand this logic. When do you have a WorkletGlobalScope > > but not a workerOrWorkletThread? > > A Worklet does not necessarily use threading. PaintWorklets do not use > threading, AudioWorklets do. Oh, that's not confusing at all! (In reply to Sam Weinig from comment #7) > (In reply to Chris Dumez from comment #6) > > (In reply to Sam Weinig from comment #5) > > > Comment on attachment 410392 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=410392&action=review > > > > > > > Source/WebCore/loader/ThreadableLoader.cpp:115 > > > > + if (is<WorkerGlobalScope>(context) || (is<WorkletGlobalScope>(context) && downcast<WorkletGlobalScope>(context).workerOrWorkletThread())) > > > > + return WorkerThreadableLoader::create(static_cast<WorkerOrWorkletGlobalScope&>(context), client, WorkerRunLoop::defaultMode(), WTFMove(request), options, WTFMove(referrer)); > > > > + > > > > + Document* document = nullptr; > > > > + if (is<WorkletGlobalScope>(context)) > > > > + document = downcast<WorkletGlobalScope>(context).responsibleDocument(); > > > > + else > > > > + document = &downcast<Document>(context); > > > > > > > > - return DocumentThreadableLoader::create(downcast<Document>(context), client, WTFMove(request), options, WTFMove(referrer)); > > > > + return DocumentThreadableLoader::create(*document, client, WTFMove(request), options, WTFMove(referrer)); > > > > > > I don't quite understand this logic. When do you have a WorkletGlobalScope > > > but not a workerOrWorkletThread? > > > > A Worklet does not necessarily use threading. PaintWorklets do not use > > threading, AudioWorklets do. > > Oh, that's not confusing at all! :) Per spec, Workets are completely thread-agnostic [1] and our current implementation of paint Worklets does not use threading at all. I am trying to add support for threading for AudioWorklets without breaking support for using the main thread so that I don't break our paint Worklets. [1] https://drafts.css-houdini.org/worklets/#motivations (In reply to Chris Dumez from comment #8) > (In reply to Sam Weinig from comment #7) > > (In reply to Chris Dumez from comment #6) > > > (In reply to Sam Weinig from comment #5) > > > > Comment on attachment 410392 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=410392&action=review > > > > > > > > > Source/WebCore/loader/ThreadableLoader.cpp:115 > > > > > + if (is<WorkerGlobalScope>(context) || (is<WorkletGlobalScope>(context) && downcast<WorkletGlobalScope>(context).workerOrWorkletThread())) > > > > > + return WorkerThreadableLoader::create(static_cast<WorkerOrWorkletGlobalScope&>(context), client, WorkerRunLoop::defaultMode(), WTFMove(request), options, WTFMove(referrer)); > > > > > + > > > > > + Document* document = nullptr; > > > > > + if (is<WorkletGlobalScope>(context)) > > > > > + document = downcast<WorkletGlobalScope>(context).responsibleDocument(); > > > > > + else > > > > > + document = &downcast<Document>(context); > > > > > > > > > > - return DocumentThreadableLoader::create(downcast<Document>(context), client, WTFMove(request), options, WTFMove(referrer)); > > > > > + return DocumentThreadableLoader::create(*document, client, WTFMove(request), options, WTFMove(referrer)); > > > > > > > > I don't quite understand this logic. When do you have a WorkletGlobalScope > > > > but not a workerOrWorkletThread? > > > > > > A Worklet does not necessarily use threading. PaintWorklets do not use > > > threading, AudioWorklets do. > > > > Oh, that's not confusing at all! > > :) Per spec, Workets are completely thread-agnostic [1] and our current > implementation of paint Worklets does not use threading at all. I am trying > to add support for threading for AudioWorklets without breaking support for > using the main thread so that I don't break our paint Worklets. > > [1] https://drafts.css-houdini.org/worklets/#motivations I would never doubt that a Houdini spec use confusing naming :). Comment on attachment 410392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410392&action=review r=me > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:56 > + AudioWorkletThread* workerOrWorkletThread() final { return m_thread.ptr(); } I would just call this "thread". But if you prefer to call it "workerOrWorkletThread", let's rename the data member to match: m_workerOrWorkletThread. > Source/WebCore/Modules/webaudio/AudioWorkletThread.h:66 > + Ref<AudioWorkletMessagingProxy> m_messagingProxy; It seems like AudioWorkletMessagingProxy and AudioWorkletThread ref each other. Is this a cycle? Comment on attachment 410392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410392&action=review > Source/WebCore/workers/WorkerGlobalScope.h:94 > + WorkerThread* workerOrWorkletThread() final { return &m_thread; } The reason I did not call this one thread() is because workerOrWorkletThread() can return null... > Source/WebCore/workers/WorkerGlobalScope.h:95 > WorkerThread& thread() const { return m_thread; } ... and WorkerGlobalScope already has a thread() function which guarantees it returns non-null. Comment on attachment 410392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410392&action=review >> Source/WebCore/Modules/webaudio/AudioWorkletThread.h:66 >> + Ref<AudioWorkletMessagingProxy> m_messagingProxy; > > It seems like AudioWorkletMessagingProxy and AudioWorkletThread ref each other. Is this a cycle? Looks like it, good catch. Will fix. Comment on attachment 410392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410392&action=review > Source/WebCore/ChangeLog:11 > + The WorkletGlobalScope maintains a queue a fetch requests (basically a URL and a completion handler) and uses Nit. s/a queue a fetch/a queue of fetch/ Created attachment 410647 [details]
Patch
Created attachment 410648 [details]
Patch
Committed r268057: <https://trac.webkit.org/changeset/268057> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410648 [details]. |