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 7a3c5bb811ac9c53744959f8ea10062c0685cee2..58d397691dcbee2c6dd9de9bdb5b52cfc74dc749 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -460,7 +460,7 @@ void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument)
     // the MediaPlayer's LocalFrame and FrameLoader references on
     // document changes so that playback can be resumed properly.
     clearMediaPlayer(LoadMediaResource);

[philipj_slow, 2016/02/09 07:34:34]

As I wrote in some earlier CL, I think this belongs inside
invokeLoadAlgorithm(), so that it's always just a single call where the spec
says "invoke the media element load algorithm", although this isn't one of those
places.

[Srirama, 2016/02/09 14:50:46]

Done.

-    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.
@@ -490,7 +490,7 @@ void HTMLMediaElement::parseAttribute(const QualifiedName& name, const AtomicStr
         // Trigger a reload, as long as the 'src' attribute is present.
         if (!value.isNull()) {
             clearMediaPlayer(LoadMediaResource);

[philipj_slow, 2016/02/09 07:34:34]

Here too.

[Srirama, 2016/02/09 14:50:46]

Done.

-            scheduleDelayedAction(LoadMediaResource);
+            invokeLoadAlgorithm();
         }
     } else if (name == controlsAttr) {
         configureMediaControls();
@@ -508,7 +508,7 @@ void HTMLMediaElement::finishParsingChildren()
     HTMLElement::finishParsingChildren();
 
     if (Traversal<HTMLTrackElement>::firstChild(*this))
-        scheduleDelayedAction(LoadTextTrackResource);
+        scheduleTextTrackResourceLoad();
 }
 
 bool HTMLMediaElement::layoutObjectIsNeeded(const ComputedStyle& style)
@@ -528,7 +528,7 @@ Node::InsertionNotificationRequest HTMLMediaElement::insertedInto(ContainerNode*
     HTMLElement::insertedInto(insertionPoint);
     if (insertionPoint->inDocument()) {
         if (!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY)
-            scheduleDelayedAction(LoadMediaResource);
+            invokeLoadAlgorithm();
     }
 
     return InsertionShouldCallDidNotifySubtreeInsertions;
@@ -565,17 +565,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);
@@ -701,6 +695,14 @@ void HTMLMediaElement::load()
     prepareToPlay();
 }
 
+void HTMLMediaElement::invokeLoadAlgorithm()
+{
+    WTF_LOG(Media, "HTMLMediaElement::invokeLoadAlgorithm(%p)", this);
+
+    prepareForLoad();
+    scheduleNextSourceChild();

[philipj_slow, 2016/02/09 07:34:34]

scheduleNextSourceChild() belongs somewhere in the resource selection algorithm.
If that move works, you can simply rename prepareForLoad() to
invokeLoadAlgorithm() which would make for one less internal concept to
understand.

[Srirama, 2016/02/09 14:50:46]

As you said in the below comment, we can move scheduleNextSourceChild() inside
invokeResourceSelectionAlgorithm() but the load() function is where it may
create problem. There we directly invoke loadInternal() and then prepareToPlay()
without using scheduleNextSourceChild(). So what should we do about
prepareToPlay()? Should we remove that and just invoke resource load algorithm
as the spec says?

[philipj_slow, 2016/02/10 05:02:30]

OK, this is interesting. Per spec load() absolutely should invoke the media
element load algorithm, in fact it is the only thing it should do.

In addition, there's an extra prepareToPlay() call, and that is needed so that
preload="none" is overridden. The spec bug is
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten around
to updating the spec for that yet.

It looks like prepareToPlay() has to be called after loadInternal(), because
before that there may not be a media resource to act on in
HTMLMediaElement::executeDeferredLoad.

The cleanest approach would be to make sure that "Delaying load because preload
== 'none'" path in HTMLMediaElement::loadResource is not taken if load() was
called, and currently that depends on m_havePreparedToPlay. I poked at it in
https://codereview.chromium.org/1687793002 and it would be nice if only
effectivePreloadType() were used there.

So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay to
something that has the semantics of "we will now ignore preload=none, clamping
the effective preload state to metadata or higher depending on other state". Not
sure what to call it, maybe you can see if it would actually work first?

[Srirama, 2016/02/10 11:16:19]

ok, i have tried the changes locally and all the layout tests are passing. But
one problem here is the flag m_havePreparedToPlay is currently being reset in
prepareForLoad(), so right now if i set the flag  inbetween prepareforload and
loadinternal it is working fine. But once we remove all this infavor of
invokeLoadAlgorithm, how can we handle this flag? Should i reset the flag at the
call sites ?

[philipj_slow, 2016/02/16 12:19:34]

Let's see, m_havePreparedToPlay is used in two ways:

1. It's combined with effectivePreloadType() to possibly defer the load in
HTMLMediaElement::loadResource, which is very much related to
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921

2. It can cause an early return in HTMLMediaElement::prepareToPlay(), and
removing that could cause startDeferredLoad() to be called twice if
loadIsDeferred(), but judging by the various methods involved in load deferral,
it looks like perhaps this isn't observable at all.

So, I would propose removing prepareToPlay() and m_havePreparedToPlay and
replacing them with two things that explicitly do what seems to be the important
bits:

1. A flag which causes effectivePreloadType() to return PreloadAuto if it is
set, and it is set by playing and seeking.

2. Whatever changes, if any, are required to ensure that a deferred load is
started precisely when it should be, at least when seeking and playing. May
require some changes to the whole deferred load machinery.

+}
+
 void HTMLMediaElement::prepareForLoad()
 {
     WTF_LOG(Media, "HTMLMediaElement::prepareForLoad(%p)", this);
@@ -777,14 +779,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();
 
@@ -793,12 +801,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

[philipj_slow, 2016/02/09 07:34:34]

How does this currently work? I think it's the earlier scheduleNextSourceChild()
that should be moved here.

[Srirama, 2016/02/09 14:50:46]

We can move it expect for the problem mentioned in my above comment.

 }
 
 void HTMLMediaElement::loadInternal()
@@ -1965,8 +1974,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/02/09 07:34:34]

Try to get rid of scheduleNextSourceChild() here and below.

Traversing source children is what the resource selection algorithm does, having
scheduleNextSourceChild() *after* the invocation here is a sign of some
mismatch.

[Srirama, 2016/02/09 14:50:46]

Probably this is also linked to the above issue.

+    }
 
     // Generally "ended" and "looping" are exclusive. Here, the loop attribute
     // is ignored to seek back to start in case loop was set after playback
@@ -2025,8 +2036,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();
+    }
 
     m_autoplayHelper.pauseMethodCalled();
 
@@ -2339,7 +2352,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
@@ -2459,7 +2472,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)
@@ -2636,7 +2649,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();
         m_nextChildNodeToConsider = source;
         return;
     }
@@ -3025,7 +3039,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;
@@ -3391,7 +3405,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();