Bug 214615

Created attachment 404865 [details] Patch

Comment on attachment 404865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404865&action=review > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:652 > +ExceptionOr<Ref<PeriodicWave>> BaseAudioContext::createPeriodicWave(Vector<float>& real, Vector<float>& imaginary, const PeriodicWaveConstraints& constraints) Vector<float>&& > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:663 > + PeriodicWaveOptions* options = new PeriodicWaveOptions(); This is a memory leak. This should be: PeriodicWaveOptions options; > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:664 > + options->real = real; WTFMove(real) > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:665 > + options->imag = imaginary; WTFMove(imaginary) > Source/WebCore/Modules/webaudio/BaseAudioContext.h:157 > ExceptionOr<Ref<PeriodicWave>> createPeriodicWave(Float32Array& real, Float32Array& imaginary); What's this one? We should probably drop it in favor of your new one, no? > Source/WebCore/Modules/webaudio/BaseAudioContext.h:158 > + ExceptionOr<Ref<PeriodicWave>> createPeriodicWave(Vector<float>& real, Vector<float>& imaginary, const PeriodicWaveConstraints& = { }); Why add it to the C++ implementation but not the IDL? What's the point? > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:57 > +ExceptionOr<Ref<PeriodicWave>> PeriodicWave::create(BaseAudioContext& context, const PeriodicWaveOptions& options) PeriodicWaveOptions&& > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:62 > + if (options.real.hasValue()) { Can we format this as in the spec? I also mentioned this in previous comments but you don't need hasValue() and value(), it results in less concise code. if (options.real && options.imag) { real = WTFMove(*options.real); imag = WTFMove(*options.imag); } else if (options.real) { } else if ... > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:72 > + real.resize(2); I could be wrong but I don't think this code is safe. I do not believe those 2 vector items will be 0 initialized. Vector::fill() is your friend here I think. > Source/WebCore/Modules/webaudio/PeriodicWave.h:99 > + void createBandLimitedTables(const float* real, const float* imag, unsigned numberOfComponents, bool = false); We don't like boolean parameters in WebKit because this result is not very readable code like this. We prefer using enum classes: enum class ShouldDisableNormalization : bool { No, Yes }; > Source/WebCore/Modules/webaudio/PeriodicWave.idl:30 > + [MayThrowException] constructor(BaseAudioContext context, optional PeriodicWaveOptions options); You're changing the behavior of the shipping API. This should be behind a flag: [EnabledBySetting=ModernUnprefixedWebAudio] > Source/WebCore/Modules/webaudio/PeriodicWaveOptions.h:34 > +struct PeriodicWaveOptions : PeriodicWaveConstraints { public PeriodicWaveConstraints

Comment on attachment 404865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404865&action=review > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:77 > + if (real.size() != imag.size() || real.size() < 2 || imag.size() < 2) Can you please do the checks exactly as in the specification and thus provide a more accurate exception message when it makes sense? If the client did not provide a 'real' sequence, only an 'imag' sequence, then no point in saying that both have incorrect length.

Created attachment 404952 [details] Patch

Comment on attachment 404952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404952&action=review > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:301 > +unsigned BaseAudioContext::getMaxPeriodicWaveLength() const WebKit getters never start with "get". Also, I don't think we should have a getter for a constant. MaxPeriodicWaveLength does not seem to be used in this file anymore, can't you simply move MaxPeriodicWaveLength definition to WebKitAudioContext.cpp? > Source/WebCore/Modules/webaudio/PeriodicWave.h:40 > +enum class ShouldDisableNormalization : bool { I believe we prefer these on one line. > Source/WebCore/Modules/webaudio/PeriodicWave.h:42 > + Yes, No need for comma.

Created attachment 404965 [details] Patch

Created attachment 404969 [details] Patch

Added #include "BaseAudioContext.h" to PeriodicWave.h to hopefully get past build errors

Comment on attachment 404969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404969&action=review > Source/WebCore/Modules/webaudio/PeriodicWave.h:32 > +#include "BaseAudioContext.h" This should have been added to the cpp, not the header. It should remain a forward declaration in the header.

Created attachment 404974 [details] Patch

Comment on attachment 404974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404974&action=review > Source/WebCore/Modules/webaudio/BaseAudioContext.h:38 > +#include "PeriodicWaveOptions.h" Where is this used in this header? > Source/WebCore/Modules/webaudio/PeriodicWaveOptions.h:30 > +#include "ExceptionOr.h" This does not make sense, there is not ExceptionOr in this header...

Created attachment 404982 [details] Patch

Comment on attachment 404982 [details] Patch Some nits below. It also seems some tests might need new baselines. View in context: https://bugs.webkit.org/attachment.cgi?id=404982&action=review > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:89 > + imag[0] = 0; There are cases where these two lines may not be necessary, should we move them where they are useful. > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:91 > + auto waveTable = adoptRef(*new PeriodicWave(context.sampleRate())); Why not passing sampleRate directly, context does not seem used anywhere else. > Source/WebCore/Modules/webaudio/PeriodicWave.h:56 > + static ExceptionOr<Ref<PeriodicWave>> create(BaseAudioContext&, PeriodicWaveOptions&& = { }); Do we need { }. Could we forward declare PeriodicWaveOptions?

(In reply to youenn fablet from comment #13) > Comment on attachment 404982 [details] > Patch > > Some nits below. > It also seems some tests might need new baselines. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404982&action=review > > > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:89 > > + imag[0] = 0; > > There are cases where these two lines may not be necessary, should we move > them where they are useful. Got it, I adjusted this code. > > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:91 > > + auto waveTable = adoptRef(*new PeriodicWave(context.sampleRate())); > > Why not passing sampleRate directly, context does not seem used anywhere > else. Yeah, I agree that context is not used anywhere. I thought I needed to match this constructor spec here: https://www.w3.org/TR/webaudio/#periodicwave, which takes in context as a parameter. > > Source/WebCore/Modules/webaudio/PeriodicWave.h:56 > > + static ExceptionOr<Ref<PeriodicWave>> create(BaseAudioContext&, PeriodicWaveOptions&& = { }); > > Do we need { }. > Could we forward declare PeriodicWaveOptions? I thought I should pass in { } for options, since according to spec: https://www.w3.org/TR/webaudio/#dom-periodicwave-periodicwave, options is an optional parameter. Though, I think I could move = { } to IDL file, if that is preferred.

Comment on attachment 404982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404982&action=review >>> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:89 >>> + imag[0] = 0; >> >> There are cases where these two lines may not be necessary, should we move them where they are useful. > > Got it, I adjusted this code. This was matching the spec text so I liked it.

Comment on attachment 404982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404982&action=review >>> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:91 >>> + auto waveTable = adoptRef(*new PeriodicWave(context.sampleRate())); >> >> Why not passing sampleRate directly, context does not seem used anywhere else. > > Yeah, I agree that context is not used anywhere. I thought I needed to match this constructor spec here: https://www.w3.org/TR/webaudio/#periodicwave, which takes in context as a parameter. Yes, Clark is correct. The factor takes a context in because that is what the IDL says. >>> Source/WebCore/Modules/webaudio/PeriodicWave.h:56 >>> + static ExceptionOr<Ref<PeriodicWave>> create(BaseAudioContext&, PeriodicWaveOptions&& = { }); >> >> Do we need { }. >> Could we forward declare PeriodicWaveOptions? > > I thought I should pass in { } for options, since according to spec: https://www.w3.org/TR/webaudio/#dom-periodicwave-periodicwave, options is an optional parameter. Though, I think I could move = { } to IDL file, if that is preferred. I like it with a default value. No reason you have to specify options and they are indeed optional in the IDL as well.

Created attachment 405043 [details] Patch

Created attachment 405045 [details] Patch

Created attachment 405048 [details] Patch

Committed r264782: <https://trac.webkit.org/changeset/264782> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405048 [details].

<rdar://problem/66005752>