Open Bug 981691 Opened 8 years ago Updated 2 years ago

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

Categories

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

defect

Tracking

()

People

(Reporter: reyre, Assigned: alwu)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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.
Component: Audio/Video → Audio/Video: Playback
Assignee: nobody → 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?
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.
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 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.
Attachment #8756747 - Flags: review?(giles)
Priority: -- → P5
Summary: [WebVTT] Honor user preferences for TextTrack selection → [WebVTT] Honor user preferences for TextTrack selection based on selected language
Assignee: bechen → alwu
Priority: P5 → P3
Summary: [WebVTT] Honor user preferences for TextTrack selection based on selected language → [webvtt] Honor user preferences for TextTrack selection based on selected language
You need to log in before you can comment on or make changes to this bug.