Throw correct exceptions on AudioBufferSourceNode start() and stop()

ASSIGNED
Assigned to

Status

()

Core
Web Audio
P3
normal
Rank:
25
ASSIGNED
4 years ago
9 months ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spec])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Comment 1

4 years ago
Created attachment 8513934 [details]
MozReview Request: bz://1091312/padenot
Attachment #8513934 - Flags: review?(ehsan.akhgari)
(Assignee)

Comment 2

4 years ago
/r/59 - Bug 1091312 - Throw InvalidAccessError when non-finite floats are passed to AudioBufferSourceNode.{start, stop}, or if offset is greater than the buffer length. r=ehsan

Pull down this commit:

hg pull review -r 25cae40162467a53f8ad9370722780386a0ba8c5
(Assignee)

Comment 3

4 years ago
Created attachment 8513938 [details] [diff] [review]
Throw InvalidAccessError when non-finite floats are passed to AudioBufferSourceNode.{start, stop}, or if offset is greater than the buffer length

Trying the new stuff, do the review here if you're not adventurous, heh.
Attachment #8513938 - Flags: review?(ehsan.akhgari)
(Assignee)

Updated

4 years ago
Assignee: nobody → padenot
Status: NEW → ASSIGNED

Comment 4

4 years ago
https://reviewboard.mozilla.org/r/59/#review29

::: dom/media/webaudio/AudioBufferSourceNode.cpp
(Diff revision 1)
> +      (mBuffer && aOffset > mBuffer->Length() / mBuffer->SampleRate())) {

Nit: please wrap the greater than check in parentheses.

::: dom/media/webaudio/test/test_audioBufferSourceNodeParams.html
(Diff revision 1)
> +      s.start(-1);

Did you mean to also add a check for Infinity, as the comment suggests?

::: dom/media/webaudio/test/test_audioBufferSourceNodeParams.html
(Diff revision 1)
> +  [-1, buffer.duration + 1, NaN, Infinity].forEach(function(v) {

Please test -Infinity as well.

::: dom/media/webaudio/test/test_audioBufferSourceNodeParams.html
(Diff revision 1)
> +  [-1, Infinity, NaN].forEach(function(v) {

Here too.

::: dom/media/webaudio/test/test_audioBufferSourceNodeParams.html
(Diff revision 1)
> +  [-1, Infinity, NaN].forEach(function(v) {

And here.

::: dom/webidl/AudioBufferSourceNode.webidl
(Diff revision 1)
> -    void start(optional double when = 0, optional double grainOffset = 0,
> +    void start(optional unrestricted double when = 0, optional unrestricted double grainOffset = 0,

I assume you're going to change the spec accordingly too.

Updated

4 years ago
Attachment #8513938 - Flags: review?(ehsan.akhgari)

Updated

4 years ago
Attachment #8513934 - Flags: review?(ehsan.akhgari) → review+
(Assignee)

Comment 6

3 years ago
Comment on attachment 8513934 [details]
MozReview Request: bz://1091312/padenot
Attachment #8513934 - Attachment is obsolete: true
Attachment #8618502 - Flags: review+
(Assignee)

Comment 7

3 years ago
Created attachment 8618502 [details]
MozReview Request: Bug 1091312 - Throw InvalidAccessError when non-finite floats are passed to AudioBufferSourceNode.{start, stop}, or if offset is greater than the buffer length. r=ehsan
Paul - this is r+'d - what's blocking landing it?  Thanks
Rank: 25
Flags: needinfo?(padenot)
Priority: -- → P2
Whiteboard: [spec]
(Assignee)

Comment 9

3 years ago
We need to talk at TPAC about this.
Flags: needinfo?(padenot)
Keywords: dev-doc-needed
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.