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

Implementing HTMLMediaElement.volume attribute #22324

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
8 participants
@lucasfantacuci
Copy link
Contributor

lucasfantacuci commented Nov 30, 2018

This P.R. is going to implement the volume attribute to the HTMLMediaElement


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22292.
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Nov 30, 2018

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

@highfive

This comment has been minimized.

Copy link

highfive commented Nov 30, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/HTMLMediaElement.webidl
  • @KiChjang: components/script/dom/webidls/HTMLMediaElement.webidl
@highfive

This comment has been minimized.

Copy link

highfive commented Nov 30, 2018

warning Warning warning

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

KiChjang left a comment

Thanks for your work on this! I only have minor comments, once you address them, please squash your commits and we can land this!

Show resolved Hide resolved components/script/dom/htmlmediaelement.rs
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 30, 2018

Before we merge this, we'll want to pass the volume to the actual player.

@lucasfantacuci

This comment has been minimized.

Copy link
Contributor

lucasfantacuci commented Dec 3, 2018

Before we merge this, we'll want to pass the volume to the actual player.

Sorry, I didn't get it.

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:implement_html_media_element_volume_attribute branch from 5d574be to e00b016 Dec 3, 2018

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 3, 2018

Right now all we do is store and retrieve a value from a local member. The volume of the media stream is not affected; we need to add get_volume and set_volume APIs to https://github.com/servo/media/blob/f90235b16d9eb956ad9dba2dbca53e8f25e8b451/player/src/lib.rs#L46 (using the inner_player_proxy macro to call the appropriate backend APIs, so that we can call those methods on self.player in this code. Does that make sense?

@lucasfantacuci

This comment has been minimized.

Copy link
Contributor

lucasfantacuci commented Dec 3, 2018

Right now all we do is store and retrieve a value from a local member. The volume of the media stream is not affected; we need to add get_volume and set_volume APIs to https://github.com/servo/media/blob/f90235b16d9eb956ad9dba2dbca53e8f25e8b451/player/src/lib.rs#L46 (using the inner_player_proxy macro to call the appropriate backend APIs, so that we can call those methods on self.player in this code. Does that make sense?

Sure, it makes sense, so, I've created this P.R. handling it.

As @ferjm said I don't think get_volume at player trait should be necessary.

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:implement_html_media_element_volume_attribute branch from e00b016 to 207f95e Dec 3, 2018

@lucasfantacuci lucasfantacuci changed the title WIP: Implementing HTMLMediaElement.volume attribute Implementing HTMLMediaElement.volume attribute Dec 3, 2018

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 3, 2018

As @ferjm said I didn't realize get_volume at player trait should be necessary.

Yeah, I don't think we need a getter in this case as we have the value already available on the DOM side.

@lucasfantacuci

This comment has been minimized.

Copy link
Contributor

lucasfantacuci commented Dec 5, 2018

Hi Sirs,
Do I have to do anything else?
Regards,

@lucasfantacuci

This comment has been minimized.

Copy link
Contributor

lucasfantacuci commented Dec 10, 2018

  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_volume_check.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_volume_loudest-manual.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_volume_silent-manual.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/event_volumechange.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/video_volume_check.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/video_volume_loudest-manual.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/video_volume_silent-manual.html
  • tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/volume_nonfinite.html
  • tests/wpt/web-platform-tests/html/dom/interfaces.https.html
@KiChjang

This comment has been minimized.

Copy link
Member

KiChjang commented Dec 10, 2018

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Dec 10, 2018

Auto merge of #22324 - lucasfantacuci:implement_html_media_element_vo…
…lume_attribute, r=<try>

Implementing HTMLMediaElement.volume attribute

<!-- Please describe your changes on the following line: -->
Work In Progress. Do not Accept.
This P.R. is going to implement the volume attribute to the HTMLMediaElement

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22292.
<!-- 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/22324)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 10, 2018

⌛️ Trying commit 98d4eab with merge c0e46db...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 10, 2018

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 11, 2018

That failure is #22062.

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 11, 2018

tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/event_volumechange.html

We are missing the volumechange event part. Check the "Whenever either of the values ..." paragraph of the spec. We need to fire that event if volume changes.

tests/wpt/web-platform-tests/html/semantics/embedded-content/media-elements/audio_volume_loudest-manual.html

Did you try the *-manual ones locally? These are not run automatically and are not listed on the test expectations.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 11, 2018

⌛️ Trying commit 46c4fc7 with merge 0649710...

bors-servo added a commit that referenced this pull request Dec 11, 2018

Auto merge of #22324 - lucasfantacuci:implement_html_media_element_vo…
…lume_attribute, r=<try>

Implementing HTMLMediaElement.volume attribute

<!-- Please describe your changes on the following line: -->
Work In Progress. Do not Accept.
This P.R. is going to implement the volume attribute to the HTMLMediaElement

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22292.
<!-- 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/22324)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 11, 2018

💔 Test failed - linux-rel-css

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 11, 2018

It seems that this patch makes an extra test pass :). Update the test expectations for /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html and squash the commits please.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 11, 2018

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

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:implement_html_media_element_volume_attribute branch from 46c4fc7 to 211f358 Dec 12, 2018

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:implement_html_media_element_volume_attribute branch from d43265e to a2020fc Dec 12, 2018

@lucasfantacuci

This comment has been minimized.

Copy link
Contributor

lucasfantacuci commented Dec 12, 2018

It seems that this patch makes an extra test pass :). Update the test expectations for /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html and squash the commits please.

Done sir, And how about this conflict, I don't know what's going on.

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

The merge conflict means that your patch does not apply cleanly on the current master. You need to rebase your patch on top of the current master.

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:implement_html_media_element_volume_attribute branch from a2020fc to e7be09d Dec 12, 2018

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:implement_html_media_element_volume_attribute branch from e7be09d to ad3ec61 Dec 12, 2018

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 12, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

📌 Commit ad3ec61 has been approved by ferjm

@highfive highfive assigned ferjm and unassigned Manishearth Dec 12, 2018

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #22324 - lucasfantacuci:implement_html_media_element_vo…
…lume_attribute, r=ferjm

Implementing HTMLMediaElement.volume attribute

<!-- Please describe your changes on the following line: -->
This P.R. is going to implement the volume attribute to the HTMLMediaElement

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22292.
<!-- Either: -->
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Testing commit ad3ec61 with merge 0c506ef...

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 12, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Testing commit ad3ec61 with merge d5c1690...

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #22324 - lucasfantacuci:implement_html_media_element_vo…
…lume_attribute, r=ferjm

Implementing HTMLMediaElement.volume attribute

<!-- Please describe your changes on the following line: -->
This P.R. is going to implement the volume attribute to the HTMLMediaElement

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22292.
<!-- Either: -->
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

@bors-servo bors-servo merged commit ad3ec61 into servo:master Dec 12, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment