981691 - [webvtt] Honor user preferences for TextTrack selection based on selected language

Status: NEW

(Core :: Audio/Video: Playback, defect, P3)

Description

[Rick Eyre (:reyre)]

Step 4 in 'performing automatic track selection' says to honor any user preferences that might have been set when selecting text tracks. For example that could be a preferred language, or TextTrack kind such as commentary or descriptions, etc.

See http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#perform-automatic-text-track-selection for details.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 981691 - perform automatic text track selection step 4. r=rillian

Review commit: https://reviewboard.mozilla.org/r/55370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55370/

Comment 2

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/55370/#review52114

Hi Ralph,
The patch here I only check the accept language in preference. I don't understand what's the "commentary" in the spec. Should I also check the "commentary" keyword in the label or src attribute?

Comment 3

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/55370/#review52116

::: dom/html/TextTrackManager.cpp:372
(Diff revision 1)
> +  nsTArray<nsString> acceptLanguages;
> +  nsTArray<TextTrack*> possibleCandidates;
> +  Navigator::GetAcceptLanguages(acceptLanguages);
> +  for (uint32_t i = 0; i < acceptLanguages.Length(); i++) {
> +    for (uint32_t j = 0; j < candidates.Length(); j++) {
> +      if (TrackIsDefault(candidates[j])) {

I should not check the default attribute.

Comment 4

[Ralph Giles (:rillian)]

Updated spec link is https://html.spec.whatwg.org/multipage/embedded-content.html#honor-user-preferences-for-automatic-text-track-selection

(In reply to Benjamin Chen [:bechen] from comment #2)

> I don't understand what's the "commentary" in the spec.

I think that you're right that the 'title' they refer to in the "commentary" example is the `label` attribute. But setting a preference for something like that requires UX involvement and is out of the scope for your change here. I think selecting based on AcceptLanguages is a good start until we have controls for selecting text tracks (bug 887934) as which point we could add some kind of frecency matching.

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8756747 [details]
MozReview Request: Bug 981691 - perform automatic text track selection step 4. r=rillian

https://reviewboard.mozilla.org/r/55370/#review52314

::: dom/html/TextTrackManager.cpp:367
(Diff revision 1)
>    // 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.
> +  // TODO: Commentary?
> +  // Compare language.
> +  nsTArray<nsString> acceptLanguages;

You can remove both TODOs here. I'd just say,

```// Compare language against user preferences.```

::: dom/html/TextTrackManager.cpp:372
(Diff revision 1)
> +  nsTArray<nsString> acceptLanguages;
> +  nsTArray<TextTrack*> possibleCandidates;
> +  Navigator::GetAcceptLanguages(acceptLanguages);
> +  for (uint32_t i = 0; i < acceptLanguages.Length(); i++) {
> +    for (uint32_t j = 0; j < candidates.Length(); j++) {
> +      if (TrackIsDefault(candidates[j])) {

nsTArray<>::Length() is a size_t. Please use auto or size_t for the loop variable.

::: dom/html/TextTrackManager.cpp:373
(Diff revision 1)
> +  nsTArray<TextTrack*> possibleCandidates;
> +  Navigator::GetAcceptLanguages(acceptLanguages);
> +  for (uint32_t i = 0; i < acceptLanguages.Length(); i++) {
> +    for (uint32_t j = 0; j < candidates.Length(); j++) {
> +      if (TrackIsDefault(candidates[j])) {
> +        nsString tl;

As you say, you shouldn't check Default here. Instead you can rely on the loop at the end of the function to set the first default track to showing since it's only reachable if `possibleCandidates` is empty.