Move text track active list management to CueTimeline

Move text track active list management to CueTimeline

This CL introduces a new class CueTimeline and moves the code used
for managing the list of active cues, the interval tree and other
state used when rendering cues from HTMLMediaElement to it.
For the most part this is code movement. The exception is that
CueTimeline::updateActiveCues check for the presence of
a WebMediaPlayer rather than a MediaPlayer when computing the new
list of active cues.
Methods/data are rename to remove [Tt]extTrack where appropriate.
Some adjustments are done to visibility/exposure of data on
HTMLMediaElement to allow the state to be queried when needed.

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

[fs, 2015-02-11 12:59:04]

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

[fs, 2015-02-11 13:55:51]

> TextTrackRenderer::updateActiveCues does not check for the presence of a
MediaPlayer

Luckily there was a test for this condition (see trybot results), but feel free
to take a look at everything else while I see if I can fix that without
(re)introducing that conditional...

[fs, 2015-02-11 17:33:56]

On 2015/02/11 13:55:51, fs wrote:
> > TextTrackRenderer::updateActiveCues does not check for the presence of a
> MediaPlayer
> 
> Luckily there was a test for this condition (see trybot results), but feel
free
> to take a look at everything else while I see if I can fix that without
> (re)introducing that conditional...

Applied a band-aid (webMediaPlayer() instead of m_player). Will try to come up
with a way to avoid this proper (the in-band code-path seems susceptible to this
same issue...)

[philipj_slow, 2015-02-11 17:52:09]

> Some adjustments are done to visibility/exposure of data on
> HTMLMediaElement to allow the state to be queried from the
> HTMLMediaElement when needed.

I think one of those HTMLMediaElement should be something else?

Also some bikeshedding on the name TextTrackRenderer. It seems like it isn't
really concerned with rendering directly, but rather with managing active cues.
Which other names did you consider? TextTrackActiveCues? What is the essence of
this thing?

[fs, 2015-02-12 09:36:20]

On 2015/02/11 17:52:09, philipj_UTC7 wrote:
> > Some adjustments are done to visibility/exposure of data on
> > HTMLMediaElement to allow the state to be queried from the
> > HTMLMediaElement when needed.
> 
> I think one of those HTMLMediaElement should be something else?

I guess it depends on how you interpret "queried from"... I dropped "from the
HTMLMediaElement" - hopefully that removes the ambiguity.

> Also some bikeshedding on the name TextTrackRenderer. It seems like it isn't
> really concerned with rendering directly, but rather with managing active
cues.
> Which other names did you consider? TextTrackActiveCues? What is the essence
of
> this thing?

I'm intending to move the rendering code (management of the text track container
and cue DOM) to it as well. I did find it hard to find a name that encompassed
both the scheduling/active state management and rendering bit... I suppose we
could split it into two - TextTrackScheduler (CueScheduler? CueTimeline?) and
TextTrackRenderer (TextTrackDisplayState? could be made per cue as
CueDisplayState). I think it'll need to start as one or the other though since
it's kind of intertwined at the moment - so maybe rename TextTrackRenderer in
this CL to CueTimeline (that's the one like best I think), and then see what
falls out of that?

[philipj_slow, 2015-02-12 10:03:52]

On 2015/02/12 09:36:20, fs wrote:
> On 2015/02/11 17:52:09, philipj_UTC7 wrote:
> > > Some adjustments are done to visibility/exposure of data on
> > > HTMLMediaElement to allow the state to be queried from the
> > > HTMLMediaElement when needed.
> > 
> > I think one of those HTMLMediaElement should be something else?
> 
> I guess it depends on how you interpret "queried from"... I dropped "from the
> HTMLMediaElement" - hopefully that removes the ambiguity.
> 
> > Also some bikeshedding on the name TextTrackRenderer. It seems like it isn't
> > really concerned with rendering directly, but rather with managing active
> cues.
> > Which other names did you consider? TextTrackActiveCues? What is the essence
> of
> > this thing?
> 
> I'm intending to move the rendering code (management of the text track
container
> and cue DOM) to it as well. I did find it hard to find a name that encompassed
> both the scheduling/active state management and rendering bit... I suppose we
> could split it into two - TextTrackScheduler (CueScheduler? CueTimeline?) and
> TextTrackRenderer (TextTrackDisplayState? could be made per cue as
> CueDisplayState). I think it'll need to start as one or the other though since
> it's kind of intertwined at the moment - so maybe rename TextTrackRenderer in
> this CL to CueTimeline (that's the one like best I think), and then see what
> falls out of that?

Ah, more good stuff to come as usual. CueTimeline for now sounds OK, at least
it's pretty accurate.

[fs, 2015-02-12 10:21:19]

Renamed to CueTimeline.

On 2015/02/12 10:03:52, philipj_UTC7 wrote:
...
> Ah, more good stuff to come as usual. CueTimeline for now sounds OK, at least
> it's pretty accurate.

Once you start it's hard to stop =)

[fs, 2015-02-16 15:06:15]

Ping philipj.

[philipj_slow, 2015-02-16 17:22:11]

lgtm

[fs, 2015-02-17 08:38:04]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-02-17 08:38:57]

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

[commit-bot: I haz the power, 2015-02-17 08:41:52]

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

[brucedawson, 2015-02-18 21:06:39]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-02-18 21:06:40]

https://codereview.chromium.org/913133003/diff/40001/Source/core/html/track/C...
File Source/core/html/track/CueTimeline.cpp (right):

https://codereview.chromium.org/913133003/diff/40001/Source/core/html/track/C...
Source/core/html/track/CueTimeline.cpp:306: RefPtrWillBeRawPtr<Event> event =
Event::create(EventTypeNames::cuechange);
The declaration of 'event' shadows the declaration with the same name/type on
line 298 (eight lines earlier). There is no indication that this is a bug, but
it is potentially confusing. Consider renaming one of the variables?

This showed up as a new warning on the /analyze builder:

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyze

[fs, 2015-02-19 09:26:38]

On 2015/02/18 21:06:40, brucedawson wrote:
>
https://codereview.chromium.org/913133003/diff/40001/Source/core/html/track/C...
> File Source/core/html/track/CueTimeline.cpp (right):
> 
>
https://codereview.chromium.org/913133003/diff/40001/Source/core/html/track/C...
> Source/core/html/track/CueTimeline.cpp:306: RefPtrWillBeRawPtr<Event> event =
> Event::create(EventTypeNames::cuechange);
> The declaration of 'event' shadows the declaration with the same name/type on
> line 298 (eight lines earlier). There is no indication that this is a bug, but
> it is potentially confusing. Consider renaming one of the variables?
 
Thanks for noticing/notifying =). There's no bug here - there on event created
and dispatched in the ancestor scope and one in this scope. I'll look into
eliminating the confusion though.

[fs, 2015-02-19 16:28:56]

On 2015/02/19 09:26:38, fs wrote:
> On 2015/02/18 21:06:40, brucedawson wrote:
> >
>
https://codereview.chromium.org/913133003/diff/40001/Source/core/html/track/C...
> > File Source/core/html/track/CueTimeline.cpp (right):
> > 
> >
>
https://codereview.chromium.org/913133003/diff/40001/Source/core/html/track/C...
> > Source/core/html/track/CueTimeline.cpp:306: RefPtrWillBeRawPtr<Event> event
=
> > Event::create(EventTypeNames::cuechange);
> > The declaration of 'event' shadows the declaration with the same name/type
on
> > line 298 (eight lines earlier). There is no indication that this is a bug,
but
> > it is potentially confusing. Consider renaming one of the variables?
>  
> Thanks for noticing/notifying =). There's no bug here - there on event created
> and dispatched in the ancestor scope and one in this scope. I'll look into
> eliminating the confusion though.

Done in https://codereview.chromium.org/935423002/