Move loading machinery out of LoadableTextTrack

Source/core/html/HTMLTrackElement.cpp

Index: Source/core/html/HTMLTrackElement.cpp
diff --git a/Source/core/html/HTMLTrackElement.cpp b/Source/core/html/HTMLTrackElement.cpp
index 3a0871731d4353a86ac37e08e7e5b6eb71140730..f32692bbf0186a360ea24f5be048e575dba16c9d 100644
--- a/Source/core/html/HTMLTrackElement.cpp
+++ b/Source/core/html/HTMLTrackElement.cpp
@@ -26,12 +26,12 @@
 #include "config.h"
 #include "core/html/HTMLTrackElement.h"
 
-#include "bindings/core/v8/ExceptionStatePlaceholder.h"
 #include "core/HTMLNames.h"
 #include "core/dom/Document.h"
 #include "core/events/Event.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/html/HTMLMediaElement.h"
+#include "core/html/track/LoadableTextTrack.h"
 #include "platform/Logging.h"
 
 namespace blink {
@@ -156,31 +156,56 @@ void HTMLTrackElement::scheduleLoad()
     if (!mediaElement())
         return;
 
-    // 4. Run the remainder of these steps asynchronously, allowing whatever caused these steps to run to continue.
+    // 4. Run the remainder of these steps in parallel, allowing whatever caused these steps to run to continue.
     m_loadTimer.startOneShot(0, FROM_HERE);
 }
 
 void HTMLTrackElement::loadTimerFired(Timer<HTMLTrackElement>*)
 {
-    if (!fastHasAttribute(srcAttr))
-        return;
-
     WTF_LOG(Media, "HTMLTrackElement::loadTimerFired");
 
-    // 6. Set the text track readiness state to loading.
-    setReadyState(HTMLTrackElement::LOADING);
+    // 5. Top: Await a stable state. The synchronous section consists of the following steps. (The steps in the

[philipj_slow, 2015/03/12 04:24:07]

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.

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

Moved, FIXME added.

+    // synchronous section are marked with [X])
 
-    // 7. Let URL be the track URL of the track element.
+    // 6. [X] Set the text track readiness state to loading.
+    setReadyState(LOADING);
+
+    // 7. [X] Let URL be the track URL of the track element.
     KURL url = getNonEmptyURLAttribute(srcAttr);
 
-    // 8. If the track element's parent is a media element then let CORS mode be the state of the parent media
+    // 8. [X] If the track element's parent is a media element then let CORS mode be the state of the parent media
     // element's crossorigin content attribute. Otherwise, let CORS mode be No CORS.
+    const AtomicString& corsMode = mediaElementCrossOriginAttribute();
+
+    // 9. End the synchronous section, continuing the remaining steps in parallel.
+
+    // 10. If URL is not the empty string, perform a potentially CORS-enabled fetch of URL, with the mode being CORS
+    // mode, the origin being the origin of the track element's node document, and the default origin behaviour set to
+    // fail.
     if (!canLoadUrl(url)) {
-        didCompleteLoad(HTMLTrackElement::Failure);
+        didCompleteLoad(Failure);
+        return;
+    }
+
+    if (url == m_url) {
+        ASSERT(m_loader);
+        // If loading of the resource from this URL is in progress, return early.
+        if (m_loader->loadState() < TextTrackLoader::Finished)
+            return;
+
+        // The track element might have changed its state to Loading waiting for a call to didCompleteLoad to continue.

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

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?

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

Adding switch and dropping this comment (which I have trouble straightening to
something comprehensible...)
 
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 =)

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

New structure looks much more sane to me, thanks!

+        didCompleteLoad(m_loader->loadState() == TextTrackLoader::Failed ? Failure : Success);
         return;
     }
 
-    ensureTrack()->scheduleLoad(url);
+    m_url = url;
+
+    if (m_loader)
+        m_loader->cancelLoad();
+
+    m_loader = TextTrackLoader::create(*this, document());
+    if (!m_loader->load(m_url, corsMode))
+        didCompleteLoad(Failure);
 }
 
 bool HTMLTrackElement::canLoadUrl(const KURL& url)
@@ -189,11 +214,6 @@ bool HTMLTrackElement::canLoadUrl(const KURL& url)
     if (!parent)
         return false;
 
-    // 4.8.10.12.3 Sourcing out-of-band text tracks
-
-    // 4. Download: If URL is not the empty string, perform a potentially CORS-enabled fetch of URL, with the
-    // mode being the state of the media element's crossorigin content attribute, the origin being the
-    // origin of the media element's Document, and the default origin behaviour set to fail.
     if (url.isEmpty())
         return false;
 
@@ -207,30 +227,58 @@ bool HTMLTrackElement::canLoadUrl(const KURL& url)
 
 void HTMLTrackElement::didCompleteLoad(LoadStatus status)
 {
-    // 4.8.10.12.3 Sourcing out-of-band text tracks (continued)
-
-    // 4. Download: ...
-    // If the fetching algorithm fails for any reason (network error, the server returns an error
-    // code, a cross-origin check fails, etc), or if URL is the empty string or has the wrong origin
-    // as determined by the condition at the start of this step, or if the fetched resource is not in
-    // a supported format, then queue a task to first change the text track readiness state to failed
-    // to load and then fire a simple event named error at the track element; and then, once that task
-    // is queued, move on to the step below labeled monitoring.
-
+    // 10. ... (continued)
+
+    // If the fetching algorithm fails for any reason (network error, the server returns an error code, a cross-origin
+    // check fails, etc), or if URL is the empty string, then queue a task to first change the text track readiness

[philipj_slow, 2015/03/12 04:24:07]

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]

Done.

+    // state to failed to load and then fire a simple event named error at the track element. This task must use the DOM
+    // manipulation task source.
+    //
+    // If the fetching algorithm does not fail, but the type of the resource is not a supported text track format, or
+    // the file was not successfully processed (e.g. the format in question is an XML format and the file contained a
+    // well-formedness error that the XML specification requires be detected and reported to the application), then the
+    // task that is queued by the networking task source in which the aforementioned problem is found must change the
+    // text track readiness state to failed to load and fire a simple event named error at the track element.
     if (status == Failure) {
-        setReadyState(HTMLTrackElement::TRACK_ERROR);
-        dispatchEvent(Event::create(EventTypeNames::error), IGNORE_EXCEPTION);
+        setReadyState(TRACK_ERROR);
+        dispatchEvent(Event::create(EventTypeNames::error));
         return;
     }
 
-    // If the fetching algorithm does not fail, then the final task that is queued by the networking
-    // task source must run the following steps:
-    //     1. Change the text track readiness state to loaded.
-    setReadyState(HTMLTrackElement::LOADED);
+    // If the fetching algorithm does not fail, and the file was successfully processed, then the final task that is
+    // queued by the networking task source, after it has finished parsing the data, must change the text track
+    // readiness state to loaded, and fire a simple event named load at the track element.
+    setReadyState(LOADED);
+    dispatchEvent(Event::create(EventTypeNames::load));
+}
+
+void HTMLTrackElement::newCuesAvailable(TextTrackLoader* loader)
+{
+    ASSERT_UNUSED(loader, m_loader == loader);
+    ASSERT(m_track);
+
+    WillBeHeapVector<RefPtrWillBeMember<TextTrackCue>> newCues;
+    m_loader->getNewCues(newCues);
+
+    m_track->addListOfCues(newCues);
+}
+
+void HTMLTrackElement::newRegionsAvailable(TextTrackLoader* loader)
+{
+    ASSERT_UNUSED(loader, m_loader == loader);
+    ASSERT(m_track);
+
+    WillBeHeapVector<RefPtrWillBeMember<VTTRegion>> newRegions;
+    m_loader->getNewRegions(newRegions);
+
+    m_track->addRegions(newRegions);
+}
+
+void HTMLTrackElement::cueLoadingCompleted(TextTrackLoader* loader, bool loadingFailed)
+{
+    ASSERT_UNUSED(loader, m_loader == loader);
 
-    //     2. If the file was successfully processed, fire a simple event named load at the
-    //        track element.
-    dispatchEvent(Event::create(EventTypeNames::load), IGNORE_EXCEPTION);
+    didCompleteLoad(loadingFailed ? Failure : Success);
 }
 
 // NOTE: The values in the TextTrack::ReadinessState enum must stay in sync with those in HTMLTrackElement::ReadyState.
@@ -270,6 +318,7 @@ HTMLMediaElement* HTMLTrackElement::mediaElement() const
 DEFINE_TRACE(HTMLTrackElement)
 {
     visitor->trace(m_track);
+    visitor->trace(m_loader);
     HTMLElement::trace(visitor);
 }