[webkit-reviews] review granted: [Bug 217153] Add basic infrastructure for AudioWorklet : [Attachment 410258] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 217153: Add basic infrastructure for AudioWorklet
https://bugs.webkit.org/show_bug.cgi?id=217153

Attachment 410258: Patch

https://bugs.webkit.org/attachment.cgi?id=410258&action=review




--- Comment #17 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 410258
  --> https://bugs.webkit.org/attachment.cgi?id=410258
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.