Bug 113062 – Cleanup text track selection logic

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Some text track selection logic has been moved from HTMLMediaElement to CaptionUserPreferences to make it easier for ports to customize. More should be moved to clean things up.

[Radar WebKit Bug Importer]

<rdar://problem/13483645>

[Eric Carlson]

Created attachment 194584 [details]
Proposed patch

[Early Warning System Bot]

Comment on attachment 194584 [details]
Proposed patch

Attachment 194584 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17133578

[EFL EWS Bot]

Comment on attachment 194584 [details]
Proposed patch

Attachment 194584 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17133581

[WebKit Review Bot]

Comment on attachment 194584 [details]
Proposed patch

Attachment 194584 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17240319

[Peter Beverloo (cr-android ews)]

Comment on attachment 194584 [details]
Proposed patch

Attachment 194584 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17133579

[Early Warning System Bot]

Comment on attachment 194584 [details]
Proposed patch

Attachment 194584 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17133587

[Eric Carlson]

Created attachment 194591 [details]
Updated patch.

[Early Warning System Bot]

Comment on attachment 194591 [details]
Updated patch.

Attachment 194591 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17145859

[Build Bot]

Comment on attachment 194591 [details]
Updated patch.

Attachment 194591 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17295054

[Early Warning System Bot]

Comment on attachment 194591 [details]
Updated patch.

Attachment 194591 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17187834

[Eric Carlson]

Created attachment 194603 [details]
Updated patch.

[Jer Noble]

Comment on attachment 194603 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=194603&action=review

r+, with a couple of nits.

> Source/WebCore/page/CaptionUserPreferences.cpp:80
> +        setUserPrefersCaptions(false);
> +        setUserPrefersCaptions(false);

Should the second line be "setUserPrefersSubtitles(false)"?

> Source/WebCore/page/CaptionUserPreferences.cpp:220
> +    return (languages.size() - languageMatchIndex) * 10;

Why 10?  This could be a constant which explains its importance.

> Source/WebCore/page/CaptionUserPreferencesMac.mm:634
> +        if (track->kind() == track->subtitlesKeyword())
> +            trackScore = 3;
> +        else {
> +            if (!track->isClosedCaptions())
> +                trackScore = 2;
> +            else
> +                trackScore = 1;
> +        }
> +    }

This is confusingly structured.  Could you make this an 'if...else if... else' instead?

[Eric Carlson]

(In reply to comment #13)
> (From update of attachment 194603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194603&action=review
> 
> r+, with a couple of nits.
> 
> > Source/WebCore/page/CaptionUserPreferences.cpp:80
> > +        setUserPrefersCaptions(false);
> > +        setUserPrefersCaptions(false);
> 
> Should the second line be "setUserPrefersSubtitles(false)"?
> 
  Oops, thanks!

> > Source/WebCore/page/CaptionUserPreferences.cpp:220
> > +    return (languages.size() - languageMatchIndex) * 10;
> 
> Why 10?  This could be a constant which explains its importance.
> 
  I added a comment to clarify.

> > Source/WebCore/page/CaptionUserPreferencesMac.mm:634
> > +        if (track->kind() == track->subtitlesKeyword())
> > +            trackScore = 3;
> > +        else {
> > +            if (!track->isClosedCaptions())
> > +                trackScore = 2;
> > +            else
> > +                trackScore = 1;
> > +        }
> > +    }
> 
> This is confusingly structured.  Could you make this an 'if...else if... else' instead?
>
  Fixed

Thanks!

[Eric Carlson]

https://trac.webkit.org/r146647

[Peter Kasting]

This seems to have caused bug 113114.