WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2013-03-22 08:59:00 PDT
<
rdar://problem/13483645
>
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
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 4
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
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
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
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
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 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
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
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
https://trac.webkit.org/r146647
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.
Top of Page
Format For Printing
XML
Clone This Bug