Refactor resource load and resource selection algorithms as per spec

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index 77aa5fe35409933471eb164bbb4b5be025ecc02e..24fd790d3bdbf8943b02a9c8ce1c9b036931dd0e 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -471,8 +471,8 @@ void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument)
     // A proper fix would provide a mechanism to allow this object to refresh
     // the MediaPlayer's LocalFrame and FrameLoader references on
     // document changes so that playback can be resumed properly.
-    clearMediaPlayer(LoadMediaResource);
-    scheduleDelayedAction(LoadMediaResource);
+    prepareForLoad();
+    scheduleNextSourceChild();
 
     // Decrement the load event delay count on oldDocument now that m_webMediaPlayer has been destroyed
     // and there is no risk of dispatching a load event from within the destructor.
@@ -501,8 +501,8 @@ void HTMLMediaElement::parseAttribute(const QualifiedName& name, const AtomicStr
     if (name == srcAttr) {
         // Trigger a reload, as long as the 'src' attribute is present.
         if (!value.isNull()) {
-            clearMediaPlayer(LoadMediaResource);
-            scheduleDelayedAction(LoadMediaResource);
+            prepareForLoad();

[philipj_slow, 2016/01/21 12:48:11]

The spec says "If a src attribute of a media element is set or changed, the user
agent must invoke the media element's media element load algorithm." If you want
to wrap these two in a new invokeLoadAlgorithm() that might be nice.

[Srirama, 2016/01/28 11:31:03]

Done.

+            scheduleNextSourceChild();
         }
     } else if (name == controlsAttr) {
         configureMediaControls();
@@ -520,7 +520,7 @@ void HTMLMediaElement::finishParsingChildren()
     HTMLElement::finishParsingChildren();
 
     if (Traversal<HTMLTrackElement>::firstChild(*this))
-        scheduleDelayedAction(LoadTextTrackResource);
+        scheduleTextTrackResourceLoad();
 }
 
 bool HTMLMediaElement::layoutObjectIsNeeded(const ComputedStyle& style)
@@ -539,8 +539,12 @@ Node::InsertionNotificationRequest HTMLMediaElement::insertedInto(ContainerNode*
 
     HTMLElement::insertedInto(insertionPoint);
     if (insertionPoint->inDocument()) {
-        if (!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY)
-            scheduleDelayedAction(LoadMediaResource);
+        if (!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) {
+            if (!(m_pendingActionFlags & LoadMediaResource))

[philipj_slow, 2016/01/21 12:48:11]

I'm not sure I understand these changes. The existing behavior is not per spec,
as inserting a media element into a document shouldn't actually do anything.
IIRC, this is actually needed here because removing from a document will destroy
the media player. Would it work to just invoke the resource selection algorithm
here? Or the media element load algorithm?

It also doesn't really make sense that this is looking at the src attribute,
when there could also be source element children. If the reason for that is that
inserting a source element child will invoke the resource selection algorithm,
then I think doing the same here makes the most sense. Please also decorate this
with a TODO explaining why it's actually wrong and not per spec.

[Srirama, 2016/01/28 11:31:03]

Removed unneeded conditions. But i couldn't exactly figure the spec location for
this, i could see the spec corresponding to source element insertion but
couldn't see the same thing for media element.

+                prepareForLoad();
+
+            scheduleNextSourceChild();
+        }
     }
 
     return InsertionShouldCallDidNotifySubtreeInsertions;
@@ -577,17 +581,11 @@ void HTMLMediaElement::didRecalcStyle(StyleRecalcChange)
         layoutObject()->updateFromElement();
 }
 
-void HTMLMediaElement::scheduleDelayedAction(DelayedActionType actionType)
+void HTMLMediaElement::scheduleTextTrackResourceLoad()
 {
-    WTF_LOG(Media, "HTMLMediaElement::scheduleDelayedAction(%p)", this);
-
-    if ((actionType & LoadMediaResource) && !(m_pendingActionFlags & LoadMediaResource)) {
-        prepareForLoad();
-        m_pendingActionFlags |= LoadMediaResource;
-    }
+    WTF_LOG(Media, "HTMLMediaElement::scheduleTextTrackResourceLoad(%p)", this);
 
-    if (actionType & LoadTextTrackResource)
-        m_pendingActionFlags |= LoadTextTrackResource;
+    m_pendingActionFlags |= LoadTextTrackResource;
 
     if (!m_loadTimer.isActive())
         m_loadTimer.startOneShot(0, BLINK_FROM_HERE);
@@ -597,7 +595,8 @@ void HTMLMediaElement::scheduleNextSourceChild()
 {
     // Schedule the timer to try the next <source> element WITHOUT resetting state ala prepareForLoad.
     m_pendingActionFlags |= LoadMediaResource;
-    m_loadTimer.startOneShot(0, BLINK_FROM_HERE);
+    if (!m_loadTimer.isActive())

[philipj_slow, 2016/01/21 12:48:11]

Why was this change needed?

[Srirama, 2016/01/28 11:31:03]

Removed, not needed, i have added it to fix layout test case failures, but i
think those failures may be because of the changes in above places. After fixing
your review comments all the test cases are passing.

+        m_loadTimer.startOneShot(0, BLINK_FROM_HERE);
 }
 
 void HTMLMediaElement::scheduleEvent(const AtomicString& eventName)
@@ -717,18 +716,8 @@ void HTMLMediaElement::prepareForLoad()
 {
     WTF_LOG(Media, "HTMLMediaElement::prepareForLoad(%p)", this);
 
-    // Perform the cleanup required for the resource load algorithm to run.
-    stopPeriodicTimers();
-    m_loadTimer.stop();
-    cancelDeferredLoad();
-    // FIXME: Figure out appropriate place to reset LoadTextTrackResource if necessary and set m_pendingActionFlags to 0 here.
-    m_pendingActionFlags &= ~LoadMediaResource;
-    m_sentEndEvent = false;
-    m_sentStalledEvent = false;
-    m_haveFiredLoadedData = false;
-    m_completelyLoaded = false;
-    m_havePreparedToPlay = false;
-    m_displayMode = Unknown;
+    NetworkState networkState = m_networkState;
+    resetMediaElement(LoadMediaResource);

[philipj_slow, 2016/01/21 12:48:10]

This will now be the only place where resetMediaElement is called with an
argument other than -1. Isn't it possible to just always clear all flags here?
Any async continuation of this method should be scheduled after this point.

If you make that change, also make sure to cancel any pending callback.

[Srirama, 2016/01/28 11:31:03]

passing -1 is resetting all the flags and because of that text track related
test cases are failing.

 
     // 1 - Abort any already-running instance of the resource selection algorithm for this element.
     m_loadState = WaitingForSource;
@@ -740,13 +729,11 @@ void HTMLMediaElement::prepareForLoad()
 
     // 3 - If the media element's networkState is set to NETWORK_LOADING or NETWORK_IDLE, queue
     // a task to fire a simple event named abort at the media element.
-    if (m_networkState == NETWORK_LOADING || m_networkState == NETWORK_IDLE)
+    if (networkState == NETWORK_LOADING || networkState == NETWORK_IDLE)
         scheduleEvent(EventTypeNames::abort);
 
-    resetMediaPlayerAndMediaSource();
-
     // 4 - If the media element's networkState is not set to NETWORK_EMPTY, then run these substeps
-    if (m_networkState != NETWORK_EMPTY) {
+    if (networkState != NETWORK_EMPTY) {
         // 4.1 - Queue a task to fire a simple event named emptied at the media element.
         scheduleEvent(EventTypeNames::emptied);
 
@@ -789,15 +776,18 @@ void HTMLMediaElement::prepareForLoad()
     m_autoplaying = true;
 
     // 7 - Invoke the media element's resource selection algorithm.
+    invokeResourceSelectionAlgorithm();
 
     // 8 - Note: Playback of any previously playing media resource for this element stops.
+}
 
+void HTMLMediaElement::invokeResourceSelectionAlgorithm()
+{
+    WTF_LOG(Media, "HTMLMediaElement::invokeResourceSelectionAlgorithm(%p)", this);
     // The resource selection algorithm
     // 1 - Set the networkState to NETWORK_NO_SOURCE
     setNetworkState(NETWORK_NO_SOURCE);
 
-    // 2 - Asynchronously await a stable state.

[philipj_slow, 2016/01/21 12:48:11]

Why was this comment removed? The spec presumably said this when this code was
implemented, and it's still not quite per spec with regard to awaiting a stable
state. If you remove this, instead sync with the new spec steps and add TODOs
for the bits that aren't correct.

Probably at the end of this method is where you should schedule an async
continuation corresponding to "await a stable state". (That should use a
microtask per spec, but let's try to not introduce any observable changes with
this cleanup.)

[Srirama, 2016/01/28 11:31:03]

Done.

-
     m_playedTimeRanges = TimeRanges::create();
 
     // FIXME: Investigate whether these can be moved into m_networkState != NETWORK_EMPTY block above
@@ -1981,8 +1971,12 @@ void HTMLMediaElement::playInternal()
         webMediaPlayer()->setBufferingStrategy(WebMediaPlayer::BufferingStrategy::Normal);
 
     // 4.8.10.9. Playing the media resource
-    if (m_networkState == NETWORK_EMPTY)
-        scheduleDelayedAction(LoadMediaResource);
+    if (m_networkState == NETWORK_EMPTY) {

[philipj_slow, 2016/01/21 12:48:11]

Can this simply be invokeResourceSelectionAlgorithm() as in the spec? If that
change has any observable side-effects, a TODO explaining the difference and how
to remove it would be good.

[Srirama, 2016/01/28 11:31:03]

Done. Do you want me to remove scheduleNextSourceChild() here? Removing
scheduleNextSourceChild() is causing test failure. So keeping it for now.

+        if (!(m_pendingActionFlags & LoadMediaResource))
+            invokeResourceSelectionAlgorithm();
+
+        scheduleNextSourceChild();
+    }
 
     // Generally "ended" and "looping" are exclusive. Here, the loop attribute
     // is ignored to seek back to start in case loop was set after playback
@@ -2041,8 +2035,12 @@ void HTMLMediaElement::pauseInternal()
 {
     WTF_LOG(Media, "HTMLMediaElement::pauseInternal(%p)", this);
 
-    if (m_networkState == NETWORK_EMPTY)
-        scheduleDelayedAction(LoadMediaResource);
+    if (m_networkState == NETWORK_EMPTY) {
+        if (!(m_pendingActionFlags & LoadMediaResource))

[philipj_slow, 2016/01/21 12:48:11]

Same as for playInternal().

[Srirama, 2016/01/28 11:31:03]

Done.

+            invokeResourceSelectionAlgorithm();
+
+        scheduleNextSourceChild();
+    }
 
     m_autoplayHelper.pauseMethodCalled();
 
@@ -2355,7 +2353,7 @@ void HTMLMediaElement::addTextTrack(WebInbandTextTrack* webTrack)
     // 7. Set the new text track's mode to the mode consistent with the user's preferences and the requirements of
     // the relevant specification for the data.
     //  - This will happen in honorUserPreferencesForAutomaticTextTrackSelection()
-    scheduleDelayedAction(LoadTextTrackResource);
+    scheduleTextTrackResourceLoad();
 
     // 8. Add the new text track to the media element's list of text tracks.
     // 9. Fire an event with the name addtrack, that does not bubble and is not cancelable, and that uses the TrackEvent
@@ -2475,7 +2473,7 @@ void HTMLMediaElement::didAddTrackElement(HTMLTrackElement* trackElement)
     // Do not schedule the track loading until parsing finishes so we don't start before all tracks
     // in the markup have been added.
     if (isFinishedParsingChildren())
-        scheduleDelayedAction(LoadTextTrackResource);
+        scheduleTextTrackResourceLoad();
 }
 
 void HTMLMediaElement::didRemoveTrackElement(HTMLTrackElement* trackElement)
@@ -2652,7 +2650,10 @@ void HTMLMediaElement::sourceWasAdded(HTMLSourceElement* source)
     // attribute and whose networkState has the value NETWORK_EMPTY, the user agent must invoke
     // the media element's resource selection algorithm.
     if (networkState() == HTMLMediaElement::NETWORK_EMPTY) {
-        scheduleDelayedAction(LoadMediaResource);
+        if (!(m_pendingActionFlags & LoadMediaResource))

[philipj_slow, 2016/01/21 12:48:11]

Here too, can we just invoke the resource selection algorithm?

[Srirama, 2016/01/28 11:31:03]

Done.

+            invokeResourceSelectionAlgorithm();
+
+        scheduleNextSourceChild();
         m_nextChildNodeToConsider = source;
         return;
     }
@@ -3006,7 +3007,7 @@ void HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLockin
     }
 }
 
-void HTMLMediaElement::clearMediaPlayer(int flags)
+void HTMLMediaElement::resetMediaElement(int flags)
 {
     forgetResourceSpecificTracks();
 
@@ -3019,6 +3020,11 @@ void HTMLMediaElement::clearMediaPlayer(int flags)
         clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
     }
 
+#if ENABLE(WEB_AUDIO)
+    if (m_audioSourceNode)
+        audioSourceProvider().setClient(m_audioSourceNode);
+#endif
+
     stopPeriodicTimers();
     m_loadTimer.stop();
 
@@ -3028,6 +3034,26 @@ void HTMLMediaElement::clearMediaPlayer(int flags)
     // We can't cast if we don't have a media player.
     m_remoteRoutesAvailable = false;
     m_playingRemotely = false;
+
+    // FIXME: Figure out appropriate place to reset LoadTextTrackResource if necessary and set m_pendingActionFlags to 0 here.
+    m_sentEndEvent = false;
+    m_sentStalledEvent = false;
+    m_haveFiredLoadedData = false;
+    m_completelyLoaded = false;
+    m_havePreparedToPlay = false;
+    m_displayMode = Unknown;
+
+    m_readyState = HAVE_NOTHING;
+    m_readyStateMaximum = HAVE_NOTHING;
+    setNetworkState(NETWORK_EMPTY);
+    setShouldDelayLoadEvent(false);
+    m_currentSourceNode = nullptr;
+    invalidateCachedTime();
+    cueTimeline().updateActiveCues(0);
+    m_playing = false;
+    m_paused = true;
+    m_seeking = false;
+
     if (mediaControls())
         mediaControls()->refreshCastButtonVisibilityWithoutUpdate();
 
@@ -3045,24 +3071,12 @@ void HTMLMediaElement::stop()
     cancelPendingEventsAndCallbacks();
     m_asyncEventQueue->close();
 
-    // Stop the playback without generating events
-    clearMediaPlayer(-1);
-    m_readyState = HAVE_NOTHING;
-    m_readyStateMaximum = HAVE_NOTHING;
-    setNetworkState(NETWORK_EMPTY);
-    setShouldDelayLoadEvent(false);
-    m_currentSourceNode = nullptr;
-    invalidateCachedTime();
-    cueTimeline().updateActiveCues(0);
-    m_playing = false;
-    m_paused = true;
-    m_seeking = false;
+    // Clear everything in the Media Element
+    resetMediaElement(-1);
 
     if (layoutObject())
         layoutObject()->updateFromElement();
 
-    stopPeriodicTimers();
-
     // Ensure that hasPendingActivity() is not preventing garbage collection, since otherwise this
     // media element will simply leak.
     ASSERT(!hasPendingActivity());
@@ -3411,7 +3425,7 @@ void* HTMLMediaElement::preDispatchEventHandler(Event* event)
     return nullptr;
 }
 
-// TODO(srirama.m): Refactor this and clearMediaPlayer to the extent possible.
+// TODO(srirama.m): Merge it to resetMediaElement if possible and remove it.
 void HTMLMediaElement::resetMediaPlayerAndMediaSource()
 {
     closeMediaSource();