Merge r221155 - HTMLTrackElement behavior violates the standard

https://bugs.webkit.org/show_bug.cgi?id=175888 Patch by Kirill Ovchinnikov <kirill.ovchinn@gmail.com> on 2017-08-24 Reviewed by Eric Carlson. Source/WebCore: Test: media/track/text-track-src-change.html: added asserts * html/HTMLTrackElement.cpp: (WebCore::HTMLTrackElement::parseAttribute): (WebCore::HTMLTrackElement::loadTimerFired): * html/track/LoadableTextTrack.cpp: (WebCore::LoadableTextTrack::scheduleLoad): * html/track/TextTrack.cpp: (WebCore::TextTrack::removeAllCues): * html/track/TextTrackCueList.cpp: (WebCore::TextTrackCueList::removeAll): * html/track/TextTrackCueList.h: LayoutTests: * media/track/text-track-src-change-expected.txt: * media/track/text-track-src-change.html:
WebKit · Aug 30, 2017 · 1d4e439 · 1d4e439
1 parent 2222e36
commit 1d4e439
Show file tree

Hide file tree

Showing 9 changed files with 99 additions and 16 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2017-08-24  Kirill Ovchinnikov  <kirill.ovchinn@gmail.com>
+
+        HTMLTrackElement behavior violates the standard
+        https://bugs.webkit.org/show_bug.cgi?id=175888
+
+        Reviewed by Eric Carlson.
+
+        * media/track/text-track-src-change-expected.txt:
+        * media/track/text-track-src-change.html:
+
 2017-08-23  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         DeleteSelectionCommand should be robust when starting and ending editable positions cannot be found

diff --git a/LayoutTests/media/track/text-track-src-change-expected.txt b/LayoutTests/media/track/text-track-src-change-expected.txt
@@ -7,8 +7,19 @@ EXPECTED (cues.length == '4') OK
 
 *** Changing 'src' attribute...
 EXPECTED (cues.length == '100') OK
+EXPECTED (cues === testTrack.track.cues == 'true') OK
 
 *** Changing back 'src' attribute...
+EXPECTED (cues === testTrack.track.cues == 'true') OK
 EXPECTED (cues.length == '4') OK
+
+*** Setting 'src' to the same value...
+EXPECTED (cues === testTrack.track.cues == 'true') OK
+EXPECTED (cues.length == '4') OK
+
+*** Setting 'src' to an empty value...
+EXPECTED (stage == '5') OK
+EXPECTED (true == 'true') OK
+EXPECTED (cues.length == '0') OK
 END OF TEST
 
diff --git a/LayoutTests/media/track/text-track-src-change.html b/LayoutTests/media/track/text-track-src-change.html
@@ -7,8 +7,9 @@
         <script src=../video-test.js></script>
         <script>
             var stage = 0; 
+            var errorEventTimer = null;
 
-            function trackLoaded()
+            function loadedTrackTest()
             {
                 var testTrack = document.getElementById('testTrack');
                 switch (stage) {
@@ -21,27 +22,58 @@
                         testTrack.src = "captions-webvtt/captions-long.vtt";
                         break;
                     case 1:
-                        cues = testTrack.track.cues;
                         testExpected("cues.length", 100);
+                        testExpected("cues === testTrack.track.cues", true);
                         consoleWrite("<br>*** Changing back 'src' attribute...");
                         ++stage;
                         testTrack.src = "captions-webvtt/tc013-settings.vtt";
                         break;
                     case 2:
-                        cues = testTrack.track.cues;
+                        testExpected("cues === testTrack.track.cues", true);
                         testExpected("cues.length", 4);
-                        endTest();
+                        consoleWrite("<br>*** Setting 'src' to the same value...");
+                        ++stage;
+                        testTrack.src = "captions-webvtt/tc013-settings.vtt";
+                        // Let's ensure that 'load' event is not fired after that
+                        // - it should jump directly to the stage 4 instead of stage 3
+                        setTimeout(function() { ++stage; loadedTrackTest(); }, 100);
+                        break;
+                    case 3:
+                        failTest("'load' event should not be fired after setting the same URL");
+                        break;
+                    case 4:
+                        testExpected("cues === testTrack.track.cues", true);
+                        testExpected("cues.length", 4);
+                        consoleWrite("<br>*** Setting 'src' to an empty value...");
+                        ++stage;
+                        testTrack.src = ""; // this should fire 'error' event
+                        errorEventTimer = setTimeout(function() {
+                                failTest("'error' event didn't fire after setting an empty URL");
+                            }, 10000);
+                        break;
+                    case 5:
+                        failTest("'load' event should not be fired after setting an empty URL");
                         break;
                 }
             }
 
+            function trackLoadFailed()
+            {
+                clearTimeout(errorEventTimer);
+                testExpected("stage", 5);
+                testExpected(cues === testTrack.track.cues, true);
+                testExpected("cues.length", 0);
+                endTest();
+            }
+
+
             setCaptionDisplayMode('Automatic');
         </script>
     </head>
     <body>
         <p>Tests Track 'src' changing handling</p>
         <video>
-            <track id="testTrack" src="captions-webvtt/tc013-settings.vtt" kind="captions" onload="trackLoaded()" default>
+            <track id="testTrack" src="captions-webvtt/tc013-settings.vtt" kind="captions" onload="loadedTrackTest()" onerror="trackLoadFailed()" default>
         </video>
     </body>
 </html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,23 @@
+2017-08-24  Kirill Ovchinnikov  <kirill.ovchinn@gmail.com>
+
+        HTMLTrackElement behavior violates the standard
+        https://bugs.webkit.org/show_bug.cgi?id=175888
+
+        Reviewed by Eric Carlson.
+
+        Test: media/track/text-track-src-change.html: added asserts
+
+        * html/HTMLTrackElement.cpp:
+        (WebCore::HTMLTrackElement::parseAttribute):
+        (WebCore::HTMLTrackElement::loadTimerFired):
+        * html/track/LoadableTextTrack.cpp:
+        (WebCore::LoadableTextTrack::scheduleLoad):
+        * html/track/TextTrack.cpp:
+        (WebCore::TextTrack::removeAllCues):
+        * html/track/TextTrackCueList.cpp:
+        (WebCore::TextTrackCueList::removeAll):
+        * html/track/TextTrackCueList.h:
+
 2017-08-23  Adrian Perez de Castro  <aperez@igalia.com>
 
         Geoclue2 based backend should provide the right desktop ID

diff --git a/Source/WebCore/html/HTMLTrackElement.cpp b/Source/WebCore/html/HTMLTrackElement.cpp
@@ -100,10 +100,7 @@ void HTMLTrackElement::removedFrom(ContainerNode& insertionPoint)
 void HTMLTrackElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     if (name == srcAttr) {
-        if (!value.isEmpty())
-            scheduleLoad();
-        else if (m_track)
-            m_track->removeAllCues();
+        scheduleLoad();
 
     // 4.8.10.12.3 Sourcing out-of-band text tracks
     // As the kind, label, and srclang attributes are set, changed, or removed, the text track must update accordingly...
@@ -186,26 +183,27 @@ void HTMLTrackElement::scheduleLoad()
 
 void HTMLTrackElement::loadTimerFired()
 {
-    if (!hasAttributeWithoutSynchronization(srcAttr))
+    if (!hasAttributeWithoutSynchronization(srcAttr)) {
+        track().removeAllCues();
         return;
+    }
 
     // 6. Set the text track readiness state to loading.
     setReadyState(HTMLTrackElement::LOADING);
 
     // 7. Let URL be the track URL of the track element.
     URL url = getNonEmptyURLAttribute(srcAttr);
 
+    // ... if URL is the empty string, then queue a task to first change the text track readiness state
+    // to failed to load and then fire an event named error at the track element.
     // 8. 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.
-    if (!canLoadURL(url)) {
+    if (url.isEmpty() || !canLoadURL(url)) {
+        track().removeAllCues();
         didCompleteLoad(HTMLTrackElement::Failure);
         return;
     }
 
-    // When src attribute is changed we need to flush all collected track data
-    if (m_track)
-        m_track->removeAllCues();
-
     track().scheduleLoad(url);
 }
 

diff --git a/Source/WebCore/html/track/LoadableTextTrack.cpp b/Source/WebCore/html/track/LoadableTextTrack.cpp
@@ -48,6 +48,9 @@ void LoadableTextTrack::scheduleLoad(const URL& url)
     if (url == m_url)
         return;
 
+    // When src attribute is changed we need to flush all collected track data
+    removeAllCues();
+
     // 4.8.10.12.3 Sourcing out-of-band text tracks (continued)
 
     // 2. Let URL be the track URL of the track element.

diff --git a/Source/WebCore/html/track/TextTrack.cpp b/Source/WebCore/html/track/TextTrack.cpp
@@ -269,7 +269,7 @@ void TextTrack::removeAllCues()
     for (size_t i = 0; i < m_cues->length(); ++i)
         m_cues->item(i)->setTrack(nullptr);
 
-    m_cues = nullptr;
+    m_cues->clear();
 }
 
 TextTrackCueList* TextTrack::activeCues() const

diff --git a/Source/WebCore/html/track/TextTrackCueList.cpp b/Source/WebCore/html/track/TextTrackCueList.cpp
@@ -106,6 +106,13 @@ void TextTrackCueList::remove(TextTrackCue& cue)
     ASSERT_SORTED(m_vector.begin(), m_vector.end());
 }
 
+void TextTrackCueList::clear()
+{
+    m_vector.clear();
+    if (m_activeCues)
+        m_activeCues->m_vector.clear();
+}
+
 void TextTrackCueList::updateCueIndex(TextTrackCue& cue)
 {
     auto cuePosition = m_vector.begin() + cueIndex(cue);

diff --git a/Source/WebCore/html/track/TextTrackCueList.h b/Source/WebCore/html/track/TextTrackCueList.h
@@ -46,6 +46,8 @@ class TextTrackCueList : public RefCounted<TextTrackCueList> {
     void remove(TextTrackCue&);
     void updateCueIndex(TextTrackCue&);
 
+    void clear();
+
     TextTrackCueList& activeCues();
 
 private: