Change play promises behaviour in the load algorithm.

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

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 743 matching lines @@
     // FIXME: Figure out appropriate place to reset LoadTextTrackResource if necessary and set m_pendingActionFlags to 0 here.
     m_pendingActionFlags &= ~LoadMediaResource;
     m_sentStalledEvent = false;
     m_haveFiredLoadedData = false;
     m_displayMode = Unknown;
 
     // 1 - Abort any already-running instance of the resource selection algorithm for this element.
     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) {
         // TODO(foolip): There is a proposal to always reset the paused state
         // in the media element load algorithm, to avoid a bogus play() promise
         // rejection: https://github.com/whatwg/html/issues/869
         // This is where that change would have an effect, and it is measured to
         // verify the assumption that it's a very rare situation.
         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()
 {
     BLINK_MEDIA_LOG << "invokeResourceSelectionAlgorithm(" << (void*)this << ")";
     // The resource selection algorithm
     // 1 - Set the networkState to NETWORK_NO_SOURCE
     setNetworkState(kNetworkNoSource);
 
     // 2 - Set the element's show poster flag to true
@@ skipping 552 matching lines @@
     m_currentSourceNode = nullptr;
 }
 
 void HTMLMediaElement::cancelPendingEventsAndCallbacks()
 {
     BLINK_MEDIA_LOG << "cancelPendingEventsAndCallbacks(" << (void*)this << ")";
     m_asyncEventQueue->cancelAllEvents();
 
     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()
 {
     setNetworkState(webMediaPlayer()->getNetworkState());
 }
 
 void HTMLMediaElement::mediaLoadingFailed(WebMediaPlayer::NetworkState error)
 {
     stopPeriodicTimers();
@@ skipping 2602 matching lines @@
 
 IntRect HTMLMediaElement::AutoplayHelperClientImpl::absoluteBoundingBoxRect() const
 {
     IntRect result;
     if (LayoutObject* object = m_element->layoutObject())
         result = object->absoluteBoundingBoxRect();
     return result;
 }
 
 } // namespace blink