Summary: | Add constructor for WaveShaperNode | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | Web Audio | Assignee: | 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
Chris Dumez
2020-07-30 14:34:01 PDT
Created attachment 405621 [details]
Patch
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. (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. Created attachment 405638 [details]
Patch
Created attachment 405639 [details]
Patch
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? Win failure seems legit, please fix it before landing. (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. (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) Created attachment 405700 [details]
Patch
Created attachment 405707 [details]
Patch
Committed r265152: <https://trac.webkit.org/changeset/265152> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405707 [details]. |