Have HTMLMediaElement::play() return a Promise.

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..fff9ef427dbd9c01f4d1e274b9c29cf19431d847 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -29,6 +29,7 @@
 #include "bindings/core/v8/ExceptionStatePlaceholder.h"
 #include "bindings/core/v8/ScriptController.h"
 #include "bindings/core/v8/ScriptEventListener.h"
+#include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/HTMLNames.h"
 #include "core/css/MediaList.h"
 #include "core/dom/Attribute.h"
@@ -44,7 +45,6 @@
 #include "core/html/HTMLMediaSource.h"
 #include "core/html/HTMLSourceElement.h"
 #include "core/html/HTMLTrackElement.h"
-#include "core/html/MediaError.h"
 #include "core/html/MediaFragmentURIParser.h"
 #include "core/html/TimeRanges.h"
 #include "core/html/shadow/MediaControls.h"
@@ -70,6 +70,7 @@
 #include "platform/MIMETypeFromURL.h"
 #include "platform/MIMETypeRegistry.h"
 #include "platform/RuntimeEnabledFeatures.h"
+#include "platform/Task.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/audio/AudioBus.h"
 #include "platform/audio/AudioSourceProviderClient.h"
@@ -726,6 +727,8 @@ void HTMLMediaElement::prepareForLoad()
     // one of the task queues, then remove those tasks.
     cancelPendingEventsAndCallbacks();
 
+    rejectPlayPromises(MediaError::MEDIA_ERR_ABORTED);
+
     // 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)
@@ -1254,12 +1257,15 @@ void HTMLMediaElement::noneSupported()
     // 6.3 - Set the element's networkState attribute to the NETWORK_NO_SOURCE value.
     setNetworkState(NETWORK_NO_SOURCE);
 
-    // 7 - Queue a task to fire a simple event named error at the media element.
+    // 5 - Queue a task to fire a simple event named error at the media element.
     scheduleEvent(EventTypeNames::error);
 
+    // 6 - For each promise in the list of pending play romises, reject it with NotSupportedError.

[philipj_slow, 2016/02/02 09:56:33]

Can you sweep your CL and update step numbers and comments to use what finally
ended up in the spec?

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

Done.

+    scheduleRejectPlayPromises(MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED);
+
     closeMediaSource();
 
-    // 8 - Set the element's delaying-the-load-event flag to false. This stops delaying the load event.
+    // 7 - Set the element's delaying-the-load-event flag to false. This stops delaying the load event.
     setShouldDelayLoadEvent(false);
 
     // 9 - Abort these steps. Until the load() method is invoked or the src attribute is changed,
@@ -1517,7 +1523,7 @@ void HTMLMediaElement::setReadyState(ReadyState state)
     if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) {
         scheduleEvent(EventTypeNames::canplay);
         if (isPotentiallyPlaying)
-            scheduleEvent(EventTypeNames::playing);
+            scheduleNotifyPlaying();
         shouldUpdateDisplayState = true;
     }
 
@@ -1525,7 +1531,7 @@ void HTMLMediaElement::setReadyState(ReadyState state)
         if (oldState <= HAVE_CURRENT_DATA) {
             scheduleEvent(EventTypeNames::canplay);
             if (isPotentiallyPlaying)
-                scheduleEvent(EventTypeNames::playing);
+                scheduleNotifyPlaying();
         }
 
         // Check for autoplay, and record metrics about it if needed.
@@ -1538,7 +1544,7 @@ void HTMLMediaElement::setReadyState(ReadyState state)
                 m_paused = false;
                 invalidateCachedTime();
                 scheduleEvent(EventTypeNames::play);
-                scheduleEvent(EventTypeNames::playing);
+                scheduleNotifyPlaying();
                 m_autoplaying = false;
             }
         }
@@ -1930,7 +1936,7 @@ WebMediaPlayer::Preload HTMLMediaElement::effectivePreloadType() const
     return autoplay() ? WebMediaPlayer::PreloadAuto : preloadType();
 }
 
-void HTMLMediaElement::play()
+ScriptPromise HTMLMediaElement::play(ScriptState* scriptState)
 {
     WTF_LOG(Media, "HTMLMediaElement::play(%p)", this);
 
@@ -1943,7 +1949,7 @@ void HTMLMediaElement::play()
             recordAutoplayMetric(PlayMethodFailed);
             String message = ExceptionMessages::failedToExecute("play", "HTMLMediaElement", "API can only be initiated by a user gesture.");
             document().addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, message));
-            return;
+            return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotAllowedError, "play() must be initiated by a user gesture."));
         }
     } else if (m_userGestureRequiredForPlay) {
         if (m_autoplayMediaCounted)
@@ -1951,13 +1957,24 @@ void HTMLMediaElement::play()
         m_userGestureRequiredForPlay = false;
     }
 
+    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+
+    m_playResolvers.append(resolver);
     playInternal();
+
+    return promise;
 }
 
 void HTMLMediaElement::playInternal()
 {
     WTF_LOG(Media, "HTMLMediaElement::playInternal(%p)", this);
 
+    if (m_error && m_error->code() == MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) {

[philipj_slow, 2016/02/02 09:56:33]

This should be together right after the user gesture check, and mustn't reject
other promises that are in the list. Can you be sure to write a test for the
distinction?

It'd be bad if internal code paths can play in cases like this where scripts
cannot, so I'm thinking that perhaps the script-exposed play() needs to forward
to a new play(flags and stuff) to replace playInternal, where there are flags to
bypass the user gesture requirement is necessary, and some way to return a
DOMException if the promise should be rejected immediately and not placed on the
list of pending play promises.

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

I don't think there is actually any difference in behaviour because when we get
a SRC_NOT_SUPPORTED error, we actually reject all promises. At that point, we
can't have more than one pending promise. Let me know if I misunderstood.
Updated things. Not exactly what you said but let me know.

[philipj_slow, 2016/02/04 10:54:20]

New structure solves it nicely. I don't think "we
can't have more than one pending promise" was true, one could have called play()
from a script after scheduleRejectPlayPromises() was called in nonSupported(),
but before the queued task had run. The order of rejections would have been
wrong, I think, as well as the exception message for the first of the promises.

[mlamouri (slow - plz ping), 2016/02/18 17:06:06]

Done. (for me to keep track)

+        rejectPlayPromises(MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED);
+        return;
+    }
+
     // Always return the buffering strategy to normal when not paused,
     // regardless of the cause. (In contrast with aggressive buffering which is
     // only enabled by pause(), not pauseInternal().)
@@ -1982,8 +1999,11 @@ void HTMLMediaElement::playInternal()
         if (m_readyState <= HAVE_CURRENT_DATA)
             scheduleEvent(EventTypeNames::waiting);
         else if (m_readyState >= HAVE_FUTURE_DATA)
-            scheduleEvent(EventTypeNames::playing);
+            scheduleNotifyPlaying();
+    } else if (m_readyState >= HAVE_FUTURE_DATA) {
+        scheduleResolvePlayPromises();
     }
+
     m_autoplaying = false;
 
     updatePlayState();
@@ -2038,6 +2058,7 @@ void HTMLMediaElement::pauseInternal()
         m_paused = true;
         scheduleTimeupdateEvent(false);
         scheduleEvent(EventTypeNames::pause);
+        scheduleRejectPlayPromises(MediaError::MEDIA_ERR_ABORTED);
     }
 
     updatePlayState();
@@ -2207,7 +2228,7 @@ void HTMLMediaElement::scheduleTimeupdateEvent(bool periodicEvent)
 void HTMLMediaElement::togglePlayState()
 {
     if (paused())
-        play();
+        playInternal();

[philipj_slow, 2016/02/02 09:56:33]

Hmm, so you don't have a ScriptState here, but if you look at the call sites for
togglePlayState(), I think you should actually keep calling play() here, because
if someone is sending synthetic events to MediaDocument or MediaControls, it
really shouldn't play. Will require some fiddling for sure :)

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

Done.

     else
         pause();
 }
@@ -3487,6 +3508,7 @@ DEFINE_TRACE(HTMLMediaElement)
     visitor->trace(m_cueTimeline);
     visitor->trace(m_textTracks);
     visitor->trace(m_textTracksWhenResourceSelectionBegan);
+    visitor->trace(m_playResolvers);
     visitor->trace(m_audioSourceProvider);
     visitor->template registerWeakMembers<HTMLMediaElement, &HTMLMediaElement::clearWeakMembers>(this);
     visitor->trace(m_autoplayHelper);
@@ -3564,6 +3586,58 @@ void HTMLMediaElement::triggerAutoplayViewportCheckForTesting()
     m_autoplayHelper.triggerAutoplayViewportCheckForTesting();
 }
 
+void HTMLMediaElement::scheduleResolvePlayPromises()
+{
+    Platform::current()->currentThread()->taskRunner()->postTask(
+        BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, this)));

[philipj_slow, 2016/02/02 09:56:33]

So this is much nicer than the Timer thing used elsewhere in HTMLMediaElement,
but how does this work with lifetime? IIRC, last I asked the task queue didn't
know about Oilpan and thus this kind of code would not be safe. Has this
changed, so that this task will actually keep the HTMLMediaElement alive until
it's run?

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

I do not know how oilpan would handle that. Worth pinging the oilpan team I
guess.

[philipj_slow, 2016/02/04 10:54:20]

Please do, this looks like a potential crash.

[mlamouri (slow - plz ping), 2016/02/18 17:06:06]

I asked haraken@. He gave me a hint and I'm adding hiroshige@ as a reviewer.

+}
+
+void HTMLMediaElement::scheduleRejectPlayPromises(MediaError::Code errorCode)
+{
+    Platform::current()->currentThread()->taskRunner()->postTask(
+        BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::rejectPlayPromises, this, errorCode)));
+}
+
+void HTMLMediaElement::scheduleNotifyPlaying()
+{
+    scheduleEvent(EventTypeNames::playing);

[philipj_slow, 2016/02/02 09:56:33]

https://html.spec.whatwg.org/#notify-about-playing is actually always queued as
a single task, and you could tell the difference. If the playing event handlers
does something that runs on the next microtask checkpoint, like resolve a
promise (I think) then that would happen before the pending play promises are
resolved, where the idea was that it should happen after.

There isn't really a grand design for what goes into separate tasks and not, so
if you think that something else would be better, we can look at changing the
spec.

There is a risk here, too. If some events go though GenericEventQueue and some
via tasks, then I'm not confident that per-spec event order will always be
preserved. It would be nice to move everything to postTask(), but AFAICT there's
no way to cancel tasks, and that's needed to fake the "media element task
source".

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

AFAICT, with the current infra we have in Blink, we really can't do better than
that. Is that incorrect? Should we look into fixing this in a follow-up?

[philipj_slow, 2016/02/04 10:54:20]

It is indeed hard to fix without revamping the media element event handling. Can
you write a failing test and file a bug to fix it? (Or I can do it in w-p-t, see
earlier comment.)

[mlamouri (slow - plz ping), 2016/02/18 17:06:06]

Done.

+    scheduleResolvePlayPromises();
+}
+
+void HTMLMediaElement::resolvePlayPromises()
+{
+    for (auto& resolver: m_playResolvers)
+        resolver->resolve();
+
+    m_playResolvers.clear();
+}
+
+void HTMLMediaElement::rejectPlayPromises(MediaError::Code errorCode)
+{
+    ExceptionCode code;
+    String message;
+
+    switch (errorCode) {
+    case MediaError::MEDIA_ERR_ABORTED:
+        code = AbortError;
+        message = "The play() command was interrupted by another command like load() or pause().";

[philipj_slow, 2016/02/02 09:56:33]

I think it'd be best to just pass code and message (the DOMException constructor
arguments) when scheduling the rejection, so that a unique message per rejection
cause can be written. These strings will also make more sense in context.

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

Done.

+        break;
+    case MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED:
+        code = NotSupportedError;
+        message = "Failed to load because no supported source was found.";

[philipj_slow, 2016/02/02 09:56:33]

It would be good to split this too, to distinguish actively failing at resource
selection vs. already having failed before.

[mlamouri (slow - plz ping), 2016/02/03 19:28:57]

Done.

+        break;
+    case MediaError::MEDIA_ERR_DECODE:
+    case MediaError::MEDIA_ERR_NETWORK:
+        ASSERT_NOT_REACHED();
+        break;
+    }
+
+    for (auto& resolver: m_playResolvers)
+        resolver->reject(DOMException::create(code, message));
+
+    m_playResolvers.clear();
+}
+
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
 {
     if (!Heap::isHeapObjectAlive(m_audioSourceNode))