Skip to content
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

Implement deprecated setPosition and setOrientation methods for AudioListener #22898 #23279

Merged
merged 1 commit into from Apr 28, 2019

Conversation

Projects
None yet
6 participants
@snarasi6
Copy link

commented Apr 26, 2019

I have uncommented the two methods SetPosition and SetOrientation in AudioListener.webidl and have included the two methods in the AudioListener.rs file. I have some errors while handling the NotSupported error for the methods. Can you please review the code and help me out with the errors?


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 26, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Apr 26, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/audiolistener.rs, components/script/dom/webidls/AudioListener.webidl
  • @KiChjang: components/script/dom/audiolistener.rs, components/script/dom/webidls/AudioListener.webidl
@highfive

This comment has been minimized.

Copy link

commented Apr 26, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

It looks to me like the error messages you're getting describe both the problem and the solution:

error[E0053]: method `SetOrientation` has an incompatible type for trait
    --> components/script/dom/audiolistener.rs:195:133
     |
195  |     fn SetOrientation(&self, x: Finite<f32>, y: Finite<f32>, z: Finite<f32>, xUp: Finite<f32>, yUp: Finite<f32>, zUp: Finite<f32>)->() {
     |                                                                                                                                     ^^ expected enum `std::result::Result`, found ()
     | 
    ::: /home/travis/build/servo/servo/target/debug/build/script-05686a0f060e3ec8/out/Bindings/AudioListenerBinding.rs:1065:135
     |
1065 |     fn SetOrientation(&self, x: Finite<f32>, y: Finite<f32>, z: Finite<f32>, xUp: Finite<f32>, yUp: Finite<f32>, zUp: Finite<f32>) -> Fallible<()>;
     |                                                                                                                                       ------------ type in trait
     |
     = note: expected type `fn(&dom::audiolistener::AudioListener, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>) -> std::result::Result<(), dom::bindings::error::Error>`
                found type `fn(&dom::audiolistener::AudioListener, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>, dom::bindings::num::Finite<f32>)`
``
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

That being said, returning Err(NotSupported) from the methods is not what the spec says. You will want to return Ok(()) when there is no automation curve present.

@Akhilesh1996 Akhilesh1996 force-pushed the snarasi6:master branch from d777828 to 4fc44de Apr 27, 2019

@Akhilesh1996

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Josh, I have made the changes and squashed commits. The build is successful but /mach test-tidy reports ./components/script/dom/audiolistener.rs:190: method declared in webidl is missing a comment with a specification link. But I have added a comment in the audiolistener.rs file for both the methods. I have run the tests in the webaudio. folder and updated results and reverted results for webaudio/the-audio-api/the-iirfilternode-interface/iirfilter.html. Can you check if what has been done till now is right and if I should make more changes?

Edit: ./mach test-tidy shows no errors now.

  • ./mach build -d does not report any errors

  • ./mach test-tidy does not report any errors

  • These changes fix #22898 (GitHub issue number if applicable)

  • There are tests for these changes OR

  • These changes do not require tests because ___

This change is Reviewable

@jdm
Copy link
Member

left a comment

This looks correct but doesn't compile right now.

Show resolved Hide resolved components/script/dom/audiolistener.rs
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Also if you add a space after the // then tidy should be happy.

Implemented SetOrientation and SetPosition functions but still have e…
…rrors in throwing NotSupported error message

@Akhilesh1996 Akhilesh1996 force-pushed the snarasi6:master branch from 22c50e6 to feeb891 Apr 27, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

📌 Commit feeb891 has been approved by jdm

@highfive highfive assigned jdm and unassigned nox Apr 27, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

⌛️ Testing commit feeb891 with merge 76415d8...

bors-servo added a commit that referenced this pull request Apr 27, 2019

Auto merge of #23279 - snarasi6:master, r=jdm
Implement deprecated setPosition and setOrientation methods for AudioListener #22898

<!-- Please describe your changes on the following line: -->
I have uncommented the two methods SetPosition and SetOrientation in AudioListener.webidl and have included the two methods in the AudioListener.rs file. I have some errors while handling the NotSupported error for the methods. Can you please review the code and help me out with the errors?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23279)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

💔 Test failed - status-taskcluster

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

⌛️ Testing commit feeb891 with merge ffd9441...

bors-servo added a commit that referenced this pull request Apr 28, 2019

Auto merge of #23279 - snarasi6:master, r=jdm
Implement deprecated setPosition and setOrientation methods for AudioListener #22898

<!-- Please describe your changes on the following line: -->
I have uncommented the two methods SetPosition and SetOrientation in AudioListener.webidl and have included the two methods in the AudioListener.rs file. I have some errors while handling the NotSupported error for the methods. Can you please review the code and help me out with the errors?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23279)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

@bors-servo bors-servo merged commit feeb891 into servo:master Apr 28, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.