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 HTMLMediaElement.loop Attribute #23236

Merged
merged 2 commits into from Apr 29, 2019

Conversation

Projects
None yet
8 participants
@swarnimarun
Copy link
Contributor

commented Apr 20, 2019

Work done for Implementing HTMLMediaElement.loop Attribute,

  • Uncomment loop attribute from webidl
  • Add make_bool macros for Loop and SetLoop functions
  • Update the required tests

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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 20, 2019

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

This comment has been minimized.

Copy link

commented Apr 20, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/HTMLMediaElement.webidl, components/script/dom/htmlmediaelement.rs
  • @KiChjang: components/script/dom/webidls/HTMLMediaElement.webidl, components/script/dom/htmlmediaelement.rs
@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

I am deeply sorry for the late PR.
Currently, there aren't any errors but I am not able to identify what else needs to be done to fix this issue, so that audio_loop_base test runs.

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

Also I didn't know where to mention it but I believe that the Fixme comment here,

// FIXME(nox): Why is this initialised to true?

Refers to the can-autoplay-flag, which seems to be the purpose of autoplaying from it's use,

self.autoplaying.set(true);
,and should begin with true as per the specs, https://html.spec.whatwg.org/multipage/media.html#can-autoplay-flag.

So it's probably an arbitary comment.

Extremely sorry if it should not be mentioned here.

@ferjm
Copy link
Member

left a comment

Thanks for the PR! The code looks good, but we need to change the test expectations a little bit before merging it.

I needed to check locally to see why the audio_loop_base.html cause the code looks ok. It appears that we are having troubles reproducing short mp3 files. So I wouldn't worry about it for now. I'll file a follow-up issue to investigate this.

Please, revert the changes done to tests/wpt/metadata/html/semantics/embedded-content/media-elements/audio_loop_base.html.ini and update the test expectations for tests/wpt/metadata/html/dom/interfaces.https.html.ini.

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Also I didn't know where to mention it but I believe that the Fixme comment here,

servo/components/script/dom/htmlmediaelement.rs

Line 291 in 3282446
// FIXME(nox): Why is this initialised to true?

Refers to the can-autoplay-flag, which seems to be the purpose of autoplaying from it's use,

servo/components/script/dom/htmlmediaelement.rs

Line 1015 in 3282446
self.autoplaying.set(true);
,and should begin with true as per the specs, https://html.spec.whatwg.org/multipage/media.html#can-autoplay-flag.

So it's probably an arbitary comment.

Extremely sorry if it should not be mentioned here.

No need to apologize :). This is a good place to mention it. A new issue would have also work.

Indeed, it seems that we can remove this comment from the code. This is likely a spec related question instead. cc @nox

Thanks for raising up the issue.

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

I needed to check locally to see why the audio_loop_base.html cause the code looks ok. It appears that we are having troubles reproducing short mp3 files. So I wouldn't worry about it for now. I'll file a follow-up issue to investigate this.

I filed #23241

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Ok thanks. Will fix up the tests ASAP.

@swarnimarun swarnimarun force-pushed the swarnimarun:loop_patch branch from a70e2ea to 4383f4d Apr 26, 2019

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Please, revert the changes done to tests/wpt/metadata/html/semantics/embedded-content/media-elements/audio_loop_base.html.ini and update the test expectations for tests/wpt/metadata/html/dom/interfaces.https.html.ini.

Rebased and updated the test expectations for HTMLMediaElement in interfaces.https

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

⌛️ Trying commit 4383f4d with merge 401b437...

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

Auto merge of #23236 - swarnimarun:loop_patch, r=<try>
[WIP] Implement HTMLMediaElement.loop Attribute

<!-- Please describe your changes on the following line: -->
Work done for Implementing HTMLMediaElement.loop Attribute,
- Uncomment `loop` attribute from webidl
- Add make_bool macros for Loop and SetLoop functions
- Update the required tests

---
<!-- 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 #22290

<!-- Either: -->
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Thanks!

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

📋 Looks like this PR is still in progress, ignoring approval

@highfive highfive assigned ferjm and unassigned SimonSapin Apr 29, 2019

@ferjm ferjm changed the title [WIP] Implement HTMLMediaElement.loop Attribute Implement HTMLMediaElement.loop Attribute Apr 29, 2019

@ferjm

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

📌 Commit 4383f4d has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

⌛️ Testing commit 4383f4d with merge 44bd09d...

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

Auto merge of #23236 - swarnimarun:loop_patch, r=ferjm
Implement HTMLMediaElement.loop Attribute

<!-- Please describe your changes on the following line: -->
Work done for Implementing HTMLMediaElement.loop Attribute,
- Uncomment `loop` attribute from webidl
- Add make_bool macros for Loop and SetLoop functions
- Update the required tests

---
<!-- 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 #22290

<!-- Either: -->
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

💔 Test failed - status-taskcluster

@CYBAI

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

@bors-servo retry

  ▶ FAIL [expected PASS] /css/CSS2/syntax/at-charset-042.xht
  └   → /css/CSS2/syntax/at-charset-042.xht 142a99007818eaad451b21601c1bfd04e79e8486
/css/CSS2/syntax/at-charset-001-ref.xht 0c2e5215e1346ac2422396dc1d3ca7f950906e29
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

⌛️ Testing commit 4383f4d with merge 172736a...

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

Auto merge of #23236 - swarnimarun:loop_patch, r=ferjm
Implement HTMLMediaElement.loop Attribute

<!-- Please describe your changes on the following line: -->
Work done for Implementing HTMLMediaElement.loop Attribute,
- Uncomment `loop` attribute from webidl
- Add make_bool macros for Loop and SetLoop functions
- Update the required tests

---
<!-- 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 #22290

<!-- Either: -->
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

💔 Test failed - mac-rel-wpt2

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

⌛️ Testing commit 4383f4d with merge 67beaa4...

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

Auto merge of #23236 - swarnimarun:loop_patch, r=ferjm
Implement HTMLMediaElement.loop Attribute

<!-- Please describe your changes on the following line: -->
Work done for Implementing HTMLMediaElement.loop Attribute,
- Uncomment `loop` attribute from webidl
- Add make_bool macros for Loop and SetLoop functions
- Update the required tests

---
<!-- 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 #22290

<!-- Either: -->
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@bors-servo bors-servo merged commit 4383f4d into servo:master Apr 29, 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
@atouchet

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Aren't the passing tests supposed to be removed?

@CYBAI

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

Aren't the passing tests supposed to be removed?

They will be removed in next Sync WPT PR :)

@atouchet

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

They will be removed in next Sync WPT PR :)

It doesn't look like they were.

@atouchet atouchet referenced this pull request May 30, 2019

Merged

Update test expectations #23480

0 of 5 tasks complete

bors-servo added a commit that referenced this pull request May 30, 2019

Auto merge of #23480 - atouchet:tests, r=ferjm
Update test expectations

<!-- Please describe your changes on the following line: -->
Some of these tests are passing as of #23236. Were these supposed to be removed?

---
<!-- 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/23480)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request May 30, 2019

Auto merge of #23480 - atouchet:tests, r=ferjm
Update test expectations

<!-- Please describe your changes on the following line: -->
Some of these tests are passing as of #23236. Were these supposed to be removed?

---
<!-- 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/23480)
<!-- Reviewable:end -->
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.