Changeset 221155 in webkit

Timestamp:

Aug 24, 2017 12:44:20 PM (7 years ago)

Author:

commit-queue@webkit.org

Message:

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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/media/track/text-track-src-change-expected.txt (modified) (1 diff)
• LayoutTests/media/track/text-track-src-change.html (modified) (4 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLTrackElement.cpp (modified) (3 diffs)
• Source/WebCore/html/track/LoadableTextTrack.cpp (modified) (1 diff)
• Source/WebCore/html/track/TextTrack.cpp (modified) (1 diff)
• Source/WebCore/html/track/TextTrackCueList.cpp (modified) (1 diff)
• Source/WebCore/html/track/TextTrackCueList.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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-24  Jonathan Bedard  <jbedard@apple.com>
 

trunk/LayoutTests/media/track/text-track-src-change-expected.txt

@@ -8 +8 @@
 *** 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
 

trunk/LayoutTests/media/track/text-track-src-change.html

@@ -8 +8 @@
         <script>
             var stage = 0; 
+            var errorEventTimer = null;
 
-            function trackLoaded()
+            function loadedTrackTest()
             {
                 var testTrack = document.getElementById('testTrack');
@@ -22 +23 @@
                         break;
                     case 1:
-                        cues = testTrack.track.cues;
                         testExpected("cues.length", 100);
+                        testExpected("cues === testTrack.track.cues", true);
                         consoleWrite("<br>*** Changing back 'src' attribute...");
                         ++stage;
@@ -29 +30 @@
                         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');
@@ -42 +74 @@
         <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>

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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-24  David Kilzer  <ddkilzer@apple.com>
 

trunk/Source/WebCore/html/HTMLTrackElement.cpp

@@ -101 +101 @@
 {
     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
@@ -187 +184 @@
 void HTMLTrackElement::loadTimerFired()
 {
-    if (!hasAttributeWithoutSynchronization(srcAttr))
-        return;
+    if (!hasAttributeWithoutSynchronization(srcAttr)) {
+        track().removeAllCues();
+        return;
+    }
 
     // 6. Set the text track readiness state to loading.
@@ -196 +195 @@
     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);

trunk/Source/WebCore/html/track/LoadableTextTrack.cpp

@@ -48 +48 @@
     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)

trunk/Source/WebCore/html/track/TextTrack.cpp

@@ -270 +270 @@
         m_cues->item(i)->setTrack(nullptr);
     
-    m_cues = nullptr;
+    m_cues->clear();
 }
 

trunk/Source/WebCore/html/track/TextTrackCueList.cpp

@@ -107 +107 @@
 }
 
+void TextTrackCueList::clear()
+{
+    m_vector.clear();
+    if (m_activeCues)
+        m_activeCues->m_vector.clear();
+}
+
 void TextTrackCueList::updateCueIndex(TextTrackCue& cue)
 {

trunk/Source/WebCore/html/track/TextTrackCueList.h

@@ -47 +47 @@
     void updateCueIndex(TextTrackCue&);
 
+    void clear();
+
     TextTrackCueList& activeCues();