Implement automatic text track selection for 'metadata' tracks

Implement automatic text track selection for 'metadata' tracks

Previously 'metadata' tracks were being configured using the "perform
automatic text track selection" algorithm from the the spec, while it
should only have applied step 4 of the "honor user preferences for
automatic text track selection" algorithm.

BUG=251254
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189185

[fs, 2015-01-28 14:17:59]

fs@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2015-01-29 10:34:52]

lgtm with nits

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-selection-metadata.html (right):

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:9: video.onloadstart =
test.step_func(function() {
To be less forgiving, do this immediately after </video> as that's the per-spec
trigger for "honor user preferences for automatic text track selection". If you
don't need a video.currentSrc for the test to pass, remove that. I does not seem
required per spec.

If you move it, you can also use the async_test(function(test) {}) pattern,
which I like if there's nothing special going on.

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:20:
assert_equals(tracks[i].mode, 'disabled');
Also assert_false(trackElement.default)?

https://codereview.chromium.org/882993002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/882993002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2984:
configureTextTrackGroup(otherTracks);
This looks like it's not per spec if you want to remove it in another CL.
There's also some renaming to be done that would make all of this more
spec-like.

[fs, 2015-01-29 12:05:51]

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-selection-metadata.html (right):

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:9: video.onloadstart =
test.step_func(function() {
On 2015/01/29 10:34:52, philipj_UTC7 wrote:
> To be less forgiving, do this immediately after </video> as that's the
per-spec
> trigger for "honor user preferences for automatic text track selection". If
you
> don't need a video.currentSrc for the test to pass, remove that. I does not
seem
> required per spec.

I've moved it. The selection is not run synchronously at </video>, so it becomes
a bit hard to observe it in a good and stable manner (hence the src). Quite
possible that should be fixed...
 
> If you move it, you can also use the async_test(function(test) {}) pattern,
> which I like if there's nothing special going on.

Done.

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:20:
assert_equals(tracks[i].mode, 'disabled');
On 2015/01/29 10:34:52, philipj_UTC7 wrote:
> Also assert_false(trackElement.default)?

That means I need to find the corresponding element in these cases too, which
kind of jogs the test setup a bit. Done.

https://codereview.chromium.org/882993002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/882993002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2984:
configureTextTrackGroup(otherTracks);
On 2015/01/29 10:34:52, philipj_UTC7 wrote:
> This looks like it's not per spec if you want to remove it in another CL.
> There's also some renaming to be done that would make all of this more
> spec-like.

Yes, I noticed this as well, and I wondered what tracks would be "other"...
replacing it with a ASSERT_NOT_REACHED above ought to do it...

[fs, 2015-01-29 12:57:50]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-01-29 12:59:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/882993002/20001

[commit-bot: I haz the power, 2015-01-29 13:20:22]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189185

[philipj_slow, 2015-01-29 14:58:04]

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-selection-metadata.html (right):

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:9: video.onloadstart =
test.step_func(function() {
On 2015/01/29 12:05:51, fs wrote:
> On 2015/01/29 10:34:52, philipj_UTC7 wrote:
> > To be less forgiving, do this immediately after </video> as that's the
> per-spec
> > trigger for "honor user preferences for automatic text track selection". If
> you
> > don't need a video.currentSrc for the test to pass, remove that. I does not
> seem
> > required per spec.
> 
> I've moved it. The selection is not run synchronously at </video>, so it
becomes
> a bit hard to observe it in a good and stable manner (hence the src). Quite
> possible that should be fixed...
>  
> > If you move it, you can also use the async_test(function(test) {}) pattern,
> > which I like if there's nothing special going on.
> 
> Done.

Sounds like the automatic selection is triggered later then required by the
spec, then.

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:20:
assert_equals(tracks[i].mode, 'disabled');
On 2015/01/29 12:05:51, fs wrote:
> On 2015/01/29 10:34:52, philipj_UTC7 wrote:
> > Also assert_false(trackElement.default)?
> 
> That means I need to find the corresponding element in these cases too, which
> kind of jogs the test setup a bit. Done.

Oh, I wasn't paying attention. It works the new way, though.

[fs, 2015-01-29 15:06:52]

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-selection-metadata.html (right):

https://codereview.chromium.org/882993002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-selection-metadata.html:9: video.onloadstart =
test.step_func(function() {
On 2015/01/29 14:58:04, philipj_UTC7 wrote:
> On 2015/01/29 12:05:51, fs wrote:
> > On 2015/01/29 10:34:52, philipj_UTC7 wrote:
> > > To be less forgiving, do this immediately after </video> as that's the
> > per-spec
> > > trigger for "honor user preferences for automatic text track selection".
If
> > you
> > > don't need a video.currentSrc for the test to pass, remove that. I does
not
> > seem
> > > required per spec.
> > 
> > I've moved it. The selection is not run synchronously at </video>, so it
> becomes
> > a bit hard to observe it in a good and stable manner (hence the src). Quite
> > possible that should be fixed...
> >  
> > > If you move it, you can also use the async_test(function(test) {})
pattern,
> > > which I like if there's nothing special going on.
> > 
> > Done.
> 
> Sounds like the automatic selection is triggered later then required by the
> spec, then.

Yes, this had me scratching my head for quite some time (I originally had a
test() just after </video>.)