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 Ended media attribute #22348

Merged
merged 2 commits into from Jan 14, 2019

Conversation

Projects
None yet
6 participants
@germangb
Copy link
Contributor

germangb commented Dec 1, 2018

This PR should implement:

This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged)

  • #22321 (Define the Loop attribute)
  • #22293 (To identify playback direction. Either forwards or backwards)

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22294.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 1, 2018

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

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 1, 2018

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
@highfive

This comment has been minimized.

Copy link

highfive commented Dec 1, 2018

warning Warning warning

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

@germangb germangb force-pushed the germangb:html_media_ended branch from a1a96cb to 989bec6 Dec 1, 2018

@germangb germangb changed the title WIP: Implement Ended media attribute Implement Ended media attribute Dec 3, 2018

@ferjm
Copy link
Member

ferjm left a comment

Wonderfull work! I have only a few minor comments.


// XXX Placeholder for [https://github.com/servo/servo/issues/22293]
#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
pub enum PlaybackDirection {

This comment has been minimized.

@ferjm

ferjm Dec 5, 2018

Member

This does not need to be public.

this.upcast::<EventTarget>().fire_event(atom!("timeupdate"));

// Step 3.2.
if this.Ended() && !this.Loop() {

This comment has been minimized.

@ferjm

ferjm Dec 5, 2018

Member

I think this should be !this.Paused() instead of !this.Loop()

This comment has been minimized.

@germangb

germangb Dec 7, 2018

Author Contributor

You're right, I misread the steps

// the HTMLMediaElementMethods::Ended method

// Step 3.
self.queue_reaches_the_end_steps();

This comment has been minimized.

@ferjm

ferjm Dec 5, 2018

Member

Since this is only going to be used here, could we inline the implementation of queue_reaches_the_end_steps here?

This comment has been minimized.

@germangb

germangb Dec 7, 2018

Author Contributor

Yes, the reason I put it there is because I looked for examples of using the queue and tried to follow the same pattern. But it this case I'm splitting the steps between two methods, which probably hurts more than it helps..

Thanks for the feedback!

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 7, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 7, 2018

⌛️ Trying commit 5db4e49 with merge 81fd21e...

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

Auto merge of #22348 - germangb:html_media_ended, r=<try>
Implement Ended media attribute

<!-- Please describe your changes on the following line: -->

This PR should implement:
* New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position)
* `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback
* Queue steps for when the playback position reaches the end

This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged)
- #22321 (Define the Loop attribute)
- #22293 (To identify playback direction. Either forwards or backwards)

---
<!-- 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
- [ ] These changes fix #22294.

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 7, 2018

💔 Test failed - linux-rel-css

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 10, 2018

This patch makes some WPT pass:

{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "<video autoplay> with <track> child", "test": "/html/semantics/embedded-content/media-elements/autoplay-with-broken-track.html", "line": 123216, "action": "test_result", "expected": "TIMEOUT"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "<video autoplay> with <track> child", "test": "/html/semantics/embedded-content/media-elements/autoplay-with-broken-track.html", "line": 123218, "action": "test_result", "expected": "TIMEOUT"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "setting src attribute on a sufficiently long autoplay audio should trigger timeupdate event", "test": "/html/semantics/embedded-content/media-elements/event_timeupdate.html", "line": 177074, "action": "test_result", "expected": "NOTRUN"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "setting src attribute on a sufficiently long autoplay video should trigger timeupdate event", "test": "/html/semantics/embedded-content/media-elements/event_timeupdate.html", "line": 177075, "action": "test_result", "expected": "NOTRUN"}
{"status": "OK", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/semantics/embedded-content/media-elements/event_timeupdate.html", "line": 177076, "action": "test_result", "expected": "TIMEOUT"}

So we need to update the test expectations before merging.

@germangb

This comment has been minimized.

Copy link
Contributor Author

germangb commented Dec 10, 2018

According to the readme (in this case) I should remove the metadata files where all the expectations are no longer TIMEOUT nor NOTRUN, correct?

@germangb germangb force-pushed the germangb:html_media_ended branch from ba20d8c to ee6ccf9 Dec 10, 2018

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 10, 2018

According to the readme (in this case) I should remove the metadata files where all the expectations are no longer TIMEOUT nor NOTRUN, correct?

Yes, that's correct.

@ferjm

ferjm approved these changes Dec 10, 2018

Copy link
Member

ferjm left a comment

I think this is ready to be merged after squashing the commits. Thanks for the great work done here.

@jdm jdm assigned ferjm and unassigned avadacatavra Dec 10, 2018

@germangb germangb force-pushed the germangb:html_media_ended branch from ee6ccf9 to 7020930 Dec 10, 2018

@germangb

This comment has been minimized.

Copy link
Contributor Author

germangb commented Dec 10, 2018

Squashed!

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Dec 10, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 10, 2018

📌 Commit 7020930 has been approved by ferjm

@ferjm ferjm removed the S-needs-rebase label Jan 10, 2019

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Jan 10, 2019

hmm, right, I was confused by the S-needs-rebase label.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 10, 2019

@bors-servo try=wpt

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 10, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 10, 2019

⌛️ Trying commit a4165c9 with merge a847f89...

bors-servo added a commit that referenced this pull request Jan 10, 2019

Auto merge of #22348 - germangb:html_media_ended, r=<try>
Implement Ended media attribute

<!-- Please describe your changes on the following line: -->

This PR should implement:
* New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position)
* `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback
* Queue steps for when the playback position reaches the end

This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged)
- #22321 (Define the Loop attribute)
- #22293 (To identify playback direction. Either forwards or backwards)

---
<!-- 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 #22294.

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 10, 2019

💔 Test failed - linux-rel-wpt

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Jan 10, 2019

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "HTMLMediaElement interface: document.createElement(\"video\") must inherit property \"ended\" with the proper type", 
    "test": "/html/dom/interfaces.https.html?include=HTML.*", 
    "line": 79837, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "HTMLMediaElement interface: document.createElement(\"audio\") must inherit property \"ended\" with the proper type", 
    "test": "/html/dom/interfaces.https.html?include=HTML.*", 
    "line": 79891, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "HTMLMediaElement interface: new Audio() must inherit property \"ended\" with the proper type", 
    "test": "/html/dom/interfaces.https.html?include=HTML.*", 
    "line": 79939, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "HTMLMediaElement interface: attribute ended", 
    "test": "/html/dom/interfaces.https.html?include=HTML.*", 
    "line": 80032, 
    "action": "test_result", 
    "expected": "FAIL"
}

We need to update the expectations for tests/wpt/web-platform-tests/html/dom/interfaces.https.html

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 11, 2019

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

germangb added some commits Dec 11, 2018

Implement Ended media attribute
Signed-off-by: german gomez <germangb42@gmail.com>
Update interfaces.https.html test expectations
Signed-off-by: german gomez <germangb42@gmail.com>

@germangb germangb force-pushed the germangb:html_media_ended branch from a4165c9 to 352fed7 Jan 11, 2019

@germangb

This comment has been minimized.

Copy link
Contributor Author

germangb commented Jan 11, 2019

Some of the patterns in the player event's match statement had been rearranged

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Jan 14, 2019

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

⌛️ Trying commit 352fed7 with merge e6f9dfa...

bors-servo added a commit that referenced this pull request Jan 14, 2019

Auto merge of #22348 - germangb:html_media_ended, r=<try>
Implement Ended media attribute

<!-- Please describe your changes on the following line: -->

This PR should implement:
* New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position)
* `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback
* Queue steps for when the playback position reaches the end

This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged)
- #22321 (Define the Loop attribute)
- #22293 (To identify playback direction. Either forwards or backwards)

---
<!-- 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 #22294.

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

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

@ferjm

This comment has been minimized.

Copy link
Member

ferjm commented Jan 14, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

📌 Commit 352fed7 has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

⌛️ Testing commit 352fed7 with merge 1f9b134...

bors-servo added a commit that referenced this pull request Jan 14, 2019

Auto merge of #22348 - germangb:html_media_ended, r=ferjm
Implement Ended media attribute

<!-- Please describe your changes on the following line: -->

This PR should implement:
* New method `HTMLMediaElement::earliest_possible_position()` for the [earliest possible position](https://html.spec.whatwg.org/multipage/media.html#earliest-possible-position)
* `Ended` attribute following https://html.spec.whatwg.org/multipage/media.html#ended-playback
* Queue steps for when the playback position reaches the end

This PR contains placeholders for the following issues (I can rebase changes after the corresponding PRs get merged)
- #22321 (Define the Loop attribute)
- #22293 (To identify playback direction. Either forwards or backwards)

---
<!-- 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 #22294.

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 14, 2019

@bors-servo bors-servo merged commit 352fed7 into servo:master Jan 14, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment