Remove HTMLMediaElement::textTrackKindChanged

Remove HTMLMediaElement::textTrackKindChanged

This method only operates on the TextTrack that it gets as an argument, so
the code can be moved to the caller without changing behavior. The
HTMLMediaElement will still be notified via the mode-change (if one takes
place.)

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

[fs, 2015-02-23 12:57:20]

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

[philipj_slow, 2015-02-23 13:41:46]

https://codereview.chromium.org/924273004/diff/20001/Source/core/html/track/T...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/924273004/diff/20001/Source/core/html/track/T...
Source/core/html/track/TextTrack.cpp:159: // If kind changes from visual to
non-visual and mode is 'showing', then force mode to 'hidden'.
Does this correspond to anything in the HTML spec? It seems reasonable, but
there's nothing equivalent when setting mode. I think this means that the follow
two bits of code won't result in the same state, if we start with kind
'captions' and mode 'hidden':

track.kind = 'metadata';
track.mode = 'showing';
// mode is showing

track.mode = 'showing';
track.kind = 'metadata';
// mode is hidden

That seems a bit odd.

[fs, 2015-02-23 14:00:03]

https://codereview.chromium.org/924273004/diff/20001/Source/core/html/track/T...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/924273004/diff/20001/Source/core/html/track/T...
Source/core/html/track/TextTrack.cpp:159: // If kind changes from visual to
non-visual and mode is 'showing', then force mode to 'hidden'.
On 2015/02/23 13:41:46, philipj_UTC7 wrote:
> Does this correspond to anything in the HTML spec?

AFAIK, no. The spec says "The kind of track can change dynamically, in the case
of a text track corresponding to a track element." but doesn't prescribe any
specific handling for that case.

> It seems reasonable, but
> there's nothing equivalent when setting mode. I think this means that the
follow
> two bits of code won't result in the same state, if we start with kind
> 'captions' and mode 'hidden':
> 
> track.kind = 'metadata';
> track.mode = 'showing';
> // mode is showing
> 
> track.mode = 'showing';
> track.kind = 'metadata';
> // mode is hidden
> 
> That seems a bit odd.

Odd indeed. I suspect what should really happen here (in TextTrack::setKind) is
that any currently rendered cues gets hidden.

[fs, 2015-02-23 14:56:00]

On 2015/02/23 14:00:03, fs wrote:
>
https://codereview.chromium.org/924273004/diff/20001/Source/core/html/track/T...
> File Source/core/html/track/TextTrack.cpp (right):
> 
>
https://codereview.chromium.org/924273004/diff/20001/Source/core/html/track/T...
> Source/core/html/track/TextTrack.cpp:159: // If kind changes from visual to
> non-visual and mode is 'showing', then force mode to 'hidden'.
> On 2015/02/23 13:41:46, philipj_UTC7 wrote:
> > Does this correspond to anything in the HTML spec?
> 
> AFAIK, no. The spec says "The kind of track can change dynamically, in the
case
> of a text track corresponding to a track element." but doesn't prescribe any
> specific handling for that case.
> 
> > It seems reasonable, but
> > there's nothing equivalent when setting mode. I think this means that the
> follow
> > two bits of code won't result in the same state, if we start with kind
> > 'captions' and mode 'hidden':
> > 
> > track.kind = 'metadata';
> > track.mode = 'showing';
> > // mode is showing
> > 
> > track.mode = 'showing';
> > track.kind = 'metadata';
> > // mode is hidden
> > 
> > That seems a bit odd.
> 
> Odd indeed. I suspect what should really happen here (in TextTrack::setKind)
is
> that any currently rendered cues gets hidden.

FWIW, I had no intention of changing current behavior in this CL - this method
just looked a bit misplaced =)

[philipj_slow, 2015-02-23 15:10:52]

Yeah, LGTM to this refactoring, but can you add a FIXME, bug, or something, so
that someone may stumble upon this again in the future?

[fs, 2015-02-23 15:18:26]

New patchsets have been uploaded after l-g-t-m from philipj@opera.com

[fs, 2015-02-23 15:19:19]

On 2015/02/23 15:10:52, philipj_UTC7 wrote:
> Yeah, LGTM to this refactoring, but can you add a FIXME, bug, or something, so
> that someone may stumble upon this again in the future?

Filed bug and added FIXME referencing it.

[fs, 2015-02-23 15:19:49]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-02-23 15:20:34]

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

[commit-bot: I haz the power, 2015-02-23 16:54:43]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190667