[webkit-reviews] review denied: [Bug 214615] Added PeriodicWave constructor according to spec : [Attachment 404865] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 214615: Added PeriodicWave constructor according to spec
https://bugs.webkit.org/show_bug.cgi?id=214615

Attachment 404865: Patch

https://bugs.webkit.org/attachment.cgi?id=404865&action=review




--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 404865
  --> https://bugs.webkit.org/attachment.cgi?id=404865
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