Bug 217242 - Update Worklet.addModule() to actually fetch and evaluate the module script
Summary: Update Worklet.addModule() to actually fetch and evaluate the module script
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 217194
Blocks: 217058 217251
  Show dependency treegraph
 
Reported: 2020-10-02 12:08 PDT by Chris Dumez
Modified: 2020-10-06 10:19 PDT (History)
21 users (show)

See Also:


Attachments
Patch (73.72 KB, patch)
2020-10-02 13:07 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (74.25 KB, patch)
2020-10-02 13:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.74 KB, patch)
2020-10-02 13:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.80 KB, patch)
2020-10-02 17:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (84.02 KB, patch)
2020-10-06 09:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (84.03 KB, patch)
2020-10-06 09:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-10-02 12:08:22 PDT
Update WorkletGlobalScope.addModule() to actually fetch the module script.
Comment 1 Chris Dumez 2020-10-02 13:07:53 PDT
Created attachment 410357 [details]
Patch
Comment 2 Chris Dumez 2020-10-02 13:42:17 PDT
Created attachment 410360 [details]
Patch
Comment 3 Chris Dumez 2020-10-02 13:56:10 PDT
Created attachment 410365 [details]
Patch
Comment 4 Chris Dumez 2020-10-02 17:42:05 PDT
Created attachment 410392 [details]
Patch
Comment 5 Sam Weinig 2020-10-03 09:57:26 PDT
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?
Comment 6 Chris Dumez 2020-10-05 08:18:10 PDT
(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.
Comment 7 Sam Weinig 2020-10-05 08:45:09 PDT
(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!
Comment 8 Chris Dumez 2020-10-05 08:49:53 PDT
(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
Comment 9 Sam Weinig 2020-10-05 08:56:50 PDT
(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 10 Geoffrey Garen 2020-10-05 15:07:25 PDT
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 11 Chris Dumez 2020-10-05 15:31:26 PDT
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 12 Chris Dumez 2020-10-05 15:32:19 PDT
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 13 Peng Liu 2020-10-05 16:02:01 PDT
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/
Comment 14 Chris Dumez 2020-10-06 09:36:41 PDT
Created attachment 410647 [details]
Patch
Comment 15 Chris Dumez 2020-10-06 09:37:25 PDT
Created attachment 410648 [details]
Patch
Comment 16 EWS 2020-10-06 10:18:34 PDT
Committed r268057: <https://trac.webkit.org/changeset/268057>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410648 [details].
Comment 17 Radar WebKit Bug Importer 2020-10-06 10:19:22 PDT
<rdar://problem/70005642>