RESOLVED FIXED 113062
Cleanup text track selection logic
https://bugs.webkit.org/show_bug.cgi?id=113062
Summary Cleanup text track selection logic
Eric Carlson
Reported 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.
Attachments
Proposed patch (19.56 KB, patch)
2013-03-22 10:25 PDT, Eric Carlson
webkit-ews: commit-queue-
Updated patch. (19.57 KB, patch)
2013-03-22 10:46 PDT, Eric Carlson
webkit-ews: commit-queue-
Updated patch. (19.70 KB, patch)
2013-03-22 11:17 PDT, Eric Carlson
no flags
Radar WebKit Bug Importer
Comment 1 2013-03-22 08:59:00 PDT
Eric Carlson
Comment 2 2013-03-22 10:25:54 PDT
Created attachment 194584 [details] Proposed patch
Early Warning System Bot
Comment 3 2013-03-22 10:32:07 PDT
EFL EWS Bot
Comment 4 2013-03-22 10:36:09 PDT
WebKit Review Bot
Comment 5 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
Peter Beverloo (cr-android ews)
Comment 6 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
Early Warning System Bot
Comment 7 2013-03-22 10:42:32 PDT
Eric Carlson
Comment 8 2013-03-22 10:46:14 PDT
Created attachment 194591 [details] Updated patch.
Early Warning System Bot
Comment 9 2013-03-22 10:51:25 PDT
Build Bot
Comment 10 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
Early Warning System Bot
Comment 11 2013-03-22 11:01:10 PDT
Eric Carlson
Comment 12 2013-03-22 11:17:38 PDT
Created attachment 194603 [details] Updated patch.
Jer Noble
Comment 13 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?
Eric Carlson
Comment 14 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!
Eric Carlson
Comment 15 2013-03-22 13:28:43 PDT
Peter Kasting
Comment 16 2013-03-22 16:40:07 PDT
This seems to have caused bug 113114.
Note You need to log in before you can comment on or make changes to this bug.