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 fb8bee62880a27bfc1ae427b6a31898015dedbbc..9f3d55bd7be640a83a1d44f38914aea6cdf94fbd 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -332,7 +332,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_sentStalledEvent(false)
     , m_sentEndEvent(false)
     , m_closedCaptionsVisible(false)
-    , m_havePreparedToPlay(false)
+    , m_ignorePreloadNone(false)
     , m_tracksAreReady(true)
     , m_processingPreferenceChange(false)
     , m_remoteRoutesAvailable(false)
@@ -471,8 +471,7 @@ 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);
+    invokeLoadAlgorithm();
 
     // 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 +500,7 @@ 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);
+            invokeLoadAlgorithm();
         }
     } else if (name == controlsAttr) {
         UseCounter::count(document(), UseCounter::HTMLMediaElementControlsAttribute);
@@ -521,7 +519,7 @@ void HTMLMediaElement::finishParsingChildren()
     HTMLElement::finishParsingChildren();
 
     if (Traversal<HTMLTrackElement>::firstChild(*this))
-        scheduleDelayedAction(LoadTextTrackResource);
+        scheduleTextTrackResourceLoad();
 }
 
 bool HTMLMediaElement::layoutObjectIsNeeded(const ComputedStyle& style)
@@ -542,7 +540,7 @@ Node::InsertionNotificationRequest HTMLMediaElement::insertedInto(ContainerNode*
     if (insertionPoint->inDocument()) {
         UseCounter::count(document(), UseCounter::HTMLMediaElementInDocument);
         if (!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY)
-            scheduleDelayedAction(LoadMediaResource);
+            invokeLoadAlgorithm();
     }
 
     return InsertionShouldCallDidNotifySubtreeInsertions;
@@ -579,17 +577,11 @@ void HTMLMediaElement::didRecalcStyle(StyleRecalcChange)
         layoutObject()->updateFromElement();
 }
 
-void HTMLMediaElement::scheduleDelayedAction(DelayedActionType actionType)
+void HTMLMediaElement::scheduleTextTrackResourceLoad()
 {
-    WTF_LOG(Media, "HTMLMediaElement::scheduleDelayedAction(%p)", this);
+    WTF_LOG(Media, "HTMLMediaElement::scheduleTextTrackResourceLoad(%p)", this);
 
-    if ((actionType & LoadMediaResource) && !(m_pendingActionFlags & LoadMediaResource)) {
-        prepareForLoad();
-        m_pendingActionFlags |= LoadMediaResource;
-    }
-
-    if (actionType & LoadTextTrackResource)
-        m_pendingActionFlags |= LoadTextTrackResource;
+    m_pendingActionFlags |= LoadTextTrackResource;
 
     if (!m_loadTimer.isActive())
         m_loadTimer.startOneShot(0, BLINK_FROM_HERE);
@@ -710,9 +702,17 @@ void HTMLMediaElement::load()
         m_autoplayMediaCounted = true;
     }
 
+    m_ignorePreloadNone = true;
     prepareForLoad();

[philipj_slow, 2016/03/09 09:15:59]

There ought to be a call to invokeLoadAlgorithm() here and not prepareForLoad()
+ loadInternal().

[philipj_slow, 2016/03/09 09:27:23]

But I suppose you will need if (loadIsDeferred()) startDeferredLoad() here.

[philipj_slow, 2016/03/09 09:30:22]

Sorry, I'm writing comments out of order. This could presumably become simply
invokeLoadAlgorithm() followed by a call to the function that sets
m_forceEffectivePreloadAuto and calls setPlayerPreload(). Since
invokeLoadAlgorithm() will already have destroyed the player it would
effectively be the same as just setting the flag.

[Srirama, 2016/03/09 12:26:22]

Replaced prepareForLoad + loadInternal with invokeLoadAlgorithm(). But do we
need to call one more function here? InvokeLoadAlgorithm internally schedules
loadInternal and we need to set the flag m_forceEffectivePreloadAuto before
reaching loadInternal. So i am setting the flag before calling
invokeLoadAlgorithm().

     loadInternal();
-    prepareToPlay();
+}
+
+void HTMLMediaElement::invokeLoadAlgorithm()
+{
+    WTF_LOG(Media, "HTMLMediaElement::invokeLoadAlgorithm(%p)", this);
+
+    prepareForLoad();
+    scheduleNextSourceChild();
 }
 
 void HTMLMediaElement::prepareForLoad()
@@ -728,7 +728,6 @@ void HTMLMediaElement::prepareForLoad()
     m_sentEndEvent = false;
     m_sentStalledEvent = false;
     m_haveFiredLoadedData = false;
-    m_havePreparedToPlay = false;
     m_displayMode = Unknown;
 
     // 1 - Abort any already-running instance of the resource selection algorithm for this element.
@@ -792,14 +791,20 @@ 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.
+    // 2 - Set the element's show poster flag to true
+    // TODO(srirama.m): Introduce show poster flag and update it as per spec
 
     m_playedTimeRanges = TimeRanges::create();
 
@@ -808,12 +813,13 @@ void HTMLMediaElement::prepareForLoad()
     m_lastSeekTime = 0;
     m_duration = std::numeric_limits<double>::quiet_NaN();
 
-    // The spec doesn't say to block the load event until we actually run the asynchronous section
-    // algorithm, but do it now because we won't start that until after the timer fires and the
-    // event may have already fired by then.
+    // 3 - Set the media element's delaying-the-load-event flag to true (this delays the load event)
     setShouldDelayLoadEvent(true);
     if (mediaControls())
         mediaControls()->reset();
+
+    // 4 - Await a stable state, allowing the task that invoked this algorithm to continue
+    // TODO(srirama.m): Schedule an async continuation of this algorithm as per the spec
 }
 
 void HTMLMediaElement::loadInternal()
@@ -975,7 +981,7 @@ void HTMLMediaElement::loadResource(const KURL& url, ContentType& contentType)
     if (attemptLoad && canLoadURL(url, contentType)) {
         ASSERT(!webMediaPlayer());
 
-        if (!m_havePreparedToPlay && effectivePreloadType() == WebMediaPlayer::PreloadNone) {
+        if (effectivePreloadType() == WebMediaPlayer::PreloadNone) {
             WTF_LOG(Media, "HTMLMediaElement::loadResource(%p) : Delaying load because preload == 'none'", this);
             deferLoad();
         } else {
@@ -1606,17 +1612,6 @@ bool HTMLMediaElement::supportsSave() const
     return webMediaPlayer() && webMediaPlayer()->supportsSave();
 }
 
-void HTMLMediaElement::prepareToPlay()
-{
-    WTF_LOG(Media, "HTMLMediaElement::prepareToPlay(%p)", this);
-    if (m_havePreparedToPlay)
-        return;
-    m_havePreparedToPlay = true;
-
-    if (loadIsDeferred())
-        startDeferredLoad();

[philipj_slow, 2016/03/09 09:27:23]

OK, so the removal is this code is the "deferred load machinery". The changes
you've made look like they could work. However, it looks like you could actually
reintroduce something very much like prepareToPlay that both sets
m_forceEffectivePreloadAuto and then calls setPlayerPreload() which will also
start the load if necessary.

[Srirama, 2016/03/09 12:26:22]

Added prepareToPlay again and modified as suggested. Do you want me to change
the name?

-}
-
 void HTMLMediaElement::seek(double time)
 {
     WTF_LOG(Media, "HTMLMediaElement::seek(%p, %f)", this, time);
@@ -1626,8 +1621,8 @@ void HTMLMediaElement::seek(double time)
         return;
 
     // If the media engine has been told to postpone loading data, let it go ahead now.
-    if (preloadType() < WebMediaPlayer::PreloadAuto && m_readyState < HAVE_FUTURE_DATA)
-        prepareToPlay();
+    if (preloadType() < WebMediaPlayer::PreloadAuto && m_readyState < HAVE_FUTURE_DATA && loadIsDeferred())
+        startDeferredLoad();
 
     // Get the current time before setting m_seeking, m_lastSeekTime is returned once it is set.
     refreshCachedTime();
@@ -1943,7 +1938,14 @@ WebMediaPlayer::Preload HTMLMediaElement::preloadType() const
 
 WebMediaPlayer::Preload HTMLMediaElement::effectivePreloadType() const
 {
-    return autoplay() ? WebMediaPlayer::PreloadAuto : preloadType();
+    if (autoplay())
+        return WebMediaPlayer::PreloadAuto;
+
+    WebMediaPlayer::Preload preload = preloadType();
+    if (m_ignorePreloadNone && preload == WebMediaPlayer::PreloadNone)
+        return WebMediaPlayer::PreloadAuto;
+
+    return preload;
 }
 
 ScriptPromise HTMLMediaElement::playForBindings(ScriptState* scriptState)
@@ -2014,8 +2016,10 @@ 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) {
+        invokeResourceSelectionAlgorithm();
+        scheduleNextSourceChild();

[philipj_slow, 2016/03/09 09:16:00]

Remove scheduleNextSourceChild(), if it's breaks then there are more things in
need of fixing :/

[Srirama, 2016/03/09 12:26:22]

Right now invokeResourceSelectionAlgorithm doesn't take care of 4th step. 4th
step is taken care in scheduleNextSourceChild(). So keeping it for now with a
TODO mentioning the bug you raised.

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

[philipj_slow, 2016/03/09 09:16:00]

Ditto

[Srirama, 2016/03/09 12:26:22]

See my comment above.

+    }
 
     m_autoplayHelper.pauseMethodCalled();
 
@@ -2414,7 +2420,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
@@ -2534,7 +2540,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)
@@ -2707,7 +2713,8 @@ 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);
+        invokeResourceSelectionAlgorithm();
+        scheduleNextSourceChild();

[philipj_slow, 2016/03/09 09:16:00]

Ditto. It would also be *nice* if m_nextChildNodeToConsider were maintained
outside of this if clause, and I'm not sure it's correct. However, that all
looks pretty involved and it's probably best to not refactor it here. If you
understand why the code is like this a comment would be nice, otherwise just
leave it as is.

[Srirama, 2016/03/09 12:26:22]

I think if we move m_nextChildNodeToConsider above there will be behaviour
change as there is a chance of never reaching the steps 2731 onwards. I think
the idea is to consider new |source| only if the current
|m_nextChildNodeToConsider| processing has not yet started.

         m_nextChildNodeToConsider = source;
         return;
     }
@@ -3037,8 +3044,8 @@ void HTMLMediaElement::updatePlayState()
         if (time > m_lastSeekTime)
             addPlayedRange(m_lastSeekTime, time);
 
-        if (couldPlayIfEnoughData())
-            prepareToPlay();
+        if (couldPlayIfEnoughData() && loadIsDeferred())
+            startDeferredLoad();
 
         if (mediaControls())
             mediaControls()->playbackStopped();
@@ -3102,7 +3109,7 @@ void HTMLMediaElement::stop()
     cancelPendingEventsAndCallbacks();
     m_asyncEventQueue->close();
 
-    // Stop the playback without generating events
+    // Clear everything in the Media Element
     clearMediaPlayer(-1);
     m_readyState = HAVE_NOTHING;
     m_readyStateMaximum = HAVE_NOTHING;
@@ -3474,7 +3481,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();