Move loading machinery out of LoadableTextTrack

Move loading machinery out of LoadableTextTrack

Currently, the code to load a text track is spread between
HTMLTrackElement, LoadableTextTrack and TextTrackLoader - where the former
two create the loader and respond to events from it. It's not obvious what
the responsibilities are for each of HTMLTrackElement and
LoadableTextTrack.
To clear this up a bit, move the shared loading responsibilities into
HTMLTrackElement only. This makes the code-flow more clear and even reduces
the amount of state involved.
Also update the spec prose comments and attempt to simplify the code
somewhat.

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

[fs, 2015-03-11 13:39:52]

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

[philipj_slow, 2015-03-12 04:24:08]

I like this, looks like there's not too much of LoadableTextTrack left..

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
File Source/core/html/HTMLTrackElement.cpp (right):

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:167: // 5. Top: Await a stable state. The
synchronous section consists of the following steps. (The steps in the
I think moving this to where the timer is started and explaining why a timer is
a close-enough proxy for "await a stable state" would be good. I think there
actually is a difference that can be observed by careful queueing of tasks that
fire events that look at the track's ready state, in which case a FIXME would be
in order.

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:196: // The track element might have
changed its state to Loading waiting for a call to didCompleteLoad to continue.
This is the track element, and this comment doesn't seem to catch the gist of
what's going on: if the load has already finished (even if failed) we'll just
report the same state again. Perhaps a switch on m_loader->loadState() would
make this a bit clearer?

I also wonder why we have the url==m_url case at all, do you know what terrible
things would happen if we just tried to load it like any other URL?

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:233: // check fails, etc), or if URL is
the empty string, then queue a task to first change the text track readiness
Firing the event here is indistinguishable from queueing an event as long as
didCompleteLoad is only called from a timer or similar, which seems to hold
true. Can you add a comment to document this?

[fs, 2015-03-12 15:22:18]

On 2015/03/12 04:24:08, philipj_UTC7 wrote:
> I like this, looks like there's not too much of LoadableTextTrack left..

Coincidence? =P

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
File Source/core/html/HTMLTrackElement.cpp (right):

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:167: // 5. Top: Await a stable state. The
synchronous section consists of the following steps. (The steps in the
On 2015/03/12 04:24:07, philipj_UTC7 wrote:
> I think moving this to where the timer is started and explaining why a timer
is
> a close-enough proxy for "await a stable state" would be good. I think there
> actually is a difference that can be observed by careful queueing of tasks
that
> fire events that look at the track's ready state, in which case a FIXME would
be
> in order.

Moved, FIXME added.

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:196: // The track element might have
changed its state to Loading waiting for a call to didCompleteLoad to continue.
On 2015/03/12 04:24:08, philipj_UTC7 wrote:
> This is the track element, and this comment doesn't seem to catch the gist of
> what's going on: if the load has already finished (even if failed) we'll just
> report the same state again. Perhaps a switch on m_loader->loadState() would
> make this a bit clearer?

Adding switch and dropping this comment (which I have trouble straightening to
something comprehensible...)
 
> I also wonder why we have the url==m_url case at all, do you know what
terrible
> things would happen if we just tried to load it like any other URL?

Hopefully that's fine... Given that we don't recreate the loader etc. (I.e. we
get the same resource from cache and nothing else). I'd put that in the "NEEDS
TEST" bucket though =)

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:233: // check fails, etc), or if URL is
the empty string, then queue a task to first change the text track readiness
On 2015/03/12 04:24:07, philipj_UTC7 wrote:
> Firing the event here is indistinguishable from queueing an event as long as
> didCompleteLoad is only called from a timer or similar, which seems to hold
> true. Can you add a comment to document this?

Done.

[philipj_slow, 2015-03-13 03:27:31]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-03-13 03:27:32]

lgtm

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
File Source/core/html/HTMLTrackElement.cpp (right):

https://codereview.chromium.org/999773002/diff/1/Source/core/html/HTMLTrackEl...
Source/core/html/HTMLTrackElement.cpp:196: // The track element might have
changed its state to Loading waiting for a call to didCompleteLoad to continue.
On 2015/03/12 15:22:18, fs wrote:
> On 2015/03/12 04:24:08, philipj_UTC7 wrote:
> > This is the track element, and this comment doesn't seem to catch the gist
of
> > what's going on: if the load has already finished (even if failed) we'll
just
> > report the same state again. Perhaps a switch on m_loader->loadState() would
> > make this a bit clearer?
> 
> Adding switch and dropping this comment (which I have trouble straightening to
> something comprehensible...)
>  
> > I also wonder why we have the url==m_url case at all, do you know what
> terrible
> > things would happen if we just tried to load it like any other URL?
> 
> Hopefully that's fine... Given that we don't recreate the loader etc. (I.e. we
> get the same resource from cache and nothing else). I'd put that in the "NEEDS
> TEST" bucket though =)

New structure looks much more sane to me, thanks!

[commit-bot: I haz the power, 2015-03-13 03:27:44]

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

[commit-bot: I haz the power, 2015-03-13 03:30:47]

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