Bug 113062 - Cleanup text track selection logic
Summary: Cleanup text track selection logic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on: 113114
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-22 08:17 PDT by Eric Carlson
Modified: 2013-03-22 16:40 PDT (History)
14 users (show)

See Also:


Attachments
Proposed patch (19.56 KB, patch)
2013-03-22 10:25 PDT, Eric Carlson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (19.57 KB, patch)
2013-03-22 10:46 PDT, Eric Carlson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (19.70 KB, patch)
2013-03-22 11:17 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-03-22 08:17:39 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-03-22 08:59:00 PDT
<rdar://problem/13483645>
Comment 2 Eric Carlson 2013-03-22 10:25:54 PDT
Created attachment 194584 [details]
Proposed patch
Comment 3 Early Warning System Bot 2013-03-22 10:32:07 PDT
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
Comment 4 EFL EWS Bot 2013-03-22 10:36:09 PDT
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
Comment 5 WebKit Review Bot 2013-03-22 10:39:09 PDT
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
Comment 6 Peter Beverloo (cr-android ews) 2013-03-22 10:40:12 PDT
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
Comment 7 Early Warning System Bot 2013-03-22 10:42:32 PDT
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
Comment 8 Eric Carlson 2013-03-22 10:46:14 PDT
Created attachment 194591 [details]
Updated patch.
Comment 9 Early Warning System Bot 2013-03-22 10:51:25 PDT
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
Comment 10 Build Bot 2013-03-22 10:54:04 PDT
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
Comment 11 Early Warning System Bot 2013-03-22 11:01:10 PDT
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
Comment 12 Eric Carlson 2013-03-22 11:17:38 PDT
Created attachment 194603 [details]
Updated patch.
Comment 13 Jer Noble 2013-03-22 12:17:42 PDT
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?
Comment 14 Eric Carlson 2013-03-22 13:28:07 PDT
(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!
Comment 15 Eric Carlson 2013-03-22 13:28:43 PDT
https://trac.webkit.org/r146647
Comment 16 Peter Kasting 2013-03-22 16:40:07 PDT
This seems to have caused bug 113114.