[webkit-reviews] review granted: [Bug 76685] Fix ASSERT fail within AudioBus::processWithGainFrom() : [Attachment 124633] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Raymond <rgbbones at gmail.com>'s
request for review:
Bug 76685: Fix ASSERT fail within AudioBus::processWithGainFrom()
https://bugs.webkit.org/show_bug.cgi?id=76685

Attachment 124633: Patch
https://bugs.webkit.org/attachment.cgi?id=124633&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124633&action=review


Thanks Raymond for updating the patch and writing a test. This patch looks
good. I have some very minor spelling/grammar nits.

> LayoutTests/webaudio/audionode-connect-order.html:14
> +description("Test AudioNode connection order for checking ASSERT.");

Maybe the following text is more descriptive: "This tests that we don't trigger
an assertion failure due to the AudioNode connection order"?

> LayoutTests/webaudio/audionode-connect-order.html:27
> +	   data[i] = Math.sin(frequency * 2.0*Math.PI * i / sampleRate);

Nit: "2.0*Math.PI" => "2 * Math.PI"

(Notice the space on either side of the multiplication operator '*'; also, it's
unnecessary to write 2 as 2.0 since all JavaScript numbers are represented
using double precision floating point format)

> LayoutTests/webaudio/audionode-connect-order.html:54
> +    // We do this becacuse we try to catch the ASSERT which might be fired
due to audionode connection order,

I think you meant to say "try to trigger the ASSERT"? I tend to associate the
word "catch" with try/catch blocks, which is a construct that provides a way to
recover from an exception by transferring control to the catch clause. In
contrast, we don't usually have recovery code associated with an ASSERT(). And,
in a debug build, an assertion failure causes a crash. Therefore, I suggest
using the word "trigger" as in "trigger the ASSERT".

becacuse => because

And

audionode => AudioNode

> LayoutTests/webaudio/audionode-connect-order.html:55
> +    // especiallly when gain node and delay node is involved e.g.
https://bugs.webkit.org/show_bug.cgi?id=76685.

especiallly => especially