977302 - [webvtt] Perform automatic text track selection whenever a text track is added to a media element

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

This consists of a number of steps including honoring the user preferences for automatic text track selection: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#honor-user-preferences-for-automatic-text-track-selection.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Perform automatic text track selection r=rillian.

I still have to update the TODO line with the appropriate bug number. I'll file a bug and fill it in once this gets reviewed.

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 8384071 [details] [diff] [review]
v1: Perform automatic text track selection r=rillian.

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

r=me with comments addressed. Sorry I took so long to get back to you.

::: content/html/content/src/TextTrackManager.cpp
@@ +242,5 @@
> +
> +  TextTrackKind ttKinds[] = { TextTrackKind::Captions,
> +                              TextTrackKind::Subtitles };
> +
> +  PerformTrackSelection(ttKinds, 2);

Please use 'ArrayLength(ttKinds)' instead of a literal '2' here.

@@ +271,5 @@
> +void
> +TextTrackManager::PerformTrackSelection(TextTrackKind aTextTrackKind)
> +{
> +  TextTrackKind ttKinds[] = { aTextTrackKind };
> +  PerformTrackSelection(ttKinds, 1);

ArrayLength(ttKinds)

@@ +278,5 @@
> +void
> +TextTrackManager::PerformTrackSelection(TextTrackKind aTextTrackKinds[],
> +                                        uint32_t size)
> +{
> +  nsTArray<TextTrack*> textTracks;

Might be more clear to call this variable 'candidates' to match the spec.

@@ +280,5 @@
> +                                        uint32_t size)
> +{
> +  nsTArray<TextTrack*> textTracks;
> +  GetTextTracksOfKinds(aTextTrackKinds, size, textTracks);
> +

Did you leave out step 3, "If any of the text tracks in candidates have a text track mode set to showing, abort these steps" here?

@@ +281,5 @@
> +{
> +  nsTArray<TextTrack*> textTracks;
> +  GetTextTracksOfKinds(aTextTrackKinds, size, textTracks);
> +
> +  // TODO: Bug XXX - Honor user preferences for text track selection.

Please do set a bug number here before landing.

::: content/media/test/test_texttrack.html
@@ +96,5 @@
> +        { label: "fourth", mode: "hidden"}
> +      ];
> +      is(video.textTracks.length, trackData.length, "TextTracks length should be " + trackData.length);
> +      for (var i = 0; i < trackData.length; i++) {
> +        var track = video.textTracks[i];

You can use 'let' instead of 'var' for things like this, btw. Mochitests only run in FF so newer js is fine.

Comment 3

[Rick Eyre (:reyre)]

Attachment: v2: Perform automatic text track selection r=rillian.

Thanks for review Ralph.

(In reply to Ralph Giles (:rillian) from comment #2)

> Please use 'ArrayLength(ttKinds)' instead of a literal '2' here.

Fixed.

> +  nsTArray<TextTrack*> textTracks;
> 
> Might be more clear to call this variable 'candidates' to match the spec.

Updated.

> Did you leave out step 3, "If any of the text tracks in candidates have a
> text track mode set to showing, abort these steps" here?

Ah, yes, I did. Good catch. I've updated it.

> Please do set a bug number here before landing.

Filed as bug 981691.

> You can use 'let' instead of 'var' for things like this, btw. Mochitests
> only run in FF so newer js is fine.

For some reason it didn't like it when I changed to use 'let' for the 'track' variable. Complained about a missing semi-colon.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 8388848 [details] [diff] [review]
v2: Perform automatic text track selection r=rillian.

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

r=me with comment addressed.

::: content/html/content/src/TextTrackManager.cpp
@@ +296,5 @@
> +  // Step 4: Honor user preferences for track selection, otherwise, set the
> +  // first TextTrack in candidates with a default attribute to showing.
> +  // TODO: Bug 981691 - Honor user preferences for text track selection.
> +  for (uint32_t i = 0; i < candidates.Length(); i++) {
> +    if (TrackIsDefault(candidates[i])) {

I believe you need to also check 'candidates[i]->GetMode() == TextTrackMode::Disabled' here.

Comment 5

[Rick Eyre (:reyre)]

Attachment: v3: Perform automatic text track selection r=rillian.

Just a rebase.

Carrying forward r=rillian.

Comment 6

[Rick Eyre (:reyre)]

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=a6bf1515fde1

Comment 7

[Rick Eyre (:reyre)]

Please land bug 983293 before this one.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply to inbound. Please rebase.

Comment 9

[Rick Eyre (:reyre)]

Attachment: v4: Perform automatic text track selection r=rillian.

Sorry Ryan.

Rebased so that landing on top of bug 983293 will work.

Carrying forward r=rillian.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d728a04f31b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d728a04f31b