Closed Bug 1308437 Opened 8 years ago Closed 7 years ago

`setValueCurve` should throw on non-finite elements.

Categories

(Core :: Web Audio, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: padenot, Assigned: beekill, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(3 files, 2 obsolete files)

If the curve contains non-finite values, setValueCurveAtTime must signal a TypeError.  These values are illegal.

Spec changes at https://github.com/webaudio/web-audio-api/commit/a067bb9e273656becad3b3d660c20850516a175b.
Rank: 20
Depends on: 1184057
Mentor: dminor
Keywords: good-first-bug
Hi Dan, the bug seems to depends on bug 1184057. Is it ok to work on this bug?
Flags: needinfo?(dminor)
Hi Bao, in this case I think it is fine, because you should only need to change the point where the values are first set and I don't believe that will conflict with the work planned for Bug 1184057. Please let me know if you are interested in working on this!
Flags: needinfo?(dminor)
Sure, I'll work on this.
Thanks!
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
The commit [1] referenced by Paul was denied [2]. So I guess we don’t have to do anything :)

Please correct me if I'm wrong.

[1] https://github.com/webaudio/web-audio-api/commit/a067bb9e273656becad3b3d660c20850516a175b
[2] https://github.com/WebAudio/web-audio-api/pull/819
Flags: needinfo?(dminor)
Looks like you're right :) I checked the spec and it looks like the WebIDL in the spec matches what is in tree, so maybe the planned changes were never made. :padenot, is this another one we can just close or does this require some spec work?
Flags: needinfo?(dminor) → needinfo?(padenot)
Indeed, it looks like we do the right thing, apart from the exception, that is not the right one: TypeError should be thrown, instead of SyntaxError:

http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioEventTimeline.h#196

A test (probably web-platform test) would be a good addition as well.
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #7)
> Indeed, it looks like we do the right thing, apart from the exception, that
> is not the right one: TypeError should be thrown, instead of SyntaxError:
> 
> http://searchfox.org/mozilla-central/source/dom/media/webaudio/
> AudioEventTimeline.h#196
> 
> A test (probably web-platform test) would be a good addition as well.

OK, I’ll modify the incorrect error and write a test case.
Attached patch set_value_curve.patch (obsolete) — Splinter Review
I changed the SyntaxError to TypeError
Attachment #8833588 - Flags: review?(dminor)
Attached patch set_value_curve_testcase.patch (obsolete) — Splinter Review
I added a test case to check whether TypeError is thrown when calling setValueCurve with Infinity and Nan
Attachment #8833589 - Flags: review?(dminor)
Comment on attachment 8833588 [details] [diff] [review]
set_value_curve.patch

Review of attachment 8833588 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Please update your commit message to be more descriptive, e.g. "Change the exception thrown by `setValueCurve` on non-finite elements to TypeError".
Attachment #8833588 - Flags: review?(padenot)
Attachment #8833588 - Flags: review?(dminor)
Attachment #8833588 - Flags: review+
Comment on attachment 8833589 [details] [diff] [review]
set_value_curve_testcase.patch

Review of attachment 8833589 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: dom/media/webaudio/test/test_setValueCurveWithNonFiniteElements.html
@@ +21,5 @@
> +  arr[4] = 0.5;
> +
> +  try {
> +    audioParam.setValueCurveAtTime(arr, audioContext.currentTime(), 2);
> +    ok(false, "We can't setValueCurve with Infinity but we can");

Please change "We can't" to "We shouldn't be able to call"

@@ +38,5 @@
> +  arr[4] = 0.5;
> +
> +  try {
> +    audioParam.setValueCurveAtTime(arr, audioContext.currentTime(), 2);
> +    ok(false, "We can't setValueCurve with NaN but we can");

Please change "We can't" to "We shouldn't be able to call"
Attachment #8833589 - Flags: review?(padenot)
Attachment #8833589 - Flags: review?(dminor)
Attachment #8833589 - Flags: review+
Attachment #8833588 - Flags: review?(padenot) → review+
Attachment #8833589 - Flags: review?(padenot) → review+
I changed the commit message like you said.
Attachment #8833588 - Attachment is obsolete: true
Attachment #8834014 - Flags: review?(padenot)
Attachment #8834014 - Flags: review?(dminor)
I changed "We can't" to "We shouldn't be able to".

I also pushed the patches to try server. Here's the link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5ec32989bbae408c75069d5b9679eb2d3a32d
Attachment #8833589 - Attachment is obsolete: true
Attachment #8834015 - Flags: review?(padenot)
Attachment #8834015 - Flags: review?(dminor)
Attachment #8834014 - Flags: review?(dminor) → review+
Attachment #8834015 - Flags: review?(dminor) → review+
Attachment #8834014 - Flags: review?(padenot) → review+
Attachment #8834015 - Flags: review?(padenot) → review+
Hi Dan,
Here's the try server's result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5ec32989bbae408c75069d5b9679eb2d3a32d.

Looks like there are a lot of failed tests but none of them related to my patches, I guess. Can you have a look at the link? Thanks!
Flags: needinfo?(dminor)
Hi Bao,

The gtest failures look significant. My guess is that you would have to update the exception type in this test http://searchfox.org/mozilla-central/source/dom/media/webaudio/gtest/TestAudioEventTimeline.cpp#157.
The rest look like normal intermittent failures in the tests. Thanks!
Flags: needinfo?(dminor)
Thanks Dan for pointing that out. I updated the expected error value to TypeError and pushed the patch to try server. 

I'll post the result back asap.
Attachment #8834423 - Flags: review?(padenot)
Attachment #8834423 - Flags: review?(dminor)
Comment on attachment 8834423 [details] [diff] [review]
set_value_curve_update_gtest.patch

Review of attachment 8834423 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks !
Attachment #8834423 - Flags: review?(padenot) → review+
Comment on attachment 8834423 [details] [diff] [review]
set_value_curve_update_gtest.patch

lgtm
Attachment #8834423 - Flags: review?(dminor) → review+
Hi Dan, here's the latest result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5c47533dc212bb4b398a1aa3503a6a62579ccc

All the previous GTest failures are gone. Can you have a look a the link to see if there's anything wrong? Thank you.
Flags: needinfo?(dminor)
Those all look like intermittent failures to me. Thanks!
Flags: needinfo?(dminor)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f634bb78c4
Part 1: Change the exception thrown by 'setValueCurve' on non-finite elements to TypeError. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/845451c03a19
Part 2: Add test case to verify TypeError is thrown when calling setValueCurve on non-finite elements. r=dminor r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/9066a2ed8493
Part 3: Change expected error to TypeError when testing setValueCurveAtTime with non-finite elements in TestAudioEventTimeline.cpp. r=dminor, a=padenot
Keywords: checkin-needed
Blocks: 927610
No longer blocks: 927610
Blocks: 1184057
No longer depends on: 1184057
You need to log in before you can comment on or make changes to this bug.