Change play promises behaviour in the load algorithm.

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 7122c3e10a3a601b087fba9a6a58399ccc60f759..5ee2bf6d34c995e1480e9be89fe142bee34aeabc 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -761,51 +761,67 @@ void HTMLMediaElement::invokeLoadAlgorithm()
     m_loadState = WaitingForSource;
     m_currentSourceNode = nullptr;
 
-    // 2 - If there are any tasks from the media element's media element event task source in
-    // one of the task queues, then remove those tasks.
+    // 2 - Let pending tasks be a list of tasks from the media element's media
+    // element task source in one of the task queues.
+    //
+    // 3 - For each task in the pending tasks that would run resolve pending
+    // play promises or project pending play prmoises algorithms, immediately
+    // resolve or reject those promises in the order the corresponding tasks

[foolip, 2016/08/03 16:31:51]

Write a TODO about the order or change the spec if it's unlikely to ever happen?

[mlamouri (slow - plz ping), 2016/08/04 11:16:16]

I'm not sure what you mean. This is in order, we append the promises. I will add
a test to make this clear.

[foolip, 2016/08/04 13:47:37]

New tests LGTM. The tricky case I was thinking about is if there are both
pending resolves and rejections interleaved in the task queue. When "readyState
attribute has the value HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA" it should be
possible to synchronously enqueue tasks to resolve by calling play(), and
synchronously enqueue tasks to reject by calling pause(). So a
play();pause();play();pause() series of calls?

Ideally, we should of course change both play() and pause() in the spec to only
resolve play() promises once really playing (i.e. never sync) in which case the
problem would go away. So the priority of fixing such a TODO would be very low.

[mlamouri (slow - plz ping), 2016/08/04 14:25:37]

Added TODO.

+    // were queued.
+    if (m_playPromiseResolveTask->isPending()) {
+        m_playPromiseResolveTask->cancel();
+        resolveScheduledPlayPromises();
+    }
+    if (m_playPromiseRejectTask->isPending()) {
+        m_playPromiseRejectTask->cancel();
+        rejectScheduledPlayPromises();
+    }
+
+    // 4 - Remove each task in pending tasks from its task queue.
     cancelPendingEventsAndCallbacks();
 
-    rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request.");
-
-    // 3 - If the media element's networkState is set to NETWORK_LOADING or NETWORK_IDLE, queue
+    // 5 - 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 == kNetworkLoading || m_networkState == kNetworkIdle)
         scheduleEvent(EventTypeNames::abort);
 
     resetMediaPlayerAndMediaSource();
 
-    // 4 - If the media element's networkState is not set to NETWORK_EMPTY, then run these substeps
+    // 6 - If the media element's networkState is not set to NETWORK_EMPTY, then run these substeps
     if (m_networkState != kNetworkEmpty) {
         // 4.1 - Queue a task to fire a simple event named emptied at the media element.
         scheduleEvent(EventTypeNames::emptied);
 
-        // 4.2 - If a fetching process is in progress for the media element, the user agent should stop it.
+        // 4.2 - Take pending play promises and reject pending play promises with the result and an "AbortError" DOMException.
+        rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request.");

[foolip, 2016/08/03 16:31:51]

If this moves down in the spec, I guess the else branch can also DCHECK that
there are no pending play promises.

[mlamouri (slow - plz ping), 2016/08/04 11:16:16]

I guess you are referring to the spec that says if the paused attribute is
false? We don't have this branch. I will keep this comment pending until we
decide what to do on the spec side.

[foolip, 2016/08/04 13:47:37]

Yeah, I mean you could DCHECK something in an else branch below, if you think it
could catch problems.

[mlamouri (slow - plz ping), 2016/08/04 14:25:37]

Added a DCHECK.

+
+        // 4.3 - If a fetching process is in progress for the media element, the user agent should stop it.
         setNetworkState(kNetworkEmpty);
 
-        // 4.3 - Forget the media element's media-resource-specific tracks.
+        // 4.5 - 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.
+        // 4.6 - If readyState is not set to HAVE_NOTHING, then set it to that state.
         m_readyState = kHaveNothing;
         m_readyStateMaximum = kHaveNothing;
 
-        // 4.5 - If the paused attribute is false, then set it to true.
+        // 4.6 - If the paused attribute is false, then set it to true.
         m_paused = true;
 
-        // 4.6 - If seeking is true, set it to false.
+        // 4.7 - If seeking is true, set it to false.
         m_seeking = false;
 
-        // 4.7 - Set the current playback position to 0.
+        // 4.8 - 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.
 
-        // 4.8 - Set the initial playback position to 0.
-        // FIXME: Make this less subtle. The position only becomes 0 because the ready state is kHaveNothing.
+        // 4.9 - Set the initial playback position to 0.
+        // FIXME: Make this less subtle. The position only becomes 0 because the ready state is HAVE_NOTHING.
         invalidateCachedTime();
 
-        // 4.9 - Set the timeline offset to Not-a-Number (NaN).
-        // 4.10 - Update the duration attribute to Not-a-Number (NaN).
+        // 4.10 - Set the timeline offset to Not-a-Number (NaN).
+        // 4.11 - Update the duration attribute to Not-a-Number (NaN).
 
         cueTimeline().updateActiveCues(0);
     } else if (!m_paused) {
@@ -817,17 +833,17 @@ void HTMLMediaElement::invokeLoadAlgorithm()
         UseCounter::count(document(), UseCounter::HTMLMediaElementLoadNetworkEmptyNotPaused);
     }
 
-    // 5 - Set the playbackRate attribute to the value of the defaultPlaybackRate attribute.
+    // 7 - Set the playbackRate attribute to the value of the defaultPlaybackRate attribute.
     setPlaybackRate(defaultPlaybackRate());
 
-    // 6 - Set the error attribute to null and the autoplaying flag to true.
+    // 8 - Set the error attribute to null and the autoplaying flag to true.
     m_error = nullptr;
     m_autoplaying = true;
 
-    // 7 - Invoke the media element's resource selection algorithm.
+    // 9 - Invoke the media element's resource selection algorithm.
     invokeResourceSelectionAlgorithm();
 
-    // 8 - Note: Playback of any previously playing media resource for this element stops.
+    // 10 - Note: Playback of any previously playing media resource for this element stops.
 }
 
 void HTMLMediaElement::invokeResourceSelectionAlgorithm()
@@ -1400,11 +1416,6 @@ void HTMLMediaElement::cancelPendingEventsAndCallbacks()
 
     for (HTMLSourceElement* source = Traversal<HTMLSourceElement>::firstChild(*this); source; source = Traversal<HTMLSourceElement>::nextSibling(*source))
         source->cancelPendingErrorEvent();
-
-    m_playPromiseResolveTask->cancel();
-    m_playPromiseResolveList.clear();
-    m_playPromiseRejectTask->cancel();
-    m_playPromiseRejectList.clear();
 }
 
 void HTMLMediaElement::networkStateChanged()