Closed
Bug 1308437
Opened 8 years ago
Closed 7 years ago
`setValueCurve` should throw on non-finite elements.
Categories
(Core :: Web Audio, defect, P2)
Core
Web Audio
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)
795 bytes,
patch
|
dminor
:
review+
padenot
:
review+
|
Details | Diff | Splinter Review |
2.72 KB,
patch
|
dminor
:
review+
padenot
:
review+
|
Details | Diff | Splinter Review |
1.61 KB,
patch
|
dminor
:
review+
padenot
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•8 years ago
|
Rank: 20
Updated•7 years ago
|
Mentor: dminor
Keywords: good-first-bug
Assignee | ||
Comment 1•7 years ago
|
||
Hi Dan, the bug seems to depends on bug 1184057. Is it ok to work on this bug?
Flags: needinfo?(dminor)
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
Sure, I'll work on this.
Assignee | ||
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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)
Reporter | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
I changed the SyntaxError to TypeError
Attachment #8833588 -
Flags: review?(dminor)
Assignee | ||
Comment 10•7 years ago
|
||
I added a test case to check whether TypeError is thrown when calling setValueCurve with Infinity and Nan
Attachment #8833589 -
Flags: review?(dminor)
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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+
Reporter | ||
Updated•7 years ago
|
Attachment #8833588 -
Flags: review?(padenot) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8833589 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 13•7 years ago
|
||
I changed the commit message like you said.
Attachment #8833588 -
Attachment is obsolete: true
Attachment #8834014 -
Flags: review?(padenot)
Attachment #8834014 -
Flags: review?(dminor)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8834014 -
Flags: review?(dminor) → review+
Updated•7 years ago
|
Attachment #8834015 -
Flags: review?(dminor) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8834014 -
Flags: review?(padenot) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8834015 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
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)
Reporter | ||
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
Comment on attachment 8834423 [details] [diff] [review] set_value_curve_update_gtest.patch lgtm
Attachment #8834423 -
Flags: review?(dminor) → review+
Assignee | ||
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
Those all look like intermittent failures to me. Thanks!
Flags: needinfo?(dminor)
Keywords: checkin-needed
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/59f634bb78c4 https://hg.mozilla.org/mozilla-central/rev/845451c03a19 https://hg.mozilla.org/mozilla-central/rev/9066a2ed8493
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 24•7 years ago
|
||
Documentation updated: https://developer.mozilla.org/en-US/docs/Web/API/AudioParam/setValueCurveAtTime https://developer.mozilla.org/en-US/Firefox/Releases/54
Keywords: dev-doc-complete
Updated•6 months ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•