StereoPannerNode DOM

[PurpleHairEngineer]

This is the DOM implementation for StereoPannerNode. Backend implemented in servo/media#243.

---

• ./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 

[highfive]

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

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/dom/baseaudiocontext.rs, components/script/dom/mod.rs, components/script/dom/stereopannernode.rs, components/script/dom/webidls/BaseAudioContext.webidl, components/script/dom/webidls/StereoPannerNode.webidl
• @KiChjang: components/script/dom/baseaudiocontext.rs, components/script/dom/mod.rs, components/script/dom/stereopannernode.rs, components/script/dom/webidls/BaseAudioContext.webidl, components/script/dom/webidls/StereoPannerNode.webidl

[highfive]

Warning

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

[jdm]

r? @Manishearth

[PurpleHairEngineer]

@Manishearth Okay, I believe I added the constraints appropriately. Thank you.

[bors-servo]

☔ The latest upstream changes (presumably #22743) made this pull request unmergeable. Please resolve the merge conflicts.

[PurpleHairEngineer]

@Manishearth I see that there are build errors for this, but a lot of them seem to be related to the fact that it can't find stereo_panner_node in servo_media. That's being implemented in a different pull request, servo/media#243 . Is there anything else I can do for this one before I get that one working?

[Manishearth]

Nah, get that one working first. There's a way to locally redirect dependencies but I'd rather just merge that first and look at this one later.

[PurpleHairEngineer]

@Manishearth Now that servo/media#243 has been merged, what is the best way to update this branch to reflect those changes? Should I pull the master branch, or something else?

[Manishearth]

./mach cargo update -p servo-media should be enough. Commit the changes it causes.

Also, you should first rebase and squash this (there will be a small merge conflict that needs fixing) so you won't have the same problem you had in the other PR.

Ideally if you can, make the commit that updates servo-media a separate commit before the main one (it's easier to rebase this in case of lockfile issues), but don't worry about that if you don't know how.

[PurpleHairEngineer]

@Manishearth Are you saying I should squash before running the /mach cargo update -p servo-media command? When I tried to run git rebase -i HEAD~5, it showed my commits at the top and then almost 400 commits after that. Are you saying I should squash them all?

[Manishearth]

git rebase -i HEAD~5

Run git fetch origin and then git rebase -i origin/master. Only squash your commits, no other commits should show up. I can do it for you if you need.

Yes, squash first.

[PurpleHairEngineer]

@Manishearth I ran git rebase -i origin/master, but in the editor, it is showing my 4 commits on top followed by about 390 others. What should I do? I don't want to close the editor because I know it does the rebasing even if I close it without making any changes.

[jdm]

If you remove all of the entries and close the editor, the rebase will be cancelled without making any changes.

[PurpleHairEngineer]

@jdm Thanks for the tip! Sorry, I don't have a lot of experience using git before this project.

[Manishearth]

On a bus right now, will push later. Is origin the servo repo or your fork? If it's your fork, do git remote add upstream https://github.com/servo/servo.git and then use git rebase -i upstream/master.

This worked for me locally.

[PurpleHairEngineer]

@Manishearth origin is the fork (https://github.com/josephhutch/servo). Thanks! I had to run git fetch in between those two commands, but I think it worked!

[Manishearth]

Pushed. I also included the update of servo-media.

Run the following to get the update locally

git fetch origin
git checkout origin/StereoPannerNode
git checkout -B StereoPannerNode
git push origin StereoPannerNode -u

[PurpleHairEngineer]

@Manishearth I'm getting the following build error:

error[E0599]: no variant or associated item named `StereoPannerNode` found for type `dom::bindings::codegen::InheritTypes::AudioNodeTypeId` in the current scope
   --> components\script\dom\audionode.rs:288:59
    |
288 |             EventTargetTypeId::AudioNode(AudioNodeTypeId::StereoPannerNode) => {
    |                                                           ^^^^^^^^^^^^^^^^ variant or associated item not found in `dom::bindings::codegen::InheritTypes::AudioNodeTypeId`

I've searched the repo for AudioNodeTypeId, but I can't figure out where I need to add StereoPannerNode in order to get rid of this error?

[Manishearth]

this gets subclassed twice, so the type id is a bit more complex

[PurpleHairEngineer]

@Manishearth This builds without issues for me now and I ran rustfmt on the relevant files. Could you please take a look?

[Manishearth]

Looks good!

Now we need to update test expectations. Please run

./mach test-wpt /webaudio/the-audio-api --log-raw log
./mach update-wpt log

and commit the changes

[Manishearth]

There shouldn't be any `MIN`, just `-1.`. You'll need to wait for a full build before running tests.

…

On Tue, Apr 30, 2019, 9:22 AM Maria Sable ***@***.***> wrote: *@PurpleHairEngineer* commented on this pull request. ------------------------------ In components/script/dom/stereopannernode.rs <#23281 (comment)>: > + let source_node = AudioScheduledSourceNode::new_inherited( + AudioNodeInit::StereoPannerNode(options.into()), + context, + node_options, + 0, /* inputs */ + 1, /* outputs */ + )?; + let node_id = source_node.node().node_id(); + let pan = AudioParam::new( + window, + context, + node_id, + ParamType::Pan, + AutomationRate::A_rate, + *options.pan, + f32::MIN, @Manishearth <https://github.com/Manishearth> Okay, I changed it to -1::MIN and did the same for MAX. However, when I run mach.bat test-wpt /webaudio/the-audio-api --log-raw log, it says Binary path C:\Users\Maria\servo\target\debug\servo.exe does not exist. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23281 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMK6SG3KTVFKIFYFVIELD3PTBW5BANCNFSM4HI4JHJA> .

[Manishearth]

@bors-servo r+

[bors-servo]

📌 Commit e22c005 has been approved by Manishearth

[bors-servo]

⌛ Testing commit e22c005 with merge 4d5f4b8...

[bors-servo]

💔 Test failed - linux-rel-wpt

[Manishearth]

@bors r+

[PurpleHairEngineer]

@Manishearth Thank you so much for all your help! It looks like you approved the merge but it is still open. Is there anything else I can do for this?

[Manishearth]

No, our CI robot will eventually test and merge this

[PurpleHairEngineer]

@Manishearth Oh okay, I was just wondering because it has the label awaiting review.

[Manishearth]

@bors-servo r+

ah, did it the wrong way

[bors-servo]

📌 Commit 58f0274 has been approved by Manishearth

[bors-servo]

⌛ Testing commit 58f0274 with merge aa63d60...

[bors-servo]

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: Manishearth
Pushing aa63d60 to master...