Have HTMLMediaElement::play() return a Promise.

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 11 matching lines @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "core/html/HTMLMediaElement.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #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"
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/Fullscreen.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/events/Event.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Settings.h"
 #include "core/frame/UseCounter.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #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"
 #include "core/html/track/AudioTrack.h"
 #include "core/html/track/AudioTrackList.h"
 #include "core/html/track/AutomaticTrackSelection.h"
 #include "core/html/track/CueTimeline.h"
 #include "core/html/track/InbandTextTrack.h"
 #include "core/html/track/TextTrackContainer.h"
 #include "core/html/track/TextTrackList.h"
 #include "core/html/track/VideoTrack.h"
 #include "core/html/track/VideoTrackList.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/layout/LayoutVideo.h"
 #include "core/layout/LayoutView.h"
 #include "core/layout/compositing/PaintLayerCompositor.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/page/ChromeClient.h"
 #include "core/page/NetworkStateNotifier.h"
 #include "platform/ContentType.h"
 #include "platform/Logging.h"
 #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"
 #include "platform/graphics/GraphicsLayer.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebAudioSourceProvider.h"
 #include "public/platform/WebContentDecryptionModule.h"
 #include "public/platform/WebInbandTextTrack.h"
 #include "wtf/CurrentTime.h"
@@ skipping 636 matching lines @@
     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.
     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)
         scheduleEvent(EventTypeNames::abort);
 
     resetMediaPlayerAndMediaSource();
 
     // 4 - If the media element's networkState is not set to NETWORK_EMPTY, then run these substeps
     if (m_networkState != NETWORK_EMPTY) {
         // 4.1 - Queue a task to fire a simple event named emptied at the media element.
@@ skipping 508 matching lines @@
     // 6.1 - Set the error attribute to a new MediaError object whose code attribute is set to
     // MEDIA_ERR_SRC_NOT_SUPPORTED.
     m_error = MediaError::create(MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED);
 
     // 6.2 - Forget the media element's media-resource-specific text tracks.
     forgetResourceSpecificTracks();
 
     // 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,
     // the element won't attempt to load another resource.
 
     updateDisplayState();
 
     if (layoutObject())
         layoutObject()->updateFromElement();
 }
@@ skipping 237 matching lines @@
         m_haveFiredLoadedData = true;
         shouldUpdateDisplayState = true;
         scheduleEvent(EventTypeNames::loadeddata);
         setShouldDelayLoadEvent(false);
     }
 
     bool isPotentiallyPlaying = potentiallyPlaying();
     if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) {
         scheduleEvent(EventTypeNames::canplay);
         if (isPotentiallyPlaying)
-            scheduleEvent(EventTypeNames::playing);
+            scheduleNotifyPlaying();
         shouldUpdateDisplayState = true;
     }
 
     if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA && tracksAreReady) {
         if (oldState <= HAVE_CURRENT_DATA) {
             scheduleEvent(EventTypeNames::canplay);
             if (isPotentiallyPlaying)
-                scheduleEvent(EventTypeNames::playing);
+                scheduleNotifyPlaying();
         }
 
         // Check for autoplay, and record metrics about it if needed.
         if (shouldAutoplay(RecordMetricsBehavior::DoRecord)) {
             // If the autoplay experiment says that it's okay to play now,
             // then don't require a user gesture.
             m_autoplayHelper.becameReadyToPlay();
 
             if (!m_userGestureRequiredForPlay) {
                 m_paused = false;
                 invalidateCachedTime();
                 scheduleEvent(EventTypeNames::play);
-                scheduleEvent(EventTypeNames::playing);
+                scheduleNotifyPlaying();
                 m_autoplaying = false;
             }
         }
 
         scheduleEvent(EventTypeNames::canplaythrough);
 
         shouldUpdateDisplayState = true;
     }
 
     if (shouldUpdateDisplayState) {
@@ skipping 371 matching lines @@
     // https://crbug.com/310450
     UseCounter::count(document(), UseCounter::HTMLMediaElementPreloadDefault);
     return WebMediaPlayer::PreloadAuto;
 }
 
 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);
 
     m_autoplayHelper.playMethodCalled();
 
     if (!UserGestureIndicator::processingUserGesture()) {
         autoplayMediaEncountered();
 
         if (m_userGestureRequiredForPlay) {
             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)
             recordAutoplayMetric(AutoplayManualStart);
         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().)
     if (webMediaPlayer())
         webMediaPlayer()->setBufferingStrategy(WebMediaPlayer::BufferingStrategy::Normal);
 
     // 4.8.10.9. Playing the media resource
     if (m_networkState == NETWORK_EMPTY)
         scheduleDelayedAction(LoadMediaResource);
 
     // Generally "ended" and "looping" are exclusive. Here, the loop attribute
     // is ignored to seek back to start in case loop was set after playback
     // ended. See http://crbug.com/364442
     if (endedPlayback(LoopCondition::Ignored))
         seek(0);
 
     if (m_paused) {
         m_paused = false;
         invalidateCachedTime();
         scheduleEvent(EventTypeNames::play);
 
         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();
 }
 
 void HTMLMediaElement::autoplayMediaEncountered()
 {
     if (!m_autoplayMediaCounted) {
         m_autoplayMediaCounted = true;
         recordAutoplayMetric(AutoplayMediaFound);
@@ skipping 34 matching lines @@
     m_autoplayHelper.pauseMethodCalled();
 
     m_autoplaying = false;
 
     if (!m_paused) {
         recordMetricsIfPausing();
 
         m_paused = true;
         scheduleTimeupdateEvent(false);
         scheduleEvent(EventTypeNames::pause);
+        scheduleRejectPlayPromises(MediaError::MEDIA_ERR_ABORTED);
     }
 
     updatePlayState();
 }
 
 void HTMLMediaElement::requestRemotePlayback()
 {
     ASSERT(m_remoteRoutesAvailable);
     webMediaPlayer()->requestRemotePlayback();
 }
@@ skipping 149 matching lines @@
     if (!periodicEvent || (haveNotRecentlyFiredTimeupdate && movieTimeHasProgressed)) {
         scheduleEvent(EventTypeNames::timeupdate);
         m_lastTimeUpdateEventWallTime = now;
         m_lastTimeUpdateEventMovieTime = movieTime;
     }
 }
 
 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();
 }
 
 AudioTrackList& HTMLMediaElement::audioTracks()
 {
     ASSERT(RuntimeEnabledFeatures::audioVideoTracksEnabled());
     return *m_audioTracks;
 }
 
@@ skipping 1259 matching lines @@
     visitor->trace(m_asyncEventQueue);
     visitor->trace(m_error);
     visitor->trace(m_currentSourceNode);
     visitor->trace(m_nextChildNodeToConsider);
     visitor->trace(m_mediaSource);
     visitor->trace(m_audioTracks);
     visitor->trace(m_videoTracks);
     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);
     HeapSupplementable<HTMLMediaElement>::trace(visitor);
 #endif
     HTMLElement::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 void HTMLMediaElement::createPlaceholderTracksIfNecessary()
@@ skipping 57 matching lines @@
 {
     m_autoplayHelper.updatePositionNotificationRegistration();
 }
 
 // TODO(liberato): remove once autoplay gesture override experiment concludes.
 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))
         audioSourceProvider().setClient(nullptr);
 }
 
 void HTMLMediaElement::AudioSourceProviderImpl::wrap(WebAudioSourceProvider* provider)
 {
     MutexLocker locker(provideInputLock);
 
@@ skipping 47 matching lines @@
 {
     visitor->trace(m_client);
 }
 
 DEFINE_TRACE(HTMLMediaElement::AudioSourceProviderImpl)
 {
     visitor->trace(m_client);
 }
 
 } // namespace blink