[webkit-reviews] review denied: [Bug 214746] Added Constructor method to OscillatorNode : [Attachment 405163] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 214746: Added Constructor method to OscillatorNode
https://bugs.webkit.org/show_bug.cgi?id=214746

Attachment 405163: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 405163
  --> https://bugs.webkit.org/attachment.cgi?id=405163
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:634
> -    refNode(node);
> +    refNode(node.releaseReturnValue());

This change is incorrect. We can’t return node *after* calling
releaseReturnValue on it. We need to put the return value into a separate
variable and return that, not "node", because the value has been moved out of
"node". That’s what the word "release" means in "releaseReturnValue".

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:48
> +// FIXME: Remove once old constructor is phased out.

I’m not sure these comments are helpful. Yes, we should remove anything unused,
but we don’t need to add comments next to each thing that we plan to remove.
Unless there is some scenario where we think this is particularly helpful? It’s
not even obvious what "old constructor" refers to, unless you are already
expert.

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:76
> +// FIXME: Remove once old constructor is phased out.

Ditto.

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:103
> +    , m_detune(AudioParam::create(context, "detune", options.detune,
-1200*log2(FLT_MAX), 1200*log2(FLT_MAX)))

WebKit project formatting adds spaces around "*".

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:107
> +    , m_firstRender(true)
> +    , m_virtualReadIndex(0)
> +    , m_phaseIncrements(AudioNode::ProcessingSizeInFrames)
> +    , m_detuneValues(AudioNode::ProcessingSizeInFrames)

These should be done in the class definition instead of in multiple
constructors.

> Source/WebCore/Modules/webaudio/OscillatorNode.h:41
> +    // FIXME: Remove once old constructor is phased out.

Ditto.

> Source/WebCore/Modules/webaudio/OscillatorNode.idl:25
>   */
> -
> -enum OscillatorType {
> -    "sine",
> -    "square",
> -    "sawtooth",
> -    "triangle",
> -    "custom"
> -};
> -
>  [

Please leave a blank line.

> Source/WebCore/Modules/webaudio/OscillatorOptions.h:37
> +struct OscillatorOptions : public AudioNodeOptions {

No need for "public" with a struct.

> Source/WebCore/Modules/webaudio/OscillatorOptions.h:41
> +    RefPtr<PeriodicWave> periodicWave;

Is it correct that this is silently ignored for types other than custom?

> Source/WebCore/Modules/webaudio/OscillatorType.h:30
> +enum class OscillatorType {

enum class OscillatorType : uint8_t

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:48
> +// FIXME: Remove once old constructor is phased out.

I suggest removing these comments. Of course we should remove things once they
are unused.

> Source/WebCore/Modules/webaudio/WebKitAudioContext.cpp:184
> +    Ref<OscillatorNode> node = OscillatorNode::create(*this, sampleRate());

auto