Bug 215093

Created attachment 405856 [details]
Patch

Comment on attachment 405856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405856&action=review

> Source/WebCore/Modules/webaudio/GainNode.cpp:59
> +    auto result = gainNode->setChannelCount(options.channelCount.valueOr(2));
> +    if (result.hasException())
> +        return result.releaseException();
> +
> +    result = gainNode->setChannelCountMode(options.channelCountMode.valueOr(ChannelCountMode::Max));
> +    if (result.hasException())
> +        return result.releaseException();
> +
> +    result = gainNode->setChannelInterpretation(options.channelInterpretation.valueOr(ChannelInterpretation::Speakers));
> +    if (result.hasException())
> +        return result.releaseException();

This coded is duplicated in at least six nodes now. It would be better to have an AudioNode constructor or method that takes an AudioNodeOptions.

> Source/WebCore/Modules/webaudio/GainNode.h:42
> +    static ExceptionOr<Ref<GainNode>> create(BaseAudioContext& context, const GainOptions& = { });

Won't the bindings generator always pass a default-initialized dictionary?

Comment on attachment 405856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405856&action=review

>> Source/WebCore/Modules/webaudio/GainNode.cpp:59
>> +        return result.releaseException();
> 
> This coded is duplicated in at least six nodes now. It would be better to have an AudioNode constructor or method that takes an AudioNodeOptions.

Yes I agree with you. The default values (when not specified in the dictionary) differ from node to node though so we have to watch out for that.
I will try and find a good way to factor this is a follow-up patch. Maybe it can wait until we're done adding all constructor to avoid conflicting.

>> Source/WebCore/Modules/webaudio/GainNode.h:42
>> +    static ExceptionOr<Ref<GainNode>> create(BaseAudioContext& context, const GainOptions& = { });
> 
> Won't the bindings generator always pass a default-initialized dictionary?

Yes, but this is also called from BaseAudioContext::createGain() above.

Tools/Scripts/svn-apply failed to apply attachment 405856 [details] to trunk.
Please resolve the conflicts and upload a new patch.

Created attachment 405868 [details]
Patch

Comment on attachment 405868 [details]
Patch

Clearing flags on attachment: 405868

Committed r265227: <https://trac.webkit.org/changeset/265227>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/66497105>