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

Add the basics of TextTrack #22392

Merged
merged 2 commits into from Dec 11, 2018

Conversation

Projects
None yet
6 participants
@dlrobertson
Copy link
Contributor

commented Dec 9, 2018

Summary

Implement the basics of the TextTrack elements:

  • TextTrack

  • TextTrackCue

  • TextTrackCueList

  • TextTrackList

  • ./mach build -d does not report any errors

  • ./mach test-tidy does not report any errors

  • These changes is related to #22311

  • There are tests for these changes


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Dec 9, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/TextTrackCueList.webidl, components/script/dom/htmlmediaelement.rs, components/script/dom/mod.rs, components/script/dom/texttrack.rs, components/script/dom/texttracklist.rs and 6 more
  • @KiChjang: components/script/dom/webidls/TextTrackCueList.webidl, components/script/dom/htmlmediaelement.rs, components/script/dom/mod.rs, components/script/dom/texttrack.rs, components/script/dom/texttracklist.rs and 6 more
@dlrobertson
Copy link
Contributor Author

left a comment

I can remove the TextTrackCue and TextTrackCueList implementation from this PR and submit it in a follow up if it helps.

// Step 3
self.text_tracks().add(&track);
// Step 4: FIXME We need to queue a event here, but the
// addtrack atom does not exist

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

How can I create addtrack as an Atom?

This comment has been minimized.

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

Awesome! Thanks.


// https://html.spec.whatwg.org/multipage/#dom-texttrackcuelist-getcuebyid
fn GetCueById(&self, id: DOMString) -> Option<DomRoot<TextTrackCue>> {
if String::from(id.clone()).is_empty() {

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

Is there a better way to determine that the DOMString is empty?

This comment has been minimized.

Copy link
@jdm

jdm Dec 9, 2018

Member

id.is_empty()?

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

😧 don't know how I missed that.

.borrow()
.iter()
.enumerate()
.filter(|(_, t)| t.id() == track.id())

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

Again, not correct.

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

Updated to use == which after looking at gecko I think is the right thing to do.

.borrow()
.iter()
.enumerate()
.filter(|(_, c)| c.id() == cue.id())

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

This is not correct, and is not what gecko does.

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

Same.

readonly attribute TextTrackCueList? cues;
// readonly attribute TextTrackCueList? activeCues;

[Throws]

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

Note the IDL does not include Throws, but addCue can throw a "InvalidStateError" in step 2 and removeCue can throw a "NotFoundError" in step 1.

This comment has been minimized.

Copy link
@jdm

jdm Dec 9, 2018

Member

Yeah, [Throws] is a Gecko/Servo-specific extension.

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 9, 2018

Author Contributor

👍 Thanks

@jdm

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

r? @ferjm

@highfive highfive assigned ferjm and unassigned asajeffrey Dec 9, 2018

@dlrobertson dlrobertson force-pushed the dlrobertson:add_text_track branch from 8bddd93 to 64573d5 Dec 9, 2018

event_handler!(onchange, GetOnaddtrack, SetOnaddtrack);

// https://html.spec.whatwg.org/multipage/#handler-texttracklist-onremovetrack
event_handler!(onchange, GetOnremovetrack, SetOnremovetrack);

This comment has been minimized.

Copy link
@jdm

jdm Dec 9, 2018

Member

These are using onchange, which probably explains why a few test failures remain in the event handling tests.

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 10, 2018

Author Contributor

Good catch. TrackEvent hasn't been implemented yet, so we'll likely have to implement that in order to get these to work correctly right? I haven't looked into the internals of events yet, so I'm very likely wrong 😄

@dlrobertson dlrobertson force-pushed the dlrobertson:add_text_track branch from 64573d5 to 4e36f33 Dec 10, 2018

@ferjm

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

⌛️ Trying commit 4e36f33 with merge 7f400d1...

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

Auto merge of #22392 - dlrobertson:add_text_track, r=<try>
Add the basics of TextTrack

## Summary

Implement the basics of the TextTrack elements:
  - TextTrack
  - TextTrackCue
  - TextTrackCueList
  - TextTrackList

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes is related to #22311
- [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/22392)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

💔 Test failed - linux-rel-css

@ferjm

ferjm approved these changes Dec 11, 2018

Copy link
Member

left a comment

Wonderfull work!

I left some comments for minor stuff.

I believe the only thing left is to update the test expectations for tests/wpt/mozilla/tests/mozilla/interfaces.html. And getting @jdm's approval for the new exposed interfaces.

/// associated [textTracks].
///
/// [textTracks]: https://html.spec.whatwg.org/multipage/#dom-media-texttracks
pub fn text_tracks(&self) -> DomRoot<TextTrackList> {

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

This does not need to be pubic.


// https://html.spec.whatwg.org/multipage/#dom-media-texttracks
fn TextTracks(&self) -> DomRoot<TextTrackList> {
self.text_tracks()

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

nit: could we inline the content of the text_tracks function here and get rid of that function? We can call self.TextTracks() from AddTextTrack

// FIXME set the ready state to Loaded
let track = TextTrack::new(&window, kind, label, language, TextTrackMode::Hidden);
// Step 3 & 4
text_tracks.add(&track);

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

nit: self.TextTracks().add(&track)

kind: kind,
label: label.into(),
language: language.into(),
id: "".to_owned(),

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

I suspect that we need to get the id property as an argument of new and new_inherited as well, so we can assign the proper id when creating a TextTrack associated to a HTMLTrackElement.

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Dec 11, 2018

Author Contributor

I wasn't sure what to do here. The spec states.

the track's identifier, if it has one, or the empty string otherwise

But in gecko the TextTrack class does not have an id member. For now I've added id to new and new_inherited

// https://html.spec.whatwg.org/multipage/#dom-texttrack-cues
fn GetCues(&self) -> Option<DomRoot<TextTrackCueList>> {
// FIXME if text track mode is disabled, return None
Some(self.get_cues())

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member
match self.Mode() {
   TextTrackMode::Disabled => None,
    _ => Some(self.get_cues()),
}
let window = window_from_node(self);
let text_tracks = self.text_tracks();
// Step 1 & 2
// FIXME set the ready state to Loaded

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

Until we have a more specific issue, reference issue #22314 here, please.


// https://html.spec.whatwg.org/multipage/#dom-texttrack-addcue
fn AddCue(&self, cue: &TextTrackCue) -> ErrorResult {
// FIXME add Step 1 & 2

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

Until we have a more specific issue, reference issue #22314 here, please.

// gecko calls RemoveCue when the given cue
// has an associated track, but doesn't return
// the error from it, so we wont either.
let _ = old_track.RemoveCue(cue);

This comment has been minimized.

Copy link
@ferjm

ferjm Dec 11, 2018

Member

The spec says nothing about returning an error if removing the cue fails, so I think this behavior is fine. However, if removing the cue fails, that means that our implementation is buggy, so we should either add a warn! message or a debug_assert here.

@dlrobertson dlrobertson force-pushed the dlrobertson:add_text_track branch from 4e36f33 to 94eec96 Dec 11, 2018

@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

@ferjm updated

@ferjm

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Thanks! r=me

@jdm r? for the new exposed interfaces.

@jdm

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@bors-servo r=ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

📌 Commit 94eec96 has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

⌛️ Testing commit 94eec96 with merge aae005c...

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

Auto merge of #22392 - dlrobertson:add_text_track, r=ferjm
Add the basics of TextTrack

## Summary

Implement the basics of the TextTrack elements:
  - TextTrack
  - TextTrackCue
  - TextTrackCueList
  - TextTrackList

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes is related to #22311
- [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/22392)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

💔 Test failed - linux-rel-wpt

@dlrobertson dlrobertson force-pushed the dlrobertson:add_text_track branch from 94eec96 to 86a9bfd Dec 11, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

⌛️ Testing commit 86a9bfd with merge fe15978...

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

Auto merge of #22392 - dlrobertson:add_text_track, r=ferjm
Add the basics of TextTrack

## Summary

Implement the basics of the TextTrack elements:
  - TextTrack
  - TextTrackCue
  - TextTrackCueList
  - TextTrackList

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes is related to #22311
- [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/22392)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

💔 Test failed - linux-rel-css

dlrobertson added some commits Dec 8, 2018

script: Create structures for TextTrack API
Fill out the basics for the WebIDLs for the following:

  - TextTrack
  - TextTrackCue
  - TextTrackCueList
  - TextTrackList
update-wpt: TextTrack tests
Update the expectations of TextTrack related tests.

@dlrobertson dlrobertson force-pushed the dlrobertson:add_text_track branch from 86a9bfd to eb531f6 Dec 11, 2018

@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

The tests don't fail on my setup. Is it an intermittent? I also just rebased on the current master

@ferjm

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

They indeed look like intermittents. Filed #22422 and #22423.

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

📌 Commit eb531f6 has been approved by ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

⌛️ Testing commit eb531f6 with merge 90fc9e2...

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

Auto merge of #22392 - dlrobertson:add_text_track, r=ferjm
Add the basics of TextTrack

## Summary

Implement the basics of the TextTrack elements:
  - TextTrack
  - TextTrackCue
  - TextTrackCueList
  - TextTrackList

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes is related to #22311
- [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/22392)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

💔 Test failed - mac-rel-css1

@jdm

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@bors-servo

This comment has been minimized.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

@bors-servo bors-servo merged commit eb531f6 into servo:master Dec 11, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@dlrobertson dlrobertson deleted the dlrobertson:add_text_track branch Dec 12, 2018

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.