Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw errors for invalid values in panner node constructor #21555

Merged
merged 2 commits into from Aug 31, 2018
Merged

Throw errors for invalid values in panner node constructor #21555

merged 2 commits into from Aug 31, 2018

Conversation

@Manishearth
Copy link
Member

Manishearth commented Aug 31, 2018 •
edited by SimonSapin

This was an oversight in the spec and is being fixed

WebAudio/web-audio-api#1728

r? @ferjm

This change is Reviewable
@highfive
Copy link

highfive commented Aug 31, 2018

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented Aug 31, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@ferjm
ferjm approved these changes Aug 31, 2018
View changes
Copy link
Member

ferjm left a comment

Thanks! It seems that https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/ctor-panner.html is not checking these constraints. So it'd be nice to add the additional checks.
components/script/dom/pannernode.rs Outdated
@@ -60,6 +60,15 @@ impl PannerNode {
if count > 2 {
return Err(Error::NotSupported)
}
if options.maxDistance < 0. {
return Err(Error::NotSupported)

This comment has been minimized.

Sign in to view
Copy link
@ferjm

ferjm Aug 31, 2018

Member

Should this be a RangeError? https://webaudio.github.io/web-audio-api/#dom-pannernode-maxdistance
components/script/dom/pannernode.rs Outdated
if options.rolloffFactor < 0. {
return Err(Error::Range("rolloffFactor should be positive".into()))
}
if options.coneOuterGain < 0. || options.coneOuterGain > 360. {

This comment has been minimized.

Sign in to view
Copy link
@ferjm

ferjm Aug 31, 2018

Member

And this one InvalidStateError? https://webaudio.github.io/web-audio-api/#dom-pannernode-coneoutergain
@Manishearth Manishearth force-pushed the Manishearth:panner-node-ctor branch 4 times, most recently from 00468be to 130ed80 Aug 31, 2018
@servo-wpt-sync servo-wpt-sync mentioned this pull request Aug 31, 2018
Open
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Aug 31, 2018

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#80.
@Manishearth Manishearth force-pushed the Manishearth:panner-node-ctor branch from 130ed80 to 7d113c3 Aug 31, 2018
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Aug 31, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#80.
@Manishearth Manishearth force-pushed the Manishearth:panner-node-ctor branch from 7d113c3 to a8eb8fc Aug 31, 2018
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Aug 31, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#80.
@jdm
Copy link
Member

jdm commented Aug 31, 2018

The WPT manifest needs updating.
@Manishearth
Throw errors for invalid values in panner node constructor
303b62a
@Manishearth Manishearth force-pushed the Manishearth:panner-node-ctor branch from a8eb8fc to 216544c Aug 31, 2018
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Aug 31, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#80.
@Manishearth
Add tests for options bounds in PannerNode's constructor
Loading status checks…
e1131b4
@Manishearth Manishearth force-pushed the Manishearth:panner-node-ctor branch from 216544c to e1131b4 Aug 31, 2018
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Aug 31, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#80.
@Manishearth
Copy link
Member Author

Manishearth commented Aug 31, 2018

Added tests.

r? @ferjm
@ferjm
ferjm approved these changes Aug 31, 2018
View changes
Copy link
Member

ferjm left a comment

r=me. Thanks!
components/script/dom/pannernode.rs
if *options.maxDistance <= 0. {
return Err(Error::Range("maxDistance should be positive".into()))
}
if *options.refDistance < 0. {

This comment has been minimized.

Sign in to view
Copy link
@ferjm

ferjm Aug 31, 2018

Member

Perhaps <= 0.. The spec says to throw when it is set to a non-positive value. Same as with maxDistance
components/script/dom/pannernode.rs
@@ -230,19 +243,23 @@ impl PannerNodeMethods for PannerNode {
Finite::wrap(self.ref_distance.get())
}
// https://webaudio.github.io/web-audio-api/#dom-pannernode-refdistance
fn SetRefDistance(&self, val: Finite<f64>) {
fn SetRefDistance(&self, val: Finite<f64>) -> Fallible<()> {
if *val < 0. {

This comment has been minimized.

Sign in to view
Copy link
@ferjm

ferjm Aug 31, 2018

Member

Same comment as before: <= 0.
components/script/dom/pannernode.rs
@@ -289,7 +306,7 @@ impl PannerNodeMethods for PannerNode {
}
// https://webaudio.github.io/web-audio-api/#dom-pannernode-coneoutergain
fn SetConeOuterGain(&self, val: Finite<f64>) -> Fallible<()> {
if *val < 0. || *val > 360. {

This comment has been minimized.

Sign in to view
Copy link
@ferjm

ferjm Aug 31, 2018

Member

Ugh, I missed this one. Sorry.
@Manishearth
Copy link
Member Author

Manishearth commented Aug 31, 2018

So the spec says that both need to be non-positive, but the test indicates that zero is allowed for refDistance (but not maxDistance), and this agrees with Chrome. Firefox doesn't seem to throw errors at all?

I'll land this as is until the spec folks can determine what it should be

@bors-servo r=ferjm
@bors-servo
Copy link
Contributor

bors-servo commented Aug 31, 2018

📌 Commit e1131b4 has been approved by ferjm
@bors-servo
Copy link
Contributor

bors-servo commented Aug 31, 2018

⌛ Testing commit e1131b4 with merge 2351872...
bors-servo added a commit that referenced this pull request Aug 31, 2018
@bors-servo
Auto merge of #21555 - Manishearth:panner-node-ctor, r=ferjm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
2351872 
Throw errors for invalid values in panner node constructor

This was an oversight in the spec and is being fixed

WebAudio/web-audio-api#1728

r? @ferjm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21555)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 31, 2018

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: ferjm
Pushing 2351872 to master...
@bors-servo bors-servo merged commit e1131b4 into servo:master Aug 31, 2018
1 of 4 checks passed
1 of 4 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Aug 31, 2018

Error syncing changes upstream. Logs saved in error-snapshot-1535735434057.
@Manishearth
Copy link
Member Author

Manishearth commented Sep 1, 2018

Spec editor confirmed that a zero value is okay for ref distance, I'll open a PR later.
jdm added a commit to web-platform-tests/wpt that referenced this pull request Sep 1, 2018
@Manishearth @jdm
Add tests for options bounds in PannerNode's constructor
1ce329c 
Upstreamed from servo/servo#21555 [ci skip]
@servo-wpt-sync servo-wpt-sync mentioned this pull request Sep 1, 2018
Merged
@ferjm ferjm added this to Done in WebAudio Nov 29, 2018
@Manishearth Manishearth deleted the Manishearth:panner-node-ctor branch May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferjm ferjm

Assignees

@ferjm ferjm

Labels
None yet
Projects
WebAudio
  
Done
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@Manishearth @highfive @servo-wpt-sync @jdm @bors-servo @ferjm
You can’t perform that action at this time.