Bug 214989 – Add constructor for WaveShaperNode

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

[Chris Dumez]

Add constructor for WaveShaperNode:
- https://www.w3.org/TR/webaudio/#waveshapernode

[Chris Dumez]

Created attachment 405621 [details]
Patch

[Clark Wang]

Comment on attachment 405621 [details]
Patch

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

> Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:70
> +        node->setCurve(curve.get());

I think there needs to be a check if curve has length < 2, according to curve's spec: https://www.w3.org/TR/webaudio/#dom-waveshapernode-curve. Maybe it can inside of setCurve, or we can also catch it earlier.

[Chris Dumez]

(In reply to Clark Wang from comment #2)
> Comment on attachment 405621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405621&action=review
> 
> > Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:70
> > +        node->setCurve(curve.get());
> 
> I think there needs to be a check if curve has length < 2, according to
> curve's spec: https://www.w3.org/TR/webaudio/#dom-waveshapernode-curve.
> Maybe it can inside of setCurve, or we can also catch it earlier.

Indeed. Will fix, thanks.

[Chris Dumez]

Created attachment 405638 [details]
Patch

[Chris Dumez]

Created attachment 405639 [details]
Patch

[youenn fablet]

Comment on attachment 405639 [details]
Patch

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

> Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:40
> +ExceptionOr<Ref<WebCore::WaveShaperNode>> WaveShaperNode::create(BaseAudioContext& context, const WaveShaperOptions &options)

s/WebCore:://
s/WaveShaperOptions &/WaveShaperOptions&/

> Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:70
> +        result = node->setCurve(curve.get());

WTFMove

> Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:89
> +ExceptionOr<void> WaveShaperNode::setCurve(RefPtr<Float32Array>&& curve)

Why &&, it does not seem we move curve inside?

[youenn fablet]

Win failure seems legit, please fix it before landing.

[Chris Dumez]

(In reply to youenn fablet from comment #6)
> Comment on attachment 405639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405639&action=review
> 
> > Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:40
> > +ExceptionOr<Ref<WebCore::WaveShaperNode>> WaveShaperNode::create(BaseAudioContext& context, const WaveShaperOptions &options)

Fixed.

> 
> s/WebCore:://
> s/WaveShaperOptions &/WaveShaperOptions&/
> 
> > Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:70
> > +        result = node->setCurve(curve.get());
> 
> WTFMove

Fixed.

> 
> > Source/WebCore/Modules/webaudio/WaveShaperNode.cpp:89
> > +ExceptionOr<void> WaveShaperNode::setCurve(RefPtr<Float32Array>&& curve)
> 
> Why &&, it does not seem we move curve inside?

This is a setter so && makes sense. The generated bindings are also passing us an rvalue reference. It is true that this implementation is currently not leveraging this (it probably could with some further refactoring) but it could at some point.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> Win failure seems legit, please fix it before landing.

I was missing a #if ENABLE(WEB_AUDIO)

[Chris Dumez]

Created attachment 405700 [details]
Patch

[Chris Dumez]

Created attachment 405707 [details]
Patch

[EWS]

Committed r265152: <https://trac.webkit.org/changeset/265152>

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

[Radar WebKit Bug Importer]

<rdar://problem/66391487>