Bug 214989

Summary: Add constructor for WaveShaperNode
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, clark_wang, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, philipj, ryuan.choi, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 212611    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2020-07-30 14:34:01 PDT
Add constructor for WaveShaperNode:
- https://www.w3.org/TR/webaudio/#waveshapernode
Comment 1 Chris Dumez 2020-07-30 14:38:21 PDT
Created attachment 405621 [details]
Patch
Comment 2 Clark Wang 2020-07-30 16:14:30 PDT
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.
Comment 3 Chris Dumez 2020-07-30 16:18:29 PDT
(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.
Comment 4 Chris Dumez 2020-07-30 16:52:04 PDT
Created attachment 405638 [details]
Patch
Comment 5 Chris Dumez 2020-07-30 17:02:51 PDT
Created attachment 405639 [details]
Patch
Comment 6 youenn fablet 2020-07-31 02:54:17 PDT
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?
Comment 7 youenn fablet 2020-07-31 02:54:46 PDT
Win failure seems legit, please fix it before landing.
Comment 8 Chris Dumez 2020-07-31 09:29:58 PDT
(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.
Comment 9 Chris Dumez 2020-07-31 09:33:16 PDT
(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)
Comment 10 Chris Dumez 2020-07-31 09:33:38 PDT
Created attachment 405700 [details]
Patch
Comment 11 Chris Dumez 2020-07-31 10:39:56 PDT
Created attachment 405707 [details]
Patch
Comment 12 EWS 2020-07-31 11:47:54 PDT
Committed r265152: <https://trac.webkit.org/changeset/265152>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405707 [details].
Comment 13 Radar WebKit Bug Importer 2020-07-31 11:48:20 PDT
<rdar://problem/66391487>