Changeset 166238 in webkit

Timestamp:

Mar 25, 2014 9:51:40 AM (10 years ago)

Author:

Brent Fulgham

Message:

Prevent 'removetrack' events from firing when all inband text tracks are removed.
​https://bugs.webkit.org/show_bug.cgi?id=130704

Reviewed by Eric Carlson.

Source/WebCore:

Test: media/track/track-remove-track.html

Based on the Blink change (patch by ​acolwell@chromium.org):
​https://codereview.chromium.org/177243018/

• html/HTMLMediaElement.cpp:

(WebCore::HTMLMediaElement::prepareForLoad): Reorder steps to match W3C specification.
(WebCore::HTMLMediaElement::noneSupported): Forget tracks as required by specification.
(WebCore::HTMLMediaElement::mediaLoadingFailed): Forget tracks as required by specification.
(WebCore::HTMLMediaElement::removeTextTrack): Only request the 'removetracks' event if
requested by caller.
(WebCore::HTMLMediaElement::removeAllInbandTracks): Renamed to 'forgetResourceSpecificTracks'
(WebCore::HTMLMediaElement::noneSupported): Specify that we want the 'removetracks' event
fired for this use case.
(WebCore::HTMLMediaElement::prepareForLoad): Switch to new 'forgetResourceSpecificTracks' name.

• html/HTMLMediaElement.h:
• html/track/TextTrackList.cpp:

(TextTrackList::remove): Only fire the 'removetrack' event if the caller requests it.

• html/track/TextTrackList.h: Add default argument to fire the 'removetrack' event

when removing a track.

• html/track/TrackListBase.cpp:

(TrackListBase::remove): Only fire the 'removetrack' event if the caller requests it.

• html/track/TrackListBase.h: Add default argument to fire the 'removetrack' event.

LayoutTests:

Based on the Blink change (patch by ​acolwell@chromium.org):
​https://codereview.chromium.org/177243018/

• media/track/track-remove-track-expected.txt: Added.
• media/track/track-remove-track.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/media/track/track-remove-track-expected.txt (added)
• LayoutTests/media/track/track-remove-track.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLMediaElement.cpp (modified) (11 diffs)
• Source/WebCore/html/HTMLMediaElement.h (modified) (1 diff)
• Source/WebCore/html/track/TextTrackList.cpp (modified) (2 diffs)
• Source/WebCore/html/track/TextTrackList.h (modified) (1 diff)
• Source/WebCore/html/track/TrackListBase.cpp (modified) (2 diffs)
• Source/WebCore/html/track/TrackListBase.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2014-03-24  Brent Fulgham  <bfulgham@apple.com>
+
+        Prevent 'removetrack' events from firing when all inband text tracks are removed.
+        https://bugs.webkit.org/show_bug.cgi?id=130704
+
+        Reviewed by Eric Carlson.
+
+        Based on the Blink change (patch by acolwell@chromium.org):
+        https://codereview.chromium.org/177243018/
+
+        * media/track/track-remove-track-expected.txt: Added.
+        * media/track/track-remove-track.html: Added.
+
 2014-03-25  Alexey Proskuryakov  <ap@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-03-24  Brent Fulgham  <bfulgham@apple.com>
+
+        Prevent 'removetrack' events from firing when all inband text tracks are removed.
+        https://bugs.webkit.org/show_bug.cgi?id=130704
+
+        Reviewed by Eric Carlson.
+
+        Test: media/track/track-remove-track.html
+
+        Based on the Blink change (patch by acolwell@chromium.org):
+        https://codereview.chromium.org/177243018/
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::prepareForLoad): Reorder steps to match W3C specification.
+        (WebCore::HTMLMediaElement::noneSupported): Forget tracks as required by specification.
+        (WebCore::HTMLMediaElement::mediaLoadingFailed): Forget tracks as required by specification.
+        (WebCore::HTMLMediaElement::removeTextTrack): Only request the 'removetracks' event if
+        requested by caller.
+        (WebCore::HTMLMediaElement::removeAllInbandTracks): Renamed to 'forgetResourceSpecificTracks'
+        (WebCore::HTMLMediaElement::noneSupported): Specify that we want the 'removetracks' event
+        fired for this use case.
+        (WebCore::HTMLMediaElement::prepareForLoad): Switch to new 'forgetResourceSpecificTracks' name.
+        * html/HTMLMediaElement.h:
+        * html/track/TextTrackList.cpp:
+        (TextTrackList::remove): Only fire the 'removetrack' event if the caller requests it.
+        * html/track/TextTrackList.h: Add default argument to fire the 'removetrack' event
+        when removing a track.
+        * html/track/TrackListBase.cpp:
+        (TrackListBase::remove): Only fire the 'removetrack' event if the caller requests it.
+        * html/track/TrackListBase.h: Add default argument to fire the 'removetrack' event.
+
 2014-03-25  David Kilzer  <ddkilzer@apple.com>
 

trunk/Source/WebCore/html/HTMLMediaElement.cpp

@@ -979 +979 @@
     // 4 - If the media element's networkState is not set to NETWORK_EMPTY, then run these substeps
     if (m_networkState != NETWORK_EMPTY) {
+        // 4.1 - Queue a task to fire a simple event named emptied at the media element.
+        scheduleEvent(eventNames().emptiedEvent);
+
+        // 4.2 - If a fetching process is in progress for the media element, the user agent should stop it.
         m_networkState = NETWORK_EMPTY;
+
+        // 4.3 - Forget the media element's media-resource-specific tracks.
+        forgetResourceSpecificTracks();
+
+        // 4.4 - If readyState is not set to HAVE_NOTHING, then set it to that state.
         m_readyState = HAVE_NOTHING;
         m_readyStateMaximum = HAVE_NOTHING;
+
+        // 4.5 - If the paused attribute is false, then set it to true.
+        m_paused = true;
+
+        // 4.6 - If seeking is true, set it to false.
+        m_seeking = false;
+
+        // 4.7 - Set the current playback position to 0.
+        //       Set the official playback position to 0.
+        //       If this changed the official playback position, then queue a task to fire a simple event named timeupdate at the media element.
+        // FIXME: Add support for firing this event. e.g., scheduleEvent(eventNames().timeUpdateEvent);
+
+        // 4.8 - Set the initial playback position to 0.
+        // FIXME: Make this less subtle. The position only becomes 0 because of the createMediaPlayer() call
+        // above.
         refreshCachedTime();
-        m_paused = true;
-        m_seeking = false;
+
         invalidateCachedTime();
-        scheduleEvent(eventNames().emptiedEvent);
+
+        // 4.9 - Set the timeline offset to Not-a-Number (NaN).
+        // 4.10 - Update the duration attribute to Not-a-Number (NaN).
+
         updateMediaController();
 #if ENABLE(VIDEO_TRACK)
@@ -1012 +1038 @@
 
     m_playedTimeRanges = TimeRanges::create();
+
+    // FIXME: Investigate whether these can be moved into m_networkState != NETWORK_EMPTY block above
+    // so they are closer to the relevant spec steps.
     m_lastSeekTime = 0;
 
@@ -1793 +1822 @@
 
     // 6.2 - Forget the media element's media-resource-specific text tracks.
+    forgetResourceSpecificTracks();
 
     // 6.3 - Set the element's networkState attribute to the NETWORK_NO_SOURCE value.
@@ -1918 +1948 @@
     if (m_readyState < HAVE_METADATA && m_loadState == LoadingFromSourceElement) {
         
+        // resource selection algorithm
+        // Step 9.Otherwise.9 - Failed with elements: Queue a task, using the DOM manipulation task source, to fire a simple event named error at the candidate element.
         if (m_currentSourceNode)
             m_currentSourceNode->scheduleErrorEvent();
@@ -1923 +1955 @@
             LOG(Media, "HTMLMediaElement::setNetworkState - error event not sent, <source> was removed");
         
+        // 9.Otherwise.10 - Asynchronously await a stable state. The synchronous section consists of all the remaining steps of this algorithm until the algorithm says the synchronous section has ended.
+        
+        // 9.Otherwise.11 - Forget the media element's media-resource-specific tracks.
+        forgetResourceSpecificTracks();
+
         if (havePotentialSourceChild()) {
             LOG(Media, "HTMLMediaElement::setNetworkState - scheduling next <source>");
@@ -3366 +3403 @@
 }
 
-void HTMLMediaElement::removeTextTrack(TextTrack* track)
+void HTMLMediaElement::removeTextTrack(TextTrack* track, bool scheduleEvent)
 {
     if (!RuntimeEnabledFeatures::sharedFeatures().webkitVideoTrackEnabled())
@@ -3376 +3413 @@
         textTrackRemoveCues(track, cues);
     track->clearClient();
-    m_textTracks->remove(track);
+    m_textTracks->remove(track, scheduleEvent);
 
     closeCaptionTracksChanged();
@@ -3389 +3426 @@
 }
 
-void HTMLMediaElement::removeAllInbandTracks()
+void HTMLMediaElement::forgetResourceSpecificTracks()
 {
     while (m_audioTracks &&  m_audioTracks->length())
@@ -3400 +3437 @@
 
             if (track->trackType() == TextTrack::InBand)
-                removeTextTrack(track);
+                removeTextTrack(track, false);
         }
     }
@@ -4520 +4557 @@
 
 #if ENABLE(VIDEO_TRACK)
-    removeAllInbandTracks();
+    forgetResourceSpecificTracks();
 #endif
 
@@ -5425 +5462 @@
 
 #if ENABLE(VIDEO_TRACK)
-    removeAllInbandTracks();
+    forgetResourceSpecificTracks();
 #endif
     m_player = MediaPlayer::create(this);

trunk/Source/WebCore/html/HTMLMediaElement.h

@@ -291 +291 @@
     void addVideoTrack(PassRefPtr<VideoTrack>);
     void removeAudioTrack(AudioTrack*);
-    void removeTextTrack(TextTrack*);
+    void removeTextTrack(TextTrack*, bool scheduleEvent = true);
     void removeVideoTrack(VideoTrack*);
-    void removeAllInbandTracks();
+    void forgetResourceSpecificTracks();
     void closeCaptionTracksChanged();
     void notifyMediaPlayerOfTextTrackChanges();

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

@@ -196 +196 @@
 }
 
-void TextTrackList::remove(TrackBase* track)
+void TextTrackList::remove(TrackBase* track, bool scheduleEvent)
 {
     TextTrack* textTrack = toTextTrack(track);
@@ -220 +220 @@
     RefPtr<TrackBase> trackRef = (*tracks)[index];
     tracks->remove(index);
-    scheduleRemoveTrackEvent(trackRef.release());
+
+    if (scheduleEvent)
+        scheduleRemoveTrackEvent(trackRef.release());
 }
 

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

@@ -53 +53 @@
 
     void append(PassRefPtr<TextTrack>);
-    virtual void remove(TrackBase*) override;
+    virtual void remove(TrackBase*, bool scheduleEvent = true) override;
 
     // EventTarget

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

@@ -59 +59 @@
 }
 
-void TrackListBase::remove(TrackBase* track)
+void TrackListBase::remove(TrackBase* track, bool scheduleEvent)
 {
     size_t index = m_inbandTracks.find(track);
@@ -71 +71 @@
     m_inbandTracks.remove(index);
 
-    scheduleRemoveTrackEvent(trackRef.release());
+    if (scheduleEvent)
+        scheduleRemoveTrackEvent(trackRef.release());
 }
 

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

@@ -49 +49 @@
     virtual unsigned length() const;
     virtual bool contains(TrackBase*) const;
-    virtual void remove(TrackBase*);
+    virtual void remove(TrackBase*, bool scheduleEvent = true);
 
     // EventTarget