909993 - [webvtt] Implement TextTrackCue as an abstract class

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

We have VTTCue implemented now which is a merged form of TextTrackCue and VTTCue in the track and WebVTT specs. To implement this correctly we need to implement TextTrackCue as an abstract class.

Comment 1

[Rick Eyre (:reyre)]

I'm getting more and more skeptical on whether or not we actually need to do this in the future. There aren't any new Cue types being added to the HTML 5. DataCue, the only other Cue type that has been added, is currently at risk [1]. If HTML 5 moves to recommendation without DataCue then I think we should close this bug.

http://www.w3.org/html/wg/wiki/HTML5.0AtRiskFeatures

Comment 2

[Brendan Long]

Shouldn't we at least make TextTrackCue an interface so `cue instanceof TextTrackCue` will work?

Comment 3

[Brendan Long]

Attachment: 0001-Add-WebIDL-for-TextTrackCue-and-fix-link-in-VTTCue.w.patch

The patch to implement this is pretty simple. The main differences this cause in JavaScript are:

  - "TextTrackCue" exists.
  - `new VTTCue(...) instanceof TextTrackCue` is `true`.
  - Cues have the correct prototype chain

I'm working on a polyfill to create another cue type, and it doesn't work right when TextTrackCue doesn't exist.

Comment 4

[Brendan Long]

Attachment: 0001-Add-WebIDL-for-TextTrackCue-and-fix-link-in-VTTCue.w.patch

Forgot to fix the tests. It's odd that there was previously a test guaranteeing that TextTrackCue is undefined :\

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8633739 [details] [diff] [review]
0001-Add-WebIDL-for-TextTrackCue-and-fix-link-in-VTTCue.w.patch

Please use the checkin-needed bug keyword to request checkin once this patch has gotten reviewed.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8633739 [details] [diff] [review]
0001-Add-WebIDL-for-TextTrackCue-and-fix-link-in-VTTCue.w.patch

Review of attachment 8633739 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this. Looks good, r=me for media with the whatwg citation updated.

The WebIDL changes also need dom peer review. Adding bz for that.

::: dom/webidl/TextTrackCue.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://www.whatwg.org/specs/web-apps/current-work/#texttrackcue

Current url for this is https://html.spec.whatwg.org/#texttrackcue

Comment 7

[Ralph Giles (:rillian)]

Pushed to try in https://treeherder.mozilla.org/#/jobs?repo=try&revision=cad9a9e9ed04

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8633739 [details] [diff] [review]
0001-Add-WebIDL-for-TextTrackCue-and-fix-link-in-VTTCue.w.patch

This pref is enabled by default, right?  Don't you need to update dom/tests/mochitest/general/test_interfaces.html accordingly?

r=me assuming we've decided to add this interface....

Comment 9

[Ralph Giles (:rillian)]

Yes, webvtt is enabled by default. As the try build shows, test_interfaces needs updating.

Brendan, please update your patch to pass test_interfaces.html and attach it here. That version should be ready to land. Need-info me or add 'checkin-needed' to the keyword field when you're ready. You can verify locally with:

  ./mach mochitest dom/tests/mochitest/general/test_interfaces.html

Also, the first line of your commit message needs to reference the bug number and reviewers. E.g.:

Bug 909993 - Add WebIDL for TextTrackCue and fix link in VTTCue.webidl. r=rillian,bz

Comment 10

[Brendan Long]

Attachment: 0001-Bug-909993-Add-WebIDL-for-TextTrackCue-and-fix-link-.patch

Updated the VTTCue WebIDL link, fixed test_interfaces, and changed the commit message.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa834e98383

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for WPT-2 test failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a0360b762d

https://treeherder.mozilla.org/logviewer.html#?job_id=12219076&repo=mozilla-inbound

Comment 13

[Ralph Giles (:rillian)]

Oops. Didn't think to run these on try. Sorry.

The good news is that the problems are all expected-fail tests we now pass, so you just need to update the web platform tests to reflect that we now comply with the spec. E.g. in https://github.com/mozilla/gecko-dev/blob/master/testing/web-platform/meta/html/dom/interfaces.html.ini#L1276

You can verify locally with './mach web-platform-tests'.

Comment 14

[Brendan Long]

I just want to mention why this test was failing since it might be confusing:

14:51:57 INFO - TEST-UNEXPECTED-PASS | /html/semantics/embedded-content/media-elements/historical.html | TextTrackCue constructor should not be supported - expected FAIL 

The TextTrackCue constructor still doesn't exist, but now it (correctly) throws a TypeError instead of a ReferenceError.

Comment 15

[Brendan Long]

I fixed the two .ini files, and I'm rebuilding to make sure this still applies well and I'll upload again after lunch.

Comment 16

[Brendan Long]

Attachment: 0001-Bug-909993-Add-WebIDL-for-TextTrackCue-and-fix-link-.patch

Ok, I fixed the two .ini files and re-ran the web-platform-tests in html/semantics and html/dom (running all of them would take days on my laptop...).

Comment 17

[Ralph Giles (:rillian)]

Thanks. New try push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=766408b3bcf3

Comment 18

[Ralph Giles (:rillian)]

Green on try with mochitests and web platform tests.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17766cdd3de4

Comment 20

[Brendan Long]

This bug is fixed, but for some reason I don't see a RESOLVED: FIXED option for the status.

Comment 21

[Boris Zbarsky [:bzbarsky]]

This bug is not fixed because it hasn't merged to the main integration branch yet.  When it does that, it will be marked fixed.

I've changed your bugzilla permissions so you should be able to modify the state of bugs arbitrarily; you should now see a "FIXED" option.

Comment 22

[Brendan Long]

Oops I didn't mean to change the resolved state when I made that comment. I'll just wait for it to be merged then. Thanks for your help!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17766cdd3de4