Bug 217242 – Update Worklet.addModule() to actually fetch and evaluate the module script

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Update WorkletGlobalScope.addModule() to actually fetch the module script.

[Chris Dumez]

Created attachment 410357 [details]
Patch

[Chris Dumez]

Created attachment 410360 [details]
Patch

[Chris Dumez]

Created attachment 410365 [details]
Patch

[Chris Dumez]

Created attachment 410392 [details]
Patch

[Sam Weinig]

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?

[Chris Dumez]

(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.

[Sam Weinig]

(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!

[Chris Dumez]

(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

[Sam Weinig]

(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 :).

[Geoffrey Garen]

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?

[Chris Dumez]

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.

[Chris Dumez]

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.

[Peng Liu]

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/

[Chris Dumez]

Created attachment 410647 [details]
Patch

[Chris Dumez]

Created attachment 410648 [details]
Patch

[EWS]

Committed r268057: <https://trac.webkit.org/changeset/268057>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410648 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70005642>