Implement basic <media> infrastructure #8454

jdm · 2015-11-10T14:37:47Z

This gets us to the point where we can start playing with actually integrating rust-media to process the data received by the network request, as currently it's just ignored.

highfive · 2015-11-10T14:37:51Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2015-11-10T14:38:19Z

Note: the diff isn't as bad as the diffstat makes it appear, because I enabled all of the skipped WPT.

jdm · 2015-11-10T14:39:06Z

479ce84 will be omitted in favour of #8430.

eefriedman · 2015-11-10T19:38:17Z

From tidy:

./components/script/dom/htmlmediaelement.rs:10: use statement is not in alphabetical order
    expected: dom::bindings::codegen::Bindings::HTMLMediaElementBinding::HTMLMediaElementConstants::*
    found: dom::bindings::codegen::Bindings::HTMLMediaElementBinding::HTMLMediaElementMethods
./components/script/dom/htmlmediaelement.rs:11: use statement is not in alphabetical order
    expected: dom::bindings::codegen::Bindings::HTMLMediaElementBinding::HTMLMediaElementMethods
    found: dom::bindings::codegen::Bindings::HTMLMediaElementBinding::HTMLMediaElementConstants::*
./components/script/dom/htmlmediaelement.rs:375: missing space before {
./components/script/dom/htmlmediaelement.rs:491: method declared in webidl is missing a comment with a specification link
./components/script/dom/htmlmediaelement.rs:495: method declared in webidl is missing a comment with a specification link
./components/script/dom/mediaerror.rs:33: method declared in webidl is missing a comment with a specification link
./components/script/dom/webidls/MediaError.webidl:6: link to WHATWG may break in the future, use this format instead: https://html.spec.whatwg.org/multipage/#mediaerror

eefriedman · 2015-11-10T20:12:55Z

(I'm skipping reviewing 479ce84; I'm assuming all the changes are part of #8430.)

Reviewed 3 of 3 files at r1, 5 of 5 files at r2, 107 of 107 files at r3, 9 of 9 files at r4, 20 of 20 files at r5, 2 of 2 files at r6, 8 of 8 files at r7, 9 of 9 files at r8, 2 of 2 files at r9, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlmediaelement.rs, line 38 [r1] (raw file):}
Missing spec link.

^{components/script/dom/htmlmediaelement.rs, line 42 [r1] (raw file):}
Missing spec link.

^{components/script/dom/htmlmediaelement.rs, line 35 [r3] (raw file):}
Is using Trusted here really safe? There isn't anything preventing the media element from being detached from the document and garbage-collected.

^{components/script/dom/htmlmediaelement.rs, line 57 [r3] (raw file):}
This seems confusing at best; could you just store a separate bool to suppress the data_available callbacks?

^{components/script/dom/htmlmediaelement.rs, line 70 [r3] (raw file):}
Doesn't HAVE_METADATA mean you have enough data to actually determine the metadata? The payload could be as little as one byte. (A TODO would be sufficient for now.)

^{components/script/dom/htmlmediaelement.rs, line 278 [r5] (raw file):}
This can just be self.Autoplay().

^{components/script/dom/htmlmediaelement.rs, line 596 [r6] (raw file):}
This seems extremely indirect; why not just call internal_pause_steps()?

^{components/script/dom/htmlmediaelement.rs, line 375 [r10] (raw file):}
Space before brace. Also, the string comparison is a bit inefficient.

^{components/script/dom/htmlmediaelement.rs, line 510 [r10] (raw file):}
You might want to explicitly note here that "The attribute's missing value default is user-agent defined".

Comments from the review on Reviewable.io

jdm · 2015-11-10T20:56:07Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlmediaelement.rs, line 35 [r3] (raw file):}
Each Trusted value acts as a global root that prevents GC, which is the whole point of them.

^{components/script/dom/htmlmediaelement.rs, line 278 [r5] (raw file):}
The text at https://html.spec.whatwg.org/multipage/embedded-content.html#ready-states:autoplaying-flag seems a bit more complicated than that.

^{components/script/dom/htmlmediaelement.rs, line 596 [r6] (raw file):}
The spec: https://html.spec.whatwg.org/multipage/embedded-content.html#playing-the-media-resource:media-element-75

Comments from the review on Reviewable.io

eefriedman · 2015-11-10T21:11:29Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlmediaelement.rs, line 35 [r3] (raw file):}
Err, of course, sorry. I haven't had to mess with Trusted yet.

^{components/script/dom/htmlmediaelement.rs, line 278 [r5] (raw file):}
I just meant "as opposed to writing out self.upcast::<Element>().has_attribute(&Atom::from_slice("autoplay"))".

^{components/script/dom/htmlmediaelement.rs, line 596 [r6] (raw file):}
Oh, sorry, I misread await_stable_state. That makes sense.

Comments from the review on Reviewable.io

jdm · 2015-11-10T21:14:46Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlmediaelement.rs, line 278 [r5] (raw file):}
Whoops, I was looking at an earlier revision in Reviewable where this appeared to be a comment about //TODO step N (autoplay logic).

Comments from the review on Reviewable.io

eefriedman · 2015-11-10T21:23:42Z

Review status: 15 of 108 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlmediaelement.rs, line 505 [r10] (raw file):}
"src" is a URL attribute, so this needs to use make_url_getter!. (The wpt tests for url attributes don't currently work correctly on servo, so there will be test failures.)

Comments from the review on Reviewable.io

Ms2ger · 2015-11-11T11:38:16Z

I'd like to look at this PR before landing at some point.

bors-servo · 2015-11-12T23:21:33Z

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

jdm · 2016-01-04T21:10:31Z

Just rebased; no comments addressed.

bors-servo · 2016-01-10T12:01:54Z

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

jdm · 2016-02-05T22:12:00Z

Now requires servo/string-cache#139.


        Add event handlers for media-related events.


        Disable two tests that take a very long time to time out.


        Execute the failure steps for an empty src attribute.


        Implement partial spec-recommended preload=none support.


        Tidy fixes.


        Add more specification step comments and update to the latest specifi…

…cation.

highfive · 2016-05-03T22:05:40Z

New code was committed to pull request.

jdm · 2016-05-03T22:06:23Z

@bors-servo: r=KiChjang

bors-servo · 2016-05-03T22:06:24Z

📌 Commit f16c054 has been approved by KiChjang

bors-servo · 2016-05-03T23:42:46Z

⌛ Testing commit f16c054 with merge f9d9cd3...


        Auto merge of #8454 - jdm:media, r=KiChjang

Implement basic <media> infrastructure This gets us to the point where we can start playing with actually integrating rust-media to process the data received by the network request, as currently it's just ignored.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8454)

bors-servo · 2016-05-04T00:59:13Z

💔 Test failed - linux-rel

highfive · 2016-05-04T00:59:15Z

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, containerHeightStyle: &#39;400px&#39;, placeholderWidthAttr: &#39;50%&#39;, placeholderHeightAttr: &#39;100%&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Test timed out

jdm · 2016-05-04T01:59:12Z

@bors-servo: retry

#10457

bors-servo · 2016-05-04T02:40:08Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

bors-servo · 2016-05-04T03:26:46Z

💔 Test failed - linux-rel

highfive · 2016-05-04T03:26:48Z

  ▶ TIMEOUT [expected OK] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html
  │ 
  └ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.

  ▶ Unexpected subtest result in /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html:
  │ TIMEOUT [expected PASS] placeholder: &#39;iframe&#39;, containerWidthStyle: &#39;400px&#39;, placeholderWidthAttr: &#39;100&#39;, placeholderHeightAttr: &#39;100%&#39;, svgViewBoxAttr: &#39;0 0 100 200&#39;, svgWidthAttr: &#39;25%&#39;, svgHeightAttr: &#39;25%&#39;, 
  └   → Te</span><span class="stdout">st timed out

cbrewster · 2016-05-04T04:01:10Z

@bors-servo retry

#10457

bors-servo · 2016-05-04T04:13:00Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

bors-servo · 2016-05-04T05:00:30Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive added the S-awaiting-review label Nov 10, 2015

eefriedman self-assigned this Nov 10, 2015

jdm added the S-fails-tidy label Nov 10, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Nov 12, 2015

eefriedman added S-needs-code-changes and removed S-awaiting-review labels Jan 3, 2016

jdm force-pushed the jdm:media branch from aea5f9e to 709c481 Jan 4, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 4, 2016

jdm removed S-fails-tidy S-needs-rebase labels Jan 4, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 4, 2016

highfive added the S-needs-rebase label Jan 10, 2016

jdm force-pushed the jdm:media branch from 750c935 to eed7c57 Feb 5, 2016

jdm added 6 commits Nov 7, 2015

Add event handlers for media-related events.

a80320e

Disable two tests that take a very long time to time out.

c64d757

Execute the failure steps for an empty src attribute.

ef10393

Implement partial spec-recommended preload=none support.

f78d88a

Tidy fixes.

99f6fb3

Add more specification step comments and update to the latest specifi…

Loading status checks…

f16c054

…cation.

jdm force-pushed the jdm:media branch from f8baa34 to f16c054 May 3, 2016

highfive added S-awaiting-review and removed S-tests-failed labels May 3, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 3, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 4, 2016

highfive added the S-tests-failed label May 4, 2016

bors-servo merged commit f16c054 into servo:master May 4, 2016
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

highfive removed the S-awaiting-merge label May 4, 2016

servo / servo

Implement basic <media> infrastructure #8454

Implement basic <media> infrastructure #8454

jdm commented Nov 10, 2015

highfive commented Nov 10, 2015

jdm commented Nov 10, 2015

jdm commented Nov 10, 2015

eefriedman commented Nov 10, 2015 •

edited by jdm

eefriedman commented Nov 10, 2015

jdm commented Nov 10, 2015

eefriedman commented Nov 10, 2015

jdm commented Nov 10, 2015

eefriedman commented Nov 10, 2015

Ms2ger commented Nov 11, 2015

bors-servo commented Nov 12, 2015

jdm commented Jan 4, 2016

bors-servo commented Jan 10, 2016

jdm commented Feb 5, 2016

highfive commented May 3, 2016

jdm commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 4, 2016

highfive commented May 4, 2016

jdm commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

highfive commented May 4, 2016

cbrewster commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

servo / servo

Join GitHub today

Implement basic <media> infrastructure #8454

Implement basic <media> infrastructure #8454

Conversation

jdm commented Nov 10, 2015

highfive commented Nov 10, 2015

jdm commented Nov 10, 2015

jdm commented Nov 10, 2015

eefriedman commented Nov 10, 2015 • edited by jdm

eefriedman commented Nov 10, 2015

jdm commented Nov 10, 2015

eefriedman commented Nov 10, 2015

jdm commented Nov 10, 2015

eefriedman commented Nov 10, 2015

Ms2ger commented Nov 11, 2015

bors-servo commented Nov 12, 2015

jdm commented Jan 4, 2016

bors-servo commented Jan 10, 2016

jdm commented Feb 5, 2016

highfive commented May 3, 2016

jdm commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 4, 2016

highfive commented May 4, 2016

jdm commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

highfive commented May 4, 2016

cbrewster commented May 4, 2016

bors-servo commented May 4, 2016

bors-servo commented May 4, 2016

eefriedman commented Nov 10, 2015 •

edited by jdm