Have HTMLMediaElement::play() return a Promise. (issue 1576283003 by mlamouri@chromium.org)

[mlam...@chromium.org]

Reviewers: philipj_UTC7
CL: https://codereview.chromium.org/1576283003/

Message:
Philip, PTAL.

Description:
Have HTMLMediaElement::play() return a Promise.

This is implementing the following proposal that was recently added to the spec:
https://github.com/whatwg/html/issues/505

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/bvs8ledF4tU/qC__g3yICAAJ

This is also adding a new exception type called NotAllowedError.

BUG=579541

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+636, -29 lines):
  M third_party/WebKit/LayoutTests/media/W3C/audio/events/event_pause_manual-expected.txt
  M third_party/WebKit/LayoutTests/media/W3C/audio/events/event_play_manual-expected.txt
  M third_party/WebKit/LayoutTests/media/W3C/audio/paused/paused_true_during_pause-expected.txt
  M third_party/WebKit/LayoutTests/media/W3C/video/events/event_pause_manual-expected.txt
  M third_party/WebKit/LayoutTests/media/W3C/video/events/event_play_manual-expected.txt
  M third_party/WebKit/LayoutTests/media/W3C/video/paused/paused_true_during_pause-expected.txt
  A third_party/WebKit/LayoutTests/media/media-play-promise.html
  A third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt
  M third_party/WebKit/LayoutTests/media/track/track-cues-pause-on-exit-expected.txt
  M third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt
  M third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button-expected.txt
  M third_party/WebKit/LayoutTests/media/video-display-none-crash-expected.txt
  M third_party/WebKit/LayoutTests/media/video-play-pause-events-expected.txt
  M third_party/WebKit/LayoutTests/media/video-play-pause-exception-expected.txt
  M third_party/WebKit/LayoutTests/media/video-play-require-user-gesture-expected.txt
  M third_party/WebKit/LayoutTests/media/video-played-collapse-expected.txt
  M third_party/WebKit/LayoutTests/media/video-played-ranges-1-expected.txt
  M third_party/WebKit/LayoutTests/media/video-preload-expected.txt
  M third_party/WebKit/Source/core/dom/DOMException.cpp
  M third_party/WebKit/Source/core/dom/ExceptionCode.h
  M third_party/WebKit/Source/core/html/HTMLMediaElement.h
  M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
  M third_party/WebKit/Source/core/html/HTMLMediaElement.idl
  M third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp
  M third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
  M third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
  M third_party/WebKit/Source/core/testing/Internals.cpp
  M third_party/WebKit/Source/web/WebViewImpl.cpp

[dalec...@chromium.org]

Haven't reviewed, but do we want to take this opportunity to only return the
promise once playback _actually_ starts? I.e. frame 0 of audio and frame 0 of
video have been played out.

https://codereview.chromium.org/1576283003/

[mlam...@chromium.org]

Comments applied. PTAL.

Note that I will be a bit slow to answer comments for the next 10 days.


https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/W3C/audio/events/event_pause_manual-expected.txt
File
third_party/WebKit/LayoutTests/media/W3C/audio/events/event_pause_manual-expected.txt
(right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/W3C/audio/events/event_pause_manual-expected.txt#newcode1
third_party/WebKit/LayoutTests/media/W3C/audio/events/event_pause_manual-expected.txt:1:
CONSOLE ERROR: Uncaught (in promise) [object DOMException]
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> Too bad this doesn't stringify to the exception message, then this
could actually have been used to see why it was rejected :/

Hmm. If you look at the previous patchsets, you will see that it used to
be different (see the file I forgot to update).

It seems that locally I now have a fully stringified error. I guess
something was broken yesterday.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/media-play-promise.html
File third_party/WebKit/LayoutTests/media/media-play-promise.html
(right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/media-play-promise.html#newcode7
third_party/WebKit/LayoutTests/media/media-play-promise.html:7: // This
is testing the behavior of play() with regards to the returned
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> This is a very big test with a mini runner harness inside :)
> 
> If tests share a lot of setup, that could be split out into a
media-play-promise.js or similar. That would also allow you to isolate
those tests which cannot readily be upstreamed to web-platform-tests.

Ack.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/media-play-promise.html#newcode42
third_party/WebKit/LayoutTests/media/media-play-promise.html:42:
function playWithUserGesture()
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> This could also be a problem for w-p-t. What I did for some fullscreen
tests that require a user gesture is to make the tests async, and if we
aren't running with window.testRunner, add a button for the user to
click. Also descent for manual testing. See
LayoutTests/fullscreen/trusted-event.js.

Where do media/ tests ready for w-p-t live? Are they in W3C/ or is that
for W3C imports only?

Note that I would prefer to do that in a follow-up in order to not slow
down landing the feature.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/media-play-promise.html#newcode174
third_party/WebKit/LayoutTests/media/media-play-promise.html:174:
internals.setMediaElementNetworkState(mediaElement, 6 /*
NetworkStateDecodeError */);
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> Not a fan of this. It's weird that WebMediaPlayer::NetworkState is
used to communicate errors (even network-related) at all, and I'd like
to not spread that around.
> 
> I guess that this bit will just go away though, since network and
decode errors shouldn't reject the promise any longer.

This is actually testing that the error doesn't reject the promise.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt
File
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt
(right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt#newcode2
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt:2:
CONSOLE ERROR: line 139: Uncaught (in promise) SecurityError: play()
must be initiated by a user gesture.
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> But here it does stringify. Weird, how's this different from the other
case?

Reason is that I forgot to update this file.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1263
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1263: // 6 -
For each promise in the list of pending play romises, reject it with
NotSupportedError.
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> Can you sweep your CL and update step numbers and comments to use what
finally ended up in the spec?

Done.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1973
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1973: if
(m_error && m_error->code() == MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED)
{
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> 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?

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.

> 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.

Updated things. Not exactly what you said but let me know.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode2231
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2231:
playInternal();
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> 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 :)

Done.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3592
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3592:
BLINK_FROM_HERE, new
Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, this)));
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> 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?

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

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3603
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3603:
scheduleEvent(EventTypeNames::playing);
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> 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".

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?

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3623
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3623: message =
"The play() command was interrupted by another command like load() or
pause().";
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> 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.

Done.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3627
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3627: message =
"Failed to load because no supported source was found.";
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> It would be good to split this too, to distinguish actively failing at
resource selection vs. already having failed before.

Done.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode35
third_party/WebKit/Source/core/html/HTMLMediaElement.h:35: #include
"core/html/MediaError.h"
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> How about including DOMException.h and using ExceptionCode instead?
Would be nice to not spread MediaError around too much.

Included ExceptionCode.h.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp#newcode318
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:318:
mediaElement().playInternal();
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> Also potentially risky.

Reverted.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
(right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp#newcode391
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:391:
mediaElement().playInternal();
On 2016/02/02 at 09:56:33, philipj_UTC7 wrote:
> Hmm, this case need not require a user gesture, but it'd also be bad
if it could bypass the new m_error check.

Fixed.

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/web/WebViewImpl.cpp
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode3528
third_party/WebKit/Source/web/WebViewImpl.cpp:3528:
mediaElement->playInternal();
On 2016/02/02 at 09:56:34, philipj_UTC7 wrote:
> Dito.

Fixed.

https://codereview.chromium.org/1576283003/

[dalec...@chromium.org]

On 2016/02/03 at 04:16:33, philipj wrote:
> On 2016/02/02 19:53:02, DaleCurtis wrote:
> > Haven't reviewed, but do we want to take this opportunity to only return the
> > promise once playback _actually_ starts? I.e. frame 0 of audio and frame 0
of
> > video have been played out.
> 
> That's the goal, but it'll require fixing
https://github.com/whatwg/html/issues/309 in the spec. In
https://github.com/whatwg/html/pull/509#issuecomment-173203696 I say some things
about what seems tricky here. Basically, I think that in order to get this
right, we have to send each play or pause command to the media pipeline and
track their progress individually, resolving each of them in order only. And
when the media element is reset by calling load(), even if there's a play/pause
being handled by the media pipeline, that particular play/pause promise will be
rejected with AbortError and forgotten.
> 
> Do you have any hunch about how complicated it would be to track individual
play/pause commands in order in WebMediaPlayerImpl? It would for sure change
most things about this feature, in particular it should never be Blink that
decides to resolve these promises based on readyState.

Not sure I follow? Individual play/pause is already tracked in WMPI (and by the
WMPDelegate+MediaWebContentsObserver on the browser side for power save blocking
/ media session).

For Chrome it shouldn't be that hard, all we'd need is a callback fired when the
_second_ frame of audio is actually written to the device. The first frame is
written before a device is actually connected. The cheap way to do this is to
just monitor current time and see when it rise above 0.  The less cheap way is
to plumb a callback which triggers once we have steady state audio delivery (or
frame delivery in the video only case) (or some combination thereof; none are
really that hard, just messy).

https://codereview.chromium.org/1576283003/

[dalec...@chromium.org]

On 2016/02/04 at 09:57:20, philipj wrote:
> 
> What I mean by "track individual play/pause commands" is this idea: for each
call to WebMediaPlayer::play() or ::pause() from Blink, there is a callback
argument, and those callbacks must be called in order, and at the right time
relative to readyState transitions and currentTime changes, as Blink would
resolve/reject promises in response to those callbacks.

This wouldn't be hard for the same reason mentioned above; all WMP
implementations are required to notify the delegate of play/pause sequences in
order.

(Also +1 to your off topic suggestion, it'd be very nice to avoid WMP having to
worry about readyState/networkState/currentTime==duration/etc).

https://codereview.chromium.org/1576283003/