Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1118613002:
Hook up Android closed captions 'enabled' setting to Blink (Closed)
|
Created:
5 years, 7 months ago by srivats Modified:
5 years, 5 months ago CC:
blink-reviews, sof, arv+blink, blink-reviews-dom_chromium.org, vivekg, gasubic, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vivekg_samsung, Inactive, blink-reviews-html_chromium.org, vcarbune.chromium, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionHook up Android closed captions 'enabled' setting to Blink
Plumb down captions state setting from platform to media element in Blink.
Register the media element to listen for changes to the platform setting
and act on it immediately.
Influence automatic track selection to prefer captions over subtitles when the platform setting indicates a preference for captions.
Chromium-side CL: https://codereview.chromium.org/1110103004/
BUG=457850, 388588
TEST=media*
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198661
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments from tkent #
Total comments: 1
Patch Set 3 : Have the flag influence automatic track selection #
Total comments: 7
Patch Set 4 : Updated automatic track selection logic #
Total comments: 5
Patch Set 5 : Addressed comment from fs #
Total comments: 16
Patch Set 6 : Addressed comments from fs #
Total comments: 34
Patch Set 7 : Addressed nits and some comments #Patch Set 8 : Prioritize language over kind in auto track selection #
Total comments: 6
Patch Set 9 : Rebase and addressed lgtm nits #
Total comments: 12
Patch Set 10 : Addressed Philip's comments #
Total comments: 1
Patch Set 11 : Addressed lgtm comment and rebased #Messages
Total messages: 43 (5 generated)
srivats@amazon.com changed reviewers: + tkent@chromium.org
Hi tkent, This is the Blink-side implementation for hooking up the Android platform closed captions state with the MediaElement in Blink. Here's the Chromium-side CL: https://codereview.chromium.org/1110103004/ PTAL Thanks, Sunitha
tkent@chromium.org changed reviewers: + fs@opera.com, philipj@opera.com
+fs and philipj
https://codereview.chromium.org/1118613002/diff/1/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/1118613002/diff/1/Source/core/dom/Document.h#... Source/core/dom/Document.h:1420: Vector<HTMLMediaElement*> m_textTracksVisibilityChangedElements; Setting change won't happen so frequently, and doesn't need to be fast. Please don't keep the list of HTMLMediaElement. https://codereview.chromium.org/1118613002/diff/1/Source/core/frame/Settings.h File Source/core/frame/Settings.h (right): https://codereview.chromium.org/1118613002/diff/1/Source/core/frame/Settings.... Source/core/frame/Settings.h:72: void setTextTracksEnabled(bool); No need to add manually-written functions. Add the following line to Settings.in: textTrackEnabled initial=false invalidate=TextTracksVisibilityChange https://codereview.chromium.org/1118613002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:379: #if OS(ANDROID) Can you set the default value of textTracksEnabled "true", and avoid |#if OS(ANDROID)|? https://codereview.chromium.org/1118613002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:383: setClosedCaptionsVisible(document.settings()->textTracksEnabled()); document.settings() can be NULL.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1118613002/diff/1/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/1118613002/diff/1/Source/core/dom/Document.h#... Source/core/dom/Document.h:1420: Vector<HTMLMediaElement*> m_textTracksVisibilityChangedElements; On 2015/04/29 23:58:44, Slow until end of May wrote: > Setting change won't happen so frequently, and doesn't need to be fast. > Please don't keep the list of HTMLMediaElement. Done. https://codereview.chromium.org/1118613002/diff/1/Source/core/frame/Settings.h File Source/core/frame/Settings.h (right): https://codereview.chromium.org/1118613002/diff/1/Source/core/frame/Settings.... Source/core/frame/Settings.h:72: void setTextTracksEnabled(bool); On 2015/04/29 23:58:44, Slow until end of May wrote: > No need to add manually-written functions. > Add the following line to Settings.in: > textTrackEnabled initial=false invalidate=TextTracksVisibilityChange Done. https://codereview.chromium.org/1118613002/diff/1/Source/core/html/HTMLMediaE... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:379: #if OS(ANDROID) On 2015/04/29 23:58:44, Slow until end of May wrote: > Can you set the default value of textTracksEnabled "true", and avoid |#if > OS(ANDROID)|? Doing that will have closed captions turned on by default for videos with tracks on all platforms and also breaks some media layout tests. https://codereview.chromium.org/1118613002/diff/1/Source/core/html/HTMLMediaE... Source/core/html/HTMLMediaElement.cpp:383: setClosedCaptionsVisible(document.settings()->textTracksEnabled()); On 2015/04/29 23:58:44, Slow until end of May wrote: > document.settings() can be NULL. Done.
Please expand on the intended semantics of this flag/setting - it seems to be too "wide" for something that has "captions" in the name. Analogously the flag appears misnamed (sounds almost like a feature-flag - which it really shouldn't IMO.) Should this rather just influence automatic track selection, and as such prefer 'captions' kinds (over 'subtitles')? https://codereview.chromium.org/1118613002/diff/40001/LayoutTests/media/text-... File LayoutTests/media/text-tracks-visibility-user-override.html (right): https://codereview.chromium.org/1118613002/diff/40001/LayoutTests/media/text-... LayoutTests/media/text-tracks-visibility-user-override.html:25: checkCaptionsDisplay(); Looks like this still tests for "Lorem"? (Still "passes" because textTrackDisplayElement(...) throws I suppose, but seems a bit weird.)
On 2015/05/04 14:58:26, fs wrote: > Please expand on the intended semantics of this flag/setting - it seems to be > too "wide" for something that has "captions" in the name. Analogously the flag > appears misnamed (sounds almost like a feature-flag - which it really shouldn't > IMO.) > How about subtitlesAndCaptionsVisibility or just closedCaptionsVisibility? The flag is used to support Android's system-wide setting for captions visibility. https://developer.android.com/about/versions/kitkat.html#44-accessibility I agree that appending "enabled" to the flag makes it sound like a feature flag and using "visible" might be more appropriate here. > Should this rather just influence automatic track selection, and as such prefer > 'captions' kinds (over 'subtitles')? > Changed it to influence the automatic track selection and it has to influence both subtitles and captions. > https://codereview.chromium.org/1118613002/diff/40001/LayoutTests/media/text-... > File LayoutTests/media/text-tracks-visibility-user-override.html (right): > > https://codereview.chromium.org/1118613002/diff/40001/LayoutTests/media/text-... > LayoutTests/media/text-tracks-visibility-user-override.html:25: > checkCaptionsDisplay(); > Looks like this still tests for "Lorem"? (Still "passes" because > textTrackDisplayElement(...) throws I suppose, but seems a bit weird.) I ended up checking captions display this way based off of media/video-controls-captions-multiple-clicks.html.
On 2015/05/05 01:38:50, srivats wrote: > On 2015/05/04 14:58:26, fs wrote: > > Please expand on the intended semantics of this flag/setting - it seems to be > > too "wide" for something that has "captions" in the name. Analogously the flag > > appears misnamed (sounds almost like a feature-flag - which it really > shouldn't > > IMO.) > > > > How about subtitlesAndCaptionsVisibility or just closedCaptionsVisibility? > The flag is used to support Android's system-wide setting for captions > visibility. > https://developer.android.com/about/versions/kitkat.html#44-accessibility > > I agree that appending "enabled" to the flag makes it sound like a feature flag > and using "visible" might be more appropriate here. Thanks for the link. > > Should this rather just influence automatic track selection, and as such > prefer > > 'captions' kinds (over 'subtitles')? > > > > Changed it to influence the automatic track selection and it has to influence > both subtitles and captions. Based on this, maybe the setting is better renamed to reflect that. Maybe automaticTextTrackSelection is a better name then? > > > https://codereview.chromium.org/1118613002/diff/40001/LayoutTests/media/text-... > > File LayoutTests/media/text-tracks-visibility-user-override.html (right): > > > > > https://codereview.chromium.org/1118613002/diff/40001/LayoutTests/media/text-... > > LayoutTests/media/text-tracks-visibility-user-override.html:25: > > checkCaptionsDisplay(); > > Looks like this still tests for "Lorem"? (Still "passes" because > > textTrackDisplayElement(...) throws I suppose, but seems a bit weird.) > > I ended up checking captions display this way based off of > media/video-controls-captions-multiple-clicks.html. Ok, still ugly, but let's leave it at that then.
https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:2490: configuration.forceEnableSubtitleOrCaptionTrack = true; This flags forces _a_ track (any track) to be enabled. I.e. it disregards language et.c. The provided "documentation" doesn't say anything about how these cases should be handled (if for instance the specified language should be preferred or required.) It almost seems better to let this flag gate auto-selection entirely: if (!settings || !settings->automaticTrackSelectionEnabled()) return; AutomaticTrackSelection trackSelection(...); ... then defaulting it to 'true' would preserve the current behavior, and Android could set it to 'false' by default if it wants to.
On 2015/05/05 15:09:16, fs wrote: > https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... > Source/core/html/HTMLMediaElement.cpp:2490: > configuration.forceEnableSubtitleOrCaptionTrack = true; > This flags forces _a_ track (any track) to be enabled. I.e. it disregards > language et.c. The provided "documentation" doesn't say anything about how these > cases should be handled (if for instance the specified language should be > preferred or required.) > > It almost seems better to let this flag gate auto-selection entirely: > > if (!settings || !settings->automaticTrackSelectionEnabled()) > return; > > AutomaticTrackSelection trackSelection(...); > ... > > then defaulting it to 'true' would preserve the current behavior, and Android > could set it to 'false' by default if it wants to. I like the idea of using the flag to control auto-selection and defaulting it to true. For videos that don't have a track set to default and a video that has tracks with no srclang specified, auto-selection doesn't end up enabling any track. To handle these cases, setting forceEnableSubtitleOrCaptionTrack to true when flag is true(for Android) would make sense. Thoughts?
On 2015/05/06 00:19:29, srivats wrote: > On 2015/05/05 15:09:16, fs wrote: > > > https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... > > File Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... > > Source/core/html/HTMLMediaElement.cpp:2490: > > configuration.forceEnableSubtitleOrCaptionTrack = true; > > This flags forces _a_ track (any track) to be enabled. I.e. it disregards > > language et.c. The provided "documentation" doesn't say anything about how > these > > cases should be handled (if for instance the specified language should be > > preferred or required.) > > > > It almost seems better to let this flag gate auto-selection entirely: > > > > if (!settings || !settings->automaticTrackSelectionEnabled()) > > return; > > > > AutomaticTrackSelection trackSelection(...); > > ... > > > > then defaulting it to 'true' would preserve the current behavior, and Android > > could set it to 'false' by default if it wants to. > > I like the idea of using the flag to control auto-selection and defaulting it to > true. > For videos that don't have a track set to default and a video that has tracks > with no srclang specified, auto-selection doesn't end up enabling any track. To > handle these cases, setting forceEnableSubtitleOrCaptionTrack to true when flag > is true(for Android) would make sense. Thoughts? This is the current behavior, and the only effect the change above would have is that it's possible to turn off auto-selection entirely, so I don't think it's very problematic (since it's a no-op). I still find an odd thing to do, but we can iterate on that as far as I'm concerned.
https://codereview.chromium.org/1118613002/diff/60001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/dom/Documen... Source/core/dom/Document.cpp:678: for (HTMLMediaElement* mediaElement = Traversal<HTMLMediaElement>::firstWithin(*this); mediaElement; There's alrady a documentToElementSetMap() in HTMLMediaElement.cpp that could be used to avoid the traversal, see usage in HTMLMediaElement::setMediaGroup. https://codereview.chromium.org/1118613002/diff/60001/Source/core/frame/Setti... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/frame/Setti... Source/core/frame/Settings.in:325: textTracksEnabled initial=false, invalidate=TextTracksVisibility This is about as nitpicky as it gets, but I think it should be TextTrackVisibility as "text track visibility" cover both the single and plural cases. https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:2490: configuration.forceEnableSubtitleOrCaptionTrack = true; On 2015/05/05 15:09:15, fs wrote: > This flags forces _a_ track (any track) to be enabled. I.e. it disregards > language et.c. The provided "documentation" doesn't say anything about how these > cases should be handled (if for instance the specified language should be > preferred or required.) > > It almost seems better to let this flag gate auto-selection entirely: > > if (!settings || !settings->automaticTrackSelectionEnabled()) > return; > > AutomaticTrackSelection trackSelection(...); > ... > > then defaulting it to 'true' would preserve the current behavior, and Android > could set it to 'false' by default if it wants to. This bit seems a bit odd to me as well. How about a setting which says which kind of tracks to prefer in automatic text track selection? Its three states would be "default" (only enable the default tracks), "subtitles" and "captions". Whenever it changes, disable all tracks, reset any relevant state, and run automatic text track selection again. Would that work?
https://codereview.chromium.org/1118613002/diff/60001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/dom/Documen... Source/core/dom/Document.cpp:678: for (HTMLMediaElement* mediaElement = Traversal<HTMLMediaElement>::firstWithin(*this); mediaElement; On 2015/05/06 09:54:27, philipj wrote: > There's alrady a documentToElementSetMap() in HTMLMediaElement.cpp that could be > used to avoid the traversal, see usage in HTMLMediaElement::setMediaGroup. Done. https://codereview.chromium.org/1118613002/diff/60001/Source/core/frame/Setti... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/frame/Setti... Source/core/frame/Settings.in:325: textTracksEnabled initial=false, invalidate=TextTracksVisibility On 2015/05/06 09:54:27, philipj wrote: > This is about as nitpicky as it gets, but I think it should be > TextTrackVisibility as "text track visibility" cover both the single and plural > cases. Done. https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/60001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:2490: configuration.forceEnableSubtitleOrCaptionTrack = true; On 2015/05/06 09:54:27, philipj wrote: > On 2015/05/05 15:09:15, fs wrote: > > This flags forces _a_ track (any track) to be enabled. I.e. it disregards > > language et.c. The provided "documentation" doesn't say anything about how > these > > cases should be handled (if for instance the specified language should be > > preferred or required.) > > > > It almost seems better to let this flag gate auto-selection entirely: > > > > if (!settings || !settings->automaticTrackSelectionEnabled()) > > return; > > > > AutomaticTrackSelection trackSelection(...); > > ... > > > > then defaulting it to 'true' would preserve the current behavior, and Android > > could set it to 'false' by default if it wants to. > > This bit seems a bit odd to me as well. How about a setting which says which > kind of tracks to prefer in automatic text track selection? Its three states > would be "default" (only enable the default tracks), "subtitles" and "captions". > Whenever it changes, disable all tracks, reset any relevant state, and run > automatic text track selection again. Would that work? Done.
https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... Source/core/html/track/AutomaticTrackSelection.cpp:77: int highestCaptionTrackScore = 0; I'd think it would be possible to just use the value of the setting to "boost" the score of a track based on the kind. Maybe only for tracks w/ score from the language != 0. https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... Source/core/html/track/AutomaticTrackSelection.cpp:116: if (m_configuration.textTrackKindUserPreference == TextTrackKindUserPreference::Default && defaultTrack) { I think this might be better suited as a switch.
https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... Source/core/html/track/AutomaticTrackSelection.cpp:77: int highestCaptionTrackScore = 0; On 2015/06/09 16:03:57, fs wrote: > I'd think it would be possible to just use the value of the setting to "boost" > the score of a track based on the kind. Maybe only for tracks w/ score from the > language != 0. I had to split the scores for track kinds to handle the case where user preferred kind is captions and there is a subtitle track with a higher score. https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... Source/core/html/track/AutomaticTrackSelection.cpp:116: if (m_configuration.textTrackKindUserPreference == TextTrackKindUserPreference::Default && defaultTrack) { On 2015/06/09 16:03:57, fs wrote: > I think this might be better suited as a switch. Agreed. I'll change it.
https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... Source/core/html/track/AutomaticTrackSelection.cpp:77: int highestCaptionTrackScore = 0; On 2015/06/10 19:55:15, srivats wrote: > On 2015/06/09 16:03:57, fs wrote: > > I'd think it would be possible to just use the value of the setting to "boost" > > the score of a track based on the kind. Maybe only for tracks w/ score from > the > > language != 0. > > I had to split the scores for track kinds to handle the case where user > preferred kind is captions and there is a subtitle track with a higher score. I'm a little uncomfortable with the increasing number of "loose" variables in this function. It might make sense to split the current "captionsAndSubtitlesTrack" into two, and then apply the score per group - and then have the logic for actually choosing one or the other on a higher level.
On 2015/06/11 10:46:05, fs wrote: > https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... > File Source/core/html/track/AutomaticTrackSelection.cpp (right): > > https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... > Source/core/html/track/AutomaticTrackSelection.cpp:77: int > highestCaptionTrackScore = 0; > On 2015/06/10 19:55:15, srivats wrote: > > On 2015/06/09 16:03:57, fs wrote: > > > I'd think it would be possible to just use the value of the setting to > "boost" > > > the score of a track based on the kind. Maybe only for tracks w/ score from > > the > > > language != 0. > > > > I had to split the scores for track kinds to handle the case where user > > preferred kind is captions and there is a subtitle track with a higher score. > > I'm a little uncomfortable with the increasing number of "loose" variables in > this function. It might make sense to split the current > "captionsAndSubtitlesTrack" into two, and then apply the score per group - and > then have the logic for actually choosing one or the other on a higher level. I split subtitles and captions into their own groups. This has fewer loose variables but there's still a lot of different cases that had to be handled outside of the performAutomaticTextTrackSelection() function.
On 2015/06/22 22:48:24, srivats wrote: > On 2015/06/11 10:46:05, fs wrote: > > > https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... > > File Source/core/html/track/AutomaticTrackSelection.cpp (right): > > > > > https://codereview.chromium.org/1118613002/diff/80001/Source/core/html/track/... > > Source/core/html/track/AutomaticTrackSelection.cpp:77: int > > highestCaptionTrackScore = 0; > > On 2015/06/10 19:55:15, srivats wrote: > > > On 2015/06/09 16:03:57, fs wrote: > > > > I'd think it would be possible to just use the value of the setting to > > "boost" > > > > the score of a track based on the kind. Maybe only for tracks w/ score > from > > > the > > > > language != 0. > > > > > > I had to split the scores for track kinds to handle the case where user > > > preferred kind is captions and there is a subtitle track with a higher > score. > > > > I'm a little uncomfortable with the increasing number of "loose" variables in > > this function. It might make sense to split the current > > "captionsAndSubtitlesTrack" into two, and then apply the score per group - and > > then have the logic for actually choosing one or the other on a higher level. > > I split subtitles and captions into their own groups. This has fewer loose > variables but there's still a lot of different cases that had to be handled > outside of the performAutomaticTextTrackSelection() function. Thanks, I think this looks like a step in the right direction - I think there's things that could be improved still [1], but I think we could leave that for later CLs. [1] I'd like to see a gather -> select -> commit kind of flow. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3302: for (unsigned i = 0; i < m_textTracks->length(); ++i) { We have this same loop in configureTextTrackDisplay(), could you factor that out so that this becomes: m_closedCaptionsVisible = m_textTracks->hasShowingTracks(); (haveVisibleTextTrack = ... in said method) Could also be a loose function. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:64: fallbackCaptionOrSubtitleTrack = nullptr; Do this in the initializer list instead. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:99: if (trackScore > highestDefaultTrackScore && textTrack->isDefault()) { I don't think there's a need to track score for 'default' tracks - the old way of doing it is probably fine. I think the spec [1] says that all 'default' tracks should be enabled, but we can probably keep the "first 'default'" behavior for now. (I also think that's been done already in the "gather" phase of perform().) [1] https://html.spec.whatwg.org/#perform-automatic-text-track-selection https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:197: RefPtrWillBeRawPtr<TextTrack> preferredSubtitleTrack = nullptr; Move this down to just before the "if (subtitleTracks..." https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:201: if (!fallbackCaptionOrSubtitleTrack) Looks like we could do this when actually needed - i.e. together with the forceEnableSubtitleOrCaptionTrack check below - it appears selecting the "fallback to the fallback" needn't be done at this point in time. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.h (right): https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:36: RefPtrWillBeRawPtr<TextTrack> performAutomaticTextTrackSelection(const TrackGroup&); PassRef... https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:38: RefPtrWillBeRawPtr<TextTrack> enableTrackBasedOnUserPreference(RefPtrWillBeRawPtr<TextTrack>, Nit: PassRef... (both args.) https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:42: RefPtrWillBeRawPtr<TextTrack> fallbackCaptionOrSubtitleTrack; Nit: m_fallback...
https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3302: for (unsigned i = 0; i < m_textTracks->length(); ++i) { On 2015/06/23 09:23:58, fs wrote: > We have this same loop in configureTextTrackDisplay(), could you factor that out > so that this becomes: > > m_closedCaptionsVisible = m_textTracks->hasShowingTracks(); > > (haveVisibleTextTrack = ... in said method) > > Could also be a loose function. Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:64: fallbackCaptionOrSubtitleTrack = nullptr; On 2015/06/23 09:23:58, fs wrote: > Do this in the initializer list instead. Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:99: if (trackScore > highestDefaultTrackScore && textTrack->isDefault()) { On 2015/06/23 09:23:58, fs wrote: > I don't think there's a need to track score for 'default' tracks - the old way > of doing it is probably fine. > > I think the spec [1] says that all 'default' tracks should be enabled, but we > can probably keep the "first 'default'" behavior for now. > > (I also think that's been done already in the "gather" phase of perform().) > > [1] https://html.spec.whatwg.org/#perform-automatic-text-track-selection Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:197: RefPtrWillBeRawPtr<TextTrack> preferredSubtitleTrack = nullptr; On 2015/06/23 09:23:58, fs wrote: > Move this down to just before the "if (subtitleTracks..." Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:201: if (!fallbackCaptionOrSubtitleTrack) On 2015/06/23 09:23:58, fs wrote: > Looks like we could do this when actually needed - i.e. together with the > forceEnableSubtitleOrCaptionTrack check below - it appears selecting the > "fallback to the fallback" needn't be done at this point in time. Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.h (right): https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:36: RefPtrWillBeRawPtr<TextTrack> performAutomaticTextTrackSelection(const TrackGroup&); On 2015/06/23 09:23:58, fs wrote: > PassRef... Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:38: RefPtrWillBeRawPtr<TextTrack> enableTrackBasedOnUserPreference(RefPtrWillBeRawPtr<TextTrack>, On 2015/06/23 09:23:58, fs wrote: > Nit: PassRef... (both args.) Done. https://codereview.chromium.org/1118613002/diff/100001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:42: RefPtrWillBeRawPtr<TextTrack> fallbackCaptionOrSubtitleTrack; On 2015/06/23 09:23:58, fs wrote: > Nit: m_fallback... Done.
LGTM w/ nits philipj? https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... File Source/core/dom/Document.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... Source/core/dom/Document.h:56: #include "core/html/track/TextTrackKindUserPreference.h" Don't appear to need this (anymore) https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... Source/core/frame/Settings.in:332: # AutomaticTrackSelection is performed based on user preference for track kind specified Nit: Should probably be "Automatic track selection" at the beginning here. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3288: for (const auto& element: elements) Nit: space around ':' https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3300: // If a track is set to showing post performing automatic track selection, Nit: showing -> 'showing' https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:12: // Display only tracks marked as default Nit: End with full stop ('.'). https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:14: // Display caption tracks if available and if not display subtitles in preferred language Ditto. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:16: // Display subtitle tracks if available Ditto.
https://codereview.chromium.org/1118613002/diff/120001/LayoutTests/media/trac... File LayoutTests/media/track/track-kind-user-preference.html (right): https://codereview.chromium.org/1118613002/diff/120001/LayoutTests/media/trac... LayoutTests/media/track/track-kind-user-preference.html:63: track.setAttribute('onload', 'trackLoaded()'); Hmm, why is this track being loaded when it hasn't been set to showing either manually nor automatically? https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... File Source/core/dom/Document.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... Source/core/dom/Document.h:242: void textTrackKindUserPreferenceChanged(); Given how it's implemented, it ought to be possible to move this entirely out of Document even if the settings take effect immediately. Document shouldn't need to know about text tracks. https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... File Source/core/frame/Settings.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... Source/core/frame/Settings.h:37: #include "core/html/track/TextTrackKindUserPreference.h" Is this actually needed in this header, or can it be included only in some other places where it's used? https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3292: void HTMLMediaElement::automaticTrackSelectionForUpdatedUserPreference() Is it really necessary to act on the changed user preference immediately? It's not required by the spec and seems to add some complexity that will almost never be noticed. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (left): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:153: TrackGroup captionAndSubtitleTracks(TrackGroup::CaptionsAndSubtitles); I see that you have discussed this with fs, but I think splitting captionAndSubtitleTracks into two groups and selecting between them at a higher level has made the structure and name of things a bit strange right now. It's odd that performAutomaticTextTrackSelection returns a track to potentially be enabled, but in the case of chapters it's already been enabled. The role of each step isn't so clear to me. It looks like the principal reason for the split was "to handle the case where user preferred kind is captions and there is a subtitle track with a higher score" but if the score is higher for subtitles, doesn't that mean that it's really in a language that the user better understands? What is the intended model? I thought it would be that language is always the first concern, and then captions/subtitles picked only if both exist in an equally suitable language. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:16: // Display subtitle tracks if available This should also fall back to captions in the preferred language rather than the default track, right?
https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (left): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:153: TrackGroup captionAndSubtitleTracks(TrackGroup::CaptionsAndSubtitles); On 2015/06/24 09:54:20, philipj wrote: > I see that you have discussed this with fs, but I think splitting > captionAndSubtitleTracks into two groups and selecting between them at a higher > level has made the structure and name of things a bit strange right now. It's > odd that performAutomaticTextTrackSelection returns a track to potentially be > enabled, but in the case of chapters it's already been enabled. The role of each > step isn't so clear to me. FWIW, this is one of the "further improvements" that I hinted at in my comments. I don't really feel that it's something that necessarily needs be done right now, even if it's a little "unclean" ATM. Not letting "perfect" be the enemy of "good" and all that. > It looks like the principal reason for the split was "to handle the case where > user preferred kind is captions and there is a subtitle track with a higher > score" but if the score is higher for subtitles, doesn't that mean that it's > really in a language that the user better understands? What is the intended > model? I thought it would be that language is always the first concern, and then > captions/subtitles picked only if both exist in an equally suitable language. I had (maybe naively) hoped that this could be done just by doing something like: if (track->kind() == preferredTrackKind) score += someBias; // 1? Possibly with a scale-factor/other adjustment added to make sure this bias doesn't affect language-based scoring (i.e. is smaller than smallest language-score.step.)
https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (left): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:153: TrackGroup captionAndSubtitleTracks(TrackGroup::CaptionsAndSubtitles); On 2015/06/24 10:23:26, fs wrote: > On 2015/06/24 09:54:20, philipj wrote: > > I see that you have discussed this with fs, but I think splitting > > captionAndSubtitleTracks into two groups and selecting between them at a > higher > > level has made the structure and name of things a bit strange right now. It's > > odd that performAutomaticTextTrackSelection returns a track to potentially be > > enabled, but in the case of chapters it's already been enabled. The role of > each > > step isn't so clear to me. > > FWIW, this is one of the "further improvements" that I hinted at in my comments. > I don't really feel that it's something that necessarily needs be done right > now, even if it's a little "unclean" ATM. Not letting "perfect" be the enemy of > "good" and all that. > > > It looks like the principal reason for the split was "to handle the case where > > user preferred kind is captions and there is a subtitle track with a higher > > score" but if the score is higher for subtitles, doesn't that mean that it's > > really in a language that the user better understands? What is the intended > > model? I thought it would be that language is always the first concern, and > then > > captions/subtitles picked only if both exist in an equally suitable language. > > I had (maybe naively) hoped that this could be done just by doing something > like: > > if (track->kind() == preferredTrackKind) > score += someBias; // 1? > > Possibly with a scale-factor/other adjustment added to make sure this bias > doesn't affect language-based scoring (i.e. is smaller than smallest > language-score.step.) That sounds good if it's possible. srivats@, did you already try something like that an find some problem with this approach?
On 2015/06/24 10:26:16, philipj wrote: > https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... > File Source/core/html/track/AutomaticTrackSelection.cpp (left): > > https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... > Source/core/html/track/AutomaticTrackSelection.cpp:153: TrackGroup > captionAndSubtitleTracks(TrackGroup::CaptionsAndSubtitles); > On 2015/06/24 10:23:26, fs wrote: > > On 2015/06/24 09:54:20, philipj wrote: > > > I see that you have discussed this with fs, but I think splitting > > > captionAndSubtitleTracks into two groups and selecting between them at a > > higher > > > level has made the structure and name of things a bit strange right now. > It's > > > odd that performAutomaticTextTrackSelection returns a track to potentially > be > > > enabled, but in the case of chapters it's already been enabled. The role of > > each > > > step isn't so clear to me. > > > > FWIW, this is one of the "further improvements" that I hinted at in my > comments. > > I don't really feel that it's something that necessarily needs be done right > > now, even if it's a little "unclean" ATM. Not letting "perfect" be the enemy > of > > "good" and all that. > > > > > It looks like the principal reason for the split was "to handle the case > where > > > user preferred kind is captions and there is a subtitle track with a higher > > > score" but if the score is higher for subtitles, doesn't that mean that it's > > > really in a language that the user better understands? What is the intended > > > model? I thought it would be that language is always the first concern, and > > then > > > captions/subtitles picked only if both exist in an equally suitable > language. > > > > I had (maybe naively) hoped that this could be done just by doing something > > like: > > > > if (track->kind() == preferredTrackKind) > > score += someBias; // 1? > > > > Possibly with a scale-factor/other adjustment added to make sure this bias > > doesn't affect language-based scoring (i.e. is smaller than smallest > > language-score.step.) > > That sounds good if it's possible. srivats@, did you already try something like > that an find some problem with this approach? I tried out that approach and found a problem with it in handling the Subtitles case. With preferred kind Subtitles, adding a bias (<= 1) to the track score will not guarantee a subtitle track to be selected if there is a captions track with a higher or equal score(higher up in the track list). Since we don't have a case for Subtitles yet, I don't really know if this approach is acceptable. Thoughts?
https://codereview.chromium.org/1118613002/diff/120001/LayoutTests/media/trac... File LayoutTests/media/track/track-kind-user-preference.html (right): https://codereview.chromium.org/1118613002/diff/120001/LayoutTests/media/trac... LayoutTests/media/track/track-kind-user-preference.html:63: track.setAttribute('onload', 'trackLoaded()'); On 2015/06/24 09:54:19, philipj wrote: > Hmm, why is this track being loaded when it hasn't been set to showing either > manually nor automatically? It's a subtitle track in a language that has the highest track score. When the track is added, auto-selection sets this track to showing. https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... File Source/core/dom/Document.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... Source/core/dom/Document.h:56: #include "core/html/track/TextTrackKindUserPreference.h" On 2015/06/24 08:59:07, fs wrote: > Don't appear to need this (anymore) Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/dom/Docume... Source/core/dom/Document.h:242: void textTrackKindUserPreferenceChanged(); On 2015/06/24 09:54:20, philipj wrote: > Given how it's implemented, it ought to be possible to move this entirely out of > Document even if the settings take effect immediately. Document shouldn't need > to know about text tracks. Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... File Source/core/frame/Settings.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... Source/core/frame/Settings.h:37: #include "core/html/track/TextTrackKindUserPreference.h" On 2015/06/24 09:54:20, philipj wrote: > Is this actually needed in this header, or can it be included only in some other > places where it's used? Settings.in references this enum and since we can't add includes in the ".in" file, it goes in the header. https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... Source/core/frame/Settings.in:332: # AutomaticTrackSelection is performed based on user preference for track kind specified On 2015/06/24 08:59:07, fs wrote: > Nit: Should probably be "Automatic track selection" at the beginning here. Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3288: for (const auto& element: elements) On 2015/06/24 08:59:07, fs wrote: > Nit: space around ':' Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3292: void HTMLMediaElement::automaticTrackSelectionForUpdatedUserPreference() On 2015/06/24 09:54:20, philipj wrote: > Is it really necessary to act on the changed user preference immediately? It's > not required by the spec and seems to add some complexity that will almost never > be noticed. It does seem a little buggy to not have the platform state affect the videos immediately on Android. Without this, the state change takes effect only on a page refresh. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3300: // If a track is set to showing post performing automatic track selection, On 2015/06/24 08:59:07, fs wrote: > Nit: showing -> 'showing' Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:12: // Display only tracks marked as default On 2015/06/24 08:59:07, fs wrote: > Nit: End with full stop ('.'). Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:14: // Display caption tracks if available and if not display subtitles in preferred language On 2015/06/24 08:59:07, fs wrote: > Ditto. Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:16: // Display subtitle tracks if available On 2015/06/24 08:59:07, fs wrote: > Ditto. Done. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:16: // Display subtitle tracks if available On 2015/06/24 09:54:20, philipj wrote: > This should also fall back to captions in the preferred language rather than the > default track, right? Since we didn't really have a case for subtitles, I wasn't sure what the expected behavior should be. The reason I handled captions differently was because the closed caption setting in platform is for the benefit of the hearing impaired. It made sense to show them preferred language subtitles when there are no captions in preferred language. Would it make sense to have the same behavior for Subtitles too?
On 2015/07/02 00:25:33, srivats wrote: > On 2015/06/24 10:26:16, philipj wrote: > > > https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... > > File Source/core/html/track/AutomaticTrackSelection.cpp (left): > > > > > https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... > > Source/core/html/track/AutomaticTrackSelection.cpp:153: TrackGroup > > captionAndSubtitleTracks(TrackGroup::CaptionsAndSubtitles); > > On 2015/06/24 10:23:26, fs wrote: > > > On 2015/06/24 09:54:20, philipj wrote: > > > > I see that you have discussed this with fs, but I think splitting > > > > captionAndSubtitleTracks into two groups and selecting between them at a > > > higher > > > > level has made the structure and name of things a bit strange right now. > > It's > > > > odd that performAutomaticTextTrackSelection returns a track to potentially > > be > > > > enabled, but in the case of chapters it's already been enabled. The role > of > > > each > > > > step isn't so clear to me. > > > > > > FWIW, this is one of the "further improvements" that I hinted at in my > > comments. > > > I don't really feel that it's something that necessarily needs be done right > > > now, even if it's a little "unclean" ATM. Not letting "perfect" be the enemy > > of > > > "good" and all that. > > > > > > > It looks like the principal reason for the split was "to handle the case > > where > > > > user preferred kind is captions and there is a subtitle track with a > higher > > > > score" but if the score is higher for subtitles, doesn't that mean that > it's > > > > really in a language that the user better understands? What is the > intended > > > > model? I thought it would be that language is always the first concern, > and > > > then > > > > captions/subtitles picked only if both exist in an equally suitable > > language. > > > > > > I had (maybe naively) hoped that this could be done just by doing something > > > like: > > > > > > if (track->kind() == preferredTrackKind) > > > score += someBias; // 1? > > > > > > Possibly with a scale-factor/other adjustment added to make sure this bias > > > doesn't affect language-based scoring (i.e. is smaller than smallest > > > language-score.step.) > > > > That sounds good if it's possible. srivats@, did you already try something > like > > that an find some problem with this approach? > > I tried out that approach and found a problem with it in handling the Subtitles > case. With preferred kind Subtitles, adding a bias (<= 1) to the track score > will not guarantee a subtitle track to be selected if there is a captions track > with a higher or equal score(higher up in the track list). Since we don't have a > case for Subtitles yet, I don't really know if this approach is acceptable. > Thoughts? It sounds like the behavior I'd expect from that strategy ("prefer language over kind"). Doing it the opposite way ("prefer kind over language" -> bias >= max lang. score) could work too I suppose under the assumption that all "scorable" languages are ones that the user can comprehend. Personally I think I'd rather have the former strategy though.
I agree, simply prioritizing language over kind seems like a good start, one that we can tweak if it turns out to not match user expectations.
On 2015/07/02 08:54:43, philipj wrote: > I agree, simply prioritizing language over kind seems like a good start, one > that we can tweak if it turns out to not match user expectations. Done
LGTM w/ nits https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (left): https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:123: if (textTrack != trackToEnable) Nit: Restore this test. https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:173: if (kind == TextTrack::captionsKeyword() || kind == TextTrack::subtitlesKeyword()) { Nit: Restore this. https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.h (right): https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:13: class TextTrack; Nit: Not needed (in the header.)
https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (left): https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:123: if (textTrack != trackToEnable) On 2015/07/07 08:22:05, fs wrote: > Nit: Restore this test. Done. https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.cpp (right): https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.cpp:173: if (kind == TextTrack::captionsKeyword() || kind == TextTrack::subtitlesKeyword()) { On 2015/07/07 08:22:05, fs wrote: > Nit: Restore this. Done. https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.h (right): https://codereview.chromium.org/1118613002/diff/160001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:13: class TextTrack; On 2015/07/07 08:22:05, fs wrote: > Nit: Not needed (in the header.) Done.
https://codereview.chromium.org/1118613002/diff/120001/LayoutTests/media/trac... File LayoutTests/media/track/track-kind-user-preference.html (right): https://codereview.chromium.org/1118613002/diff/120001/LayoutTests/media/trac... LayoutTests/media/track/track-kind-user-preference.html:63: track.setAttribute('onload', 'trackLoaded()'); On 2015/07/02 01:41:19, srivats wrote: > On 2015/06/24 09:54:19, philipj wrote: > > Hmm, why is this track being loaded when it hasn't been set to showing either > > manually nor automatically? > It's a subtitle track in a language that has the highest track score. When the > track is added, auto-selection sets this track to showing. Acknowledged. https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... File Source/core/frame/Settings.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/frame/Sett... Source/core/frame/Settings.h:37: #include "core/html/track/TextTrackKindUserPreference.h" On 2015/07/02 01:41:20, srivats wrote: > On 2015/06/24 09:54:20, philipj wrote: > > Is this actually needed in this header, or can it be included only in some > other > > places where it's used? > > Settings.in references this enum and since we can't add includes in the ".in" > file, it goes in the header. Acknowledged. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3292: void HTMLMediaElement::automaticTrackSelectionForUpdatedUserPreference() On 2015/07/02 01:41:21, srivats wrote: > On 2015/06/24 09:54:20, philipj wrote: > > Is it really necessary to act on the changed user preference immediately? It's > > not required by the spec and seems to add some complexity that will almost > never > > be noticed. > > It does seem a little buggy to not have the platform state affect the videos > immediately on Android. Without this, the state change takes effect only on a > page refresh. Do native video players take the setting into account immediately when it's changed? https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:16: // Display subtitle tracks if available On 2015/07/02 01:41:21, srivats wrote: > On 2015/06/24 08:59:07, fs wrote: > > Ditto. > > Done. The documentation is the same in the most recent CL, was there some other change? (Sorry, hard to follow big reviews.)
OK, I'm on board with this now I think, just some nits. https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:37: const AtomicString& preferredTrackKind(); I guess this can be const? https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:14: // Display caption tracks if available and if not display subtitles in preferred language. So now I've read the code again, and I can't see a difference between captions and subtitles as far as fallback is concerned, in both cases the other kind may be used under the same conditions, right? Not a problem, just documentation that needs to be updated. https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/TextTrackList.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/TextTrackList.h:79: bool hasShowingTracks(); const? https://codereview.chromium.org/1118613002/diff/180001/Source/core/page/Page.cpp File Source/core/page/Page.cpp (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/page/Page.... Source/core/page/Page.cpp:502: HTMLMediaElement::setTextTrackKindUserPreferenceForAllMediaElements(toLocalFrame(frame)->document()); There's a null check for doc above so I guess that document can be null here too? If so it needs to be handled. Handling it here and passing a reference to HTMLMediaElement would make it clearly not null at that point, but anything that's roughly consistent with existing code is fine. https://codereview.chromium.org/1118613002/diff/180001/Source/core/testing/In... File Source/core/testing/InternalSettings.cpp (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/testing/In... Source/core/testing/InternalSettings.cpp:306: exceptionState.throwDOMException(SyntaxError, "The user preference for text track kind " + preference + ")' is invalid."); This should also return, otherwise we'll still call setTextTrackKindUserPreference which seems weird. If setTextTrackKindUserPreference already handles errors, it doesn't seem necessary to do it here too.
https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3292: void HTMLMediaElement::automaticTrackSelectionForUpdatedUserPreference() On 2015/07/09 15:20:49, philipj wrote: > On 2015/07/02 01:41:21, srivats wrote: > > On 2015/06/24 09:54:20, philipj wrote: > > > Is it really necessary to act on the changed user preference immediately? > It's > > > not required by the spec and seems to add some complexity that will almost > > never > > > be noticed. > > > > It does seem a little buggy to not have the platform state affect the videos > > immediately on Android. Without this, the state change takes effect only on a > > page refresh. > > Do native video players take the setting into account immediately when it's > changed? I know that WebKit's native video player reacts to it a similar way. So the settings take effect immediately on Safari and Chrome on iOS and Safari on Mac. Unfortunately none of the other browsers support the platform caption settings yet. https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:16: // Display subtitle tracks if available On 2015/07/09 15:20:49, philipj wrote: > On 2015/07/02 01:41:21, srivats wrote: > > On 2015/06/24 08:59:07, fs wrote: > > > Ditto. > > > > Done. > > The documentation is the same in the most recent CL, was there some other > change? (Sorry, hard to follow big reviews.) Sorry I missed updating this earlier. https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/AutomaticTrackSelection.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/AutomaticTrackSelection.h:37: const AtomicString& preferredTrackKind(); On 2015/07/09 15:34:09, philipj wrote: > I guess this can be const? Done. https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/TextTrackKindUserPreference.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/TextTrackKindUserPreference.h:14: // Display caption tracks if available and if not display subtitles in preferred language. On 2015/07/09 15:34:09, philipj wrote: > So now I've read the code again, and I can't see a difference between captions > and subtitles as far as fallback is concerned, in both cases the other kind may > be used under the same conditions, right? Not a problem, just documentation that > needs to be updated. Done. https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/TextTrackList.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/TextTrackList.h:79: bool hasShowingTracks(); On 2015/07/09 15:34:09, philipj wrote: > const? The method accesses item(i) which is not a const. Changing this to const complains about item(i) discarding qualifiers. https://codereview.chromium.org/1118613002/diff/180001/Source/core/page/Page.cpp File Source/core/page/Page.cpp (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/page/Page.... Source/core/page/Page.cpp:502: HTMLMediaElement::setTextTrackKindUserPreferenceForAllMediaElements(toLocalFrame(frame)->document()); On 2015/07/09 15:34:09, philipj wrote: > There's a null check for doc above so I guess that document can be null here > too? If so it needs to be handled. Handling it here and passing a reference to > HTMLMediaElement would make it clearly not null at that point, but anything > that's roughly consistent with existing code is fine. Done. https://codereview.chromium.org/1118613002/diff/180001/Source/core/testing/In... File Source/core/testing/InternalSettings.cpp (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/testing/In... Source/core/testing/InternalSettings.cpp:306: exceptionState.throwDOMException(SyntaxError, "The user preference for text track kind " + preference + ")' is invalid."); On 2015/07/09 15:34:09, philipj wrote: > This should also return, otherwise we'll still call > setTextTrackKindUserPreference which seems weird. If > setTextTrackKindUserPreference already handles errors, it doesn't seem necessary > to do it here too. Since userPreference is initialized to Default, if an invalid preference is passed in this method, we set the preference to default instead of returning. Does that seem reasonable?
https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1118613002/diff/120001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3292: void HTMLMediaElement::automaticTrackSelectionForUpdatedUserPreference() On 2015/07/09 21:25:37, srivats wrote: > On 2015/07/09 15:20:49, philipj wrote: > > On 2015/07/02 01:41:21, srivats wrote: > > > On 2015/06/24 09:54:20, philipj wrote: > > > > Is it really necessary to act on the changed user preference immediately? > > It's > > > > not required by the spec and seems to add some complexity that will almost > > > never > > > > be noticed. > > > > > > It does seem a little buggy to not have the platform state affect the videos > > > immediately on Android. Without this, the state change takes effect only on > a > > > page refresh. > > > > Do native video players take the setting into account immediately when it's > > changed? > > I know that WebKit's native video player reacts to it a similar way. So the > settings take effect immediately on Safari and Chrome on iOS and Safari on Mac. > Unfortunately none of the other browsers support the platform caption settings > yet. OK, fair enough. https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... File Source/core/html/track/TextTrackList.h (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/html/track... Source/core/html/track/TextTrackList.h:79: bool hasShowingTracks(); On 2015/07/09 21:25:37, srivats wrote: > On 2015/07/09 15:34:09, philipj wrote: > > const? > > The method accesses item(i) which is not a const. Changing this to const > complains about item(i) discarding qualifiers. Acknowledged. https://codereview.chromium.org/1118613002/diff/180001/Source/core/testing/In... File Source/core/testing/InternalSettings.cpp (right): https://codereview.chromium.org/1118613002/diff/180001/Source/core/testing/In... Source/core/testing/InternalSettings.cpp:306: exceptionState.throwDOMException(SyntaxError, "The user preference for text track kind " + preference + ")' is invalid."); On 2015/07/09 21:25:37, srivats wrote: > On 2015/07/09 15:34:09, philipj wrote: > > This should also return, otherwise we'll still call > > setTextTrackKindUserPreference which seems weird. If > > setTextTrackKindUserPreference already handles errors, it doesn't seem > necessary > > to do it here too. > > Since userPreference is initialized to Default, if an invalid preference is > passed in this method, we set the preference to default instead of returning. > Does that seem reasonable? I don't think continuing the execution after throwing an exception makes much sense, but I see that InternalSettings::setDisplayModeOverride and others do the same and this is an internal API so it doesn't really matter, so OK.
This now LGTM % documentation out of sync https://codereview.chromium.org/1118613002/diff/200001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/1118613002/diff/200001/public/web/WebSettings... public/web/WebSettings.h:104: // Display caption tracks if available and if not display subtitles in preferred language Copy the documentation from the other place here. (The obvious difference + missing trailing periods.)
The CQ bit was checked by srivats@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1118613002/#ps220001 (title: "Addressed lgtm comment and rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118613002/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198661 |
