Bug 217153

Created attachment 410177 [details] Patch

Created attachment 410179 [details] Patch

Created attachment 410182 [details] Patch

Created attachment 410183 [details] Patch

Created attachment 410186 [details] Patch

Created attachment 410189 [details] Patch

Created attachment 410194 [details] Patch

Created attachment 410195 [details] Patch

Created attachment 410198 [details] Patch

Created attachment 410234 [details] Patch

Created attachment 410236 [details] Patch

Created attachment 410238 [details] Patch

Created attachment 410240 [details] Patch

Comment on attachment 410240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410240&action=review > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.idl:31 > + EnabledAtRuntime=AudioWorklet, Given that you can't actually created a AudioWorklet unless this is true, I don't think this is necessary. > Source/WebCore/Modules/webaudio/AudioWorkletNode.idl:31 > Conditional=WEB_AUDIO, > - EnabledBySetting=AudioWorklet, > + EnabledAtRuntime=AudioWorklet, Why is this needed? Seems like this should only ever called on the main thread.

Comment on attachment 410240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410240&action=review >> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.idl:31 >> + EnabledAtRuntime=AudioWorklet, > > Given that you can't actually created a AudioWorklet unless this is true, I don't think this is necessary. Oh, I think you are right. If so, I can probably keep using a setting. Let me give it a try.

Created attachment 410258 [details] Patch

Comment on attachment 410258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410258&action=review > Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.cpp:46 > + auto* document = worklet.document(); > + auto* frame = document->frame(); > + auto jsRuntimeFlags = frame ? frame->settings().javaScriptRuntimeFlags() : JSC::RuntimeFlags(); You can get settings directly from Document, and it is guaranteed to be non-null. > Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.h:57 > + WeakPtr<AudioWorklet> m_worklet; Is this used? > Source/WebCore/Modules/webaudio/AudioWorkletThread.cpp:96 > + scriptController->scheduleExecutionTermination(); > + scriptController->forbidExecution(); What ensures scriptController is non-null here? > Source/WebCore/Modules/webaudio/AudioWorkletThread.h:48 > +struct WorkletParameters { > +public: > + URL windowURL; > + JSC::RuntimeFlags jsRuntimeFlags; > + > + WorkletParameters isolatedCopy() const; > +}; This should get its own file, also no need for the explicit public:. > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:129 > + // FIXME: We should probably limit the number of threads we create for offline audio. > + m_renderThread = Thread::create("offline renderer", WTFMove(offThreadRendering), ThreadType::Audio); > return { }; Should we be using explicit threads at all here? Seems like a WorkQueue style option might work better (especially if these don't need to be "realish-time". > Source/WebCore/bindings/scripts/preprocess-idls.pl:101 > +$audioWorkletGlobalScopeConstructorsFile = CygwinPathIfNeeded($audioWorkletGlobalScopeConstructorsFile); If we are going to keep adding new types of global scopes, we really need to work out a better model than this copy and paste silliness.

Comment on attachment 410258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410258&action=review >> Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.cpp:46 >> + auto jsRuntimeFlags = frame ? frame->settings().javaScriptRuntimeFlags() : JSC::RuntimeFlags(); > > You can get settings directly from Document, and it is guaranteed to be non-null. Nice, will fix. >> Source/WebCore/Modules/webaudio/AudioWorkletMessagingProxy.h:57 >> + WeakPtr<AudioWorklet> m_worklet; > > Is this used? Hmm, maybe not yet. I guess I can omit it until it is actually needed. >> Source/WebCore/Modules/webaudio/AudioWorkletThread.cpp:96 >> + scriptController->forbidExecution(); > > What ensures scriptController is non-null here? The AudioWorkletGlobalScope constructs it unconditionally and we literally just constructed the AudioWorkletGlobalScope. The script only gets nulled out when prepareForDestruction() gets called. >> Source/WebCore/Modules/webaudio/AudioWorkletThread.h:48 >> +}; > > This should get its own file, also no need for the explicit public:. Ok. >> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:129 >> return { }; > > Should we be using explicit threads at all here? Seems like a WorkQueue style option might work better (especially if these don't need to be "realish-time". This is pre-existing code so I'd rather not change this at this point. Switching to a WorkQueue would also not be a trivial effort I believe because the WorkQueue's backing thread can change and we have assertions everywhere to make sure we are on the audio rendering thread. >> Source/WebCore/bindings/scripts/preprocess-idls.pl:101 >> +$audioWorkletGlobalScopeConstructorsFile = CygwinPathIfNeeded($audioWorkletGlobalScopeConstructorsFile); > > If we are going to keep adding new types of global scopes, we really need to work out a better model than this copy and paste silliness. Agreed. I did not anticipate we would add so many new ones :)

Created attachment 410270 [details] Patch

(In reply to Chris Dumez from comment #18) > > >> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:129 > >> return { }; > > > > Should we be using explicit threads at all here? Seems like a WorkQueue style option might work better (especially if these don't need to be "realish-time". > > This is pre-existing code so I'd rather not change this at this point. > Switching to a WorkQueue would also not be a trivial effort I believe > because the WorkQueue's backing thread can change and we have assertions > everywhere to make sure we are on the audio rendering thread. > My comment was only in relation to the comment you added, not about making a code change now.

Comment on attachment 410258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410258&action=review > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:-100 > - // FIXME: We should probably limit the number of threads we create for offline audio. I did not add the comment, it was already here. As I said, this is pre-existing code that I moved :)

(In reply to Chris Dumez from comment #21) > Comment on attachment 410258 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410258&action=review > > > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:-100 > > - // FIXME: We should probably limit the number of threads we create for offline audio. > > I did not add the comment, it was already here. As I said, this is > pre-existing code that I moved :) Ah, ok.

Comment on attachment 410270 [details] Patch Clearing flags on attachment: 410270 Committed r267859: <https://trac.webkit.org/changeset/267859>

All reviewed patches have been landed. Closing bug.

<rdar://problem/69861084>