Add task source for media element #21879

chansuke · 2018-10-06T05:39:04Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #21612 .
These changes do not require tests because the existing test should cover this code.

This change is

ghost · 2018-10-06T05:39:06Z

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

highfive · 2018-10-06T05:39:09Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_thread.rs, components/script/dom/window.rs, components/script/dom/htmlmediaelement.rs, components/script/task_source/mod.rs, components/script/task_source/media_element.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/window.rs, components/script/dom/htmlmediaelement.rs, components/script/task_source/mod.rs, components/script/task_source/media_element.rs

highfive · 2018-10-06T05:39:11Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

ferjm · 2018-10-09T13:46:37Z

components/script/dom/htmlmediaelement.rs

@@ -619,9 +615,7 @@ impl HTMLMediaElement {
        let this = Trusted::new(self);
        let generation_id = self.generation_id.get();
        self.take_pending_play_promises(Err(Error::NotSupported));
-        // FIXME(nox): Why are errors silenced here?


This FIXME is still valid :) Do not remove it, please.

ferjm

Thanks for working on this @chansuke :)

The task source related changes looks good to me. However, it would be great if you could remove the second commit and rebase on top of master instead of creating a merge commit.

Thanks!

chansuke · 2018-10-09T13:55:06Z

@ferjm Ok.Thanks for your review.

ferjm

Looks better now :) Thanks for the rebase.

I think there are only two things left to fix here:

The servo_channel::Sender related build error.
These tidy issues https://travis-ci.org/servo/servo/jobs/439453305#L447

ferjm · 2018-10-10T09:37:24Z

components/script/task_source/media_element.rs

+use task_source::{TaskSource, TaskSourceName};
+
+#[derive(Clone, JSTraceable)]
+pub struct MediaElementTaskSource(pub Sender<MainThreadScriptMsg>, pub PipelineId);


This Sender needs to be a servo_channel::Sender instead of a std::sync::mpsc::Sender

ferjm · 2018-10-10T12:53:49Z

One last change ;)

https://travis-ci.org/servo/servo/jobs/439558351#L447

Tip: you can check that tidy is happy with the patch by running ./mach test-tidy locally.


        Add task source for media element

ferjm · 2018-10-10T13:31:34Z

@bors-servo try

bors-servo · 2018-10-10T13:31:40Z

⌛ Trying commit 7b3cf27 with merge 7c41e75...


        Auto merge of #21879 - chansuke:add_element_event_taskforce, r=<try>

Add task source for media element  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21612 . - [x] These changes do not require tests because the existing test should cover this code.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21879)

bors-servo · 2018-10-10T13:34:29Z

💔 Test failed - android-x86

CYBAI · 2018-10-10T16:06:32Z

@bors-servo retry

#21205

bors-servo · 2018-10-10T16:06:37Z

⌛ Trying commit 7b3cf27 with merge 1523000...


        Auto merge of #21879 - chansuke:add_element_event_taskforce, r=<try>

Add task source for media element  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21612 . - [x] These changes do not require tests because the existing test should cover this code.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21879)

bors-servo · 2018-10-10T16:32:58Z

💔 Test failed - linux-rel-wpt

ferjm · 2018-10-10T17:22:55Z

@bors-servo try retry

bors-servo · 2018-10-10T17:23:02Z

⌛ Trying commit 7b3cf27 with merge 7ac3543...


        Auto merge of #21879 - chansuke:add_element_event_taskforce, r=<try>

Add task source for media element  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21612 . - [x] These changes do not require tests because the existing test should cover this code.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21879)

bors-servo · 2018-10-10T17:53:44Z

💔 Test failed - status-taskcluster

ferjm · 2018-10-11T07:36:04Z

@bors-servo r+

bors-servo · 2018-10-11T07:36:05Z

📌 Commit 7b3cf27 has been approved by ferjm

bors-servo · 2018-10-11T07:36:12Z

⌛ Testing commit 7b3cf27 with merge ae0a5a8...


        Auto merge of #21879 - chansuke:add_element_event_taskforce, r=ferjm

Add task source for media element  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21612 . - [x] These changes do not require tests because the existing test should cover this code.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21879)

bors-servo · 2018-10-11T08:00:31Z

💔 Test failed - linux-rel-wpt

CYBAI · 2018-10-11T09:27:03Z

@bors-servo retry

bors-servo · 2018-10-11T09:50:36Z

⚡ Previous build results for android, android-x86, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, status-taskcluster, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt...

bors-servo · 2018-10-11T10:15:02Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, status-taskcluster, windows-msvc-dev
Approved by: ferjm
Pushing ae0a5a8 to master...

highfive assigned asajeffrey Oct 6, 2018

highfive added the S-awaiting-review label Oct 6, 2018

ferjm reviewed Oct 9, 2018

View changes

ferjm requested changes Oct 9, 2018

View changes

chansuke force-pushed the chansuke:add_element_event_taskforce branch 3 times, most recently from 09ba080 to 5f62b64 Oct 10, 2018

ferjm requested changes Oct 10, 2018

View changes

chansuke force-pushed the chansuke:add_element_event_taskforce branch from 5f62b64 to 3e50fda Oct 10, 2018

Add task source for media element

Loading status checks…

7b3cf27

chansuke force-pushed the chansuke:add_element_event_taskforce branch from 3e50fda to 7b3cf27 Oct 10, 2018

highfive added the S-tests-failed label Oct 10, 2018

highfive assigned ferjm and unassigned asajeffrey Oct 11, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Oct 11, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 11, 2018

bors-servo merged commit 7b3cf27 into servo:master Oct 11, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Oct 11, 2018

Implement unhandledrejection event #20755

Merged

Unify the task source and task canceller API #21804

Merged

ferjm mentioned this pull request Nov 6, 2018

HTMLMediaElement - use media element instead of dom manipulation task… #22129

Merged

2 of 2 tasks complete

ferjm added this to In progress in Media playback Nov 29, 2018

ferjm moved this from In progress to Done in Media playback Nov 29, 2018

servo / servo

Add task source for media element #21879

Add task source for media element #21879

chansuke commented Oct 6, 2018 •

edited by SimonSapin

ghost commented Oct 6, 2018

highfive commented Oct 6, 2018

highfive commented Oct 6, 2018

This comment has been minimized.

ferjm left a comment

chansuke commented Oct 9, 2018

ferjm left a comment

This comment has been minimized.

ferjm commented Oct 10, 2018

ferjm commented Oct 10, 2018

bors-servo commented Oct 10, 2018

bors-servo commented Oct 10, 2018

CYBAI commented Oct 10, 2018

bors-servo commented Oct 10, 2018

bors-servo commented Oct 10, 2018

ferjm commented Oct 10, 2018

bors-servo commented Oct 10, 2018

bors-servo commented Oct 10, 2018

ferjm commented Oct 11, 2018

bors-servo commented Oct 11, 2018

bors-servo commented Oct 11, 2018

bors-servo commented Oct 11, 2018

CYBAI commented Oct 11, 2018

bors-servo commented Oct 11, 2018

bors-servo commented Oct 11, 2018

servo / servo

Join GitHub today

Add task source for media element #21879

Add task source for media element #21879

Conversation

chansuke commented Oct 6, 2018 • edited by SimonSapin

ghost commented Oct 6, 2018

highfive commented Oct 6, 2018

highfive commented Oct 6, 2018

This comment has been minimized.

ferjm Oct 9, 2018

ferjm left a comment

chansuke commented Oct 9, 2018

ferjm left a comment

This comment has been minimized.

ferjm Oct 10, 2018

ferjm commented Oct 10, 2018

ferjm commented Oct 10, 2018

bors-servo commented Oct 10, 2018

bors-servo commented Oct 10, 2018

CYBAI commented Oct 10, 2018

bors-servo commented Oct 10, 2018

bors-servo commented Oct 10, 2018

ferjm commented Oct 10, 2018

bors-servo commented Oct 10, 2018

bors-servo commented Oct 10, 2018

ferjm commented Oct 11, 2018

bors-servo commented Oct 11, 2018

bors-servo commented Oct 11, 2018

bors-servo commented Oct 11, 2018

CYBAI commented Oct 11, 2018

bors-servo commented Oct 11, 2018

bors-servo commented Oct 11, 2018

chansuke commented Oct 6, 2018 •

edited by SimonSapin