Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1576283003:
Have HTMLMediaElement::play() return a Promise. (Closed)
|
Created:
4 years, 11 months ago by mlamouri (slow - plz ping) Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHave 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
Committed: https://crrev.com/dd99205e1045321b1812f64c5838e641cda7a2e9
Cr-Commit-Position: refs/heads/master@{#377608}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : WIP #Patch Set 4 : oupsy tests #Patch Set 5 : tests #Patch Set 6 : #
Total comments: 45
Patch Set 7 : review comments #
Total comments: 7
Patch Set 8 : review comments #
Total comments: 2
Patch Set 9 : hiroshige comments #
Total comments: 69
Patch Set 10 : review comments #
Total comments: 14
Patch Set 11 : review comments and cancel tasks #
Total comments: 1
Patch Set 12 : update layout tests #
Total comments: 11
Patch Set 13 : #Messages
Total messages: 51 (16 generated)
Description was changed from ========== POC of HTMLMediaElement.play() returning a Promise. This is a quick and dirty patch partially implementing the following proposal: https://github.com/whatwg/html/issues/505 The implementation lack: - tests; - DOMException granularity (sending fake ones); - clean support for internal play() method; - support for embedder's rejecting a play() request [needed?]. BUG=<TODO> ========== to ========== WIP of HTMLMediaElement.play() returning a Promise. This is a WIP patch implementing the following proposal: https://github.com/whatwg/html/issues/505 There are still a few things to clean up and some tests to add but the gist is there. BUG=579541 ==========
Description was changed from ========== WIP of HTMLMediaElement.play() returning a Promise. This is a WIP patch implementing the following proposal: https://github.com/whatwg/html/issues/505 There are still a few things to clean up and some tests to add but the gist is there. BUG=579541 ========== to ========== 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 This is also adding a new exception type called NotAllowedError. BUG=579541 ==========
Description was changed from ========== 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 This is also adding a new exception type called NotAllowedError. BUG=579541 ========== to ========== 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__g3yI... This is also adding a new exception type called NotAllowedError. BUG=579541 ==========
mlamouri@chromium.org changed reviewers: + philipj@opera.com
Philip, PTAL.
OK, took a first pass. It took a while, so I've only skimmed the new tests. They could already be complete, but the general approach I'd take is one test per place in the spec that says either resolves or rejects promises, so separate tests for the two ways to get NotSupportedError. And as many crazy pendantic tests about task/microtask/event ordering as you can imagine bugs for :) https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... 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/Lay... third_party/WebKit/LayoutTests/media/W3C/audio/events/event_pause_manual-expected.txt:1: CONSOLE ERROR: Uncaught (in promise) [object DOMException] Too bad this doesn't stringify to the exception message, then this could actually have been used to see why it was rejected :/ https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:7: // This is testing the behavior of play() with regards to the returned 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. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:42: function playWithUserGesture() 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. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:174: internals.setMediaElementNetworkState(mediaElement, 6 /* NetworkStateDecodeError */); 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. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... 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. But here it does stringify. Weird, how's this different from the other case? https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:85: { "NotAllowedError", "The request is not allowed by the user agent or the platform in the current context.", 0 }, Oh, somehow I failed to notice or just forgot that https://github.com/heycam/webidl/pull/85 was still open. Document that this was added for HTML and Media Session, assuming you agree with using it for activate() also? https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1263: // 6 - For each promise in the list of pending play romises, reject it with NotSupportedError. Can you sweep your CL and update step numbers and comments to use what finally ended up in the spec? https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1973: if (m_error && m_error->code() == MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) { 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. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2231: playInternal(); 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 :) https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3592: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, this))); 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? https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3603: scheduleEvent(EventTypeNames::playing); 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". https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3623: message = "The play() command was interrupted by another command like load() or pause()."; 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. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3627: message = "Failed to load because no supported source was found."; It would be good to split this too, to distinguish actively failing at resource selection vs. already having failed before. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:35: #include "core/html/MediaError.h" How about including DOMException.h and using ExceptionCode instead? Would be nice to not spread MediaError around too much. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:318: mediaElement().playInternal(); Also potentially risky. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:391: mediaElement().playInternal(); Hmm, this case need not require a user gesture, but it'd also be bad if it could bypass the new m_error check. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3528: mediaElement->playInternal(); Dito.
dalecurtis@chromium.org changed reviewers: + dalecurtis@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.
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.
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/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes-expected.txt (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3528: mediaElement->playInternal(); On 2016/02/02 at 09:56:34, philipj_UTC7 wrote: > Dito. Fixed.
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).
On 2016/02/03 23:58:06, DaleCurtis wrote: > 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). 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. However, I'm not sure it has to be that complicated. If WebMediaPlayerImpl has accurate callbacks for when currentTime begins moving in response to a play and stops in response to a play, Blink should be able to reconstruct what happened to the pending plays and pauses from that. In particular, when there is a play();pause() sequence, one has to be able to tell if the play moved currentTime forward ever so slightly, or not. The timeChanged() callback could work if it were called at precisely the right time after play(), but there's no timeStopped() callback for pause().
(Off-topic: More generally, I think that the WebMediaPlayerClient interface knows more about HTMLMediaElement concepts that ideal, in particular networkState, readyState and time/duration changes must happen, and would love to discuss how to improve that at some point.)
https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:42: function playWithUserGesture() On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > 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. Imported w-p-t tests all live in LayoutTests/imported/web-platform-tests/html/semantics/embedded-content/media-elements/, but most interesting aren't run because they need wptserve. w-p-t tests anywhere else are ad-hoc imports with no sync process. For this, simply writing the tests in the style of w-p-t where this test already is would be OK. I think that if we don't write w-p-t now, it just won't ever be prioritized, so if you like I can take a shot at converting these tests and submitted what can be submitted upstream. Let me know :) https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:85: { "NotAllowedError", "The request is not allowed by the user agent or the platform in the current context.", 0 }, On 2016/02/02 09:56:33, philipj_UTC7 wrote: > Oh, somehow I failed to notice or just forgot that > https://github.com/heycam/webidl/pull/85 was still open. > > Document that this was added for HTML and Media Session, assuming you agree with > using it for activate() also? Ping. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1973: if (m_error && m_error->code() == MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) { On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > 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. 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. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3592: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, this))); On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > 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. Please do, this looks like a potential crash. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3603: scheduleEvent(EventTypeNames::playing); On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > 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? 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.) https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1943: message = "play() must be initiated by a user gesture."; Perhaps "can only" to match the console message? https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1958: playInternal(); play() already calls playInternal() https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:55: class MediaError; revert
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).
On 2016/02/04 19:36:32, DaleCurtis wrote: > 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. That sounds very promising! Mounir, are you interesting in taking a look at that? Depending on how big a change it is, it may or not make sense to first ship promise-vending play() with the current (per-spec, inaccurate) resolution time. If it seems simple enough, I can prioritize changing the spec to match.
mlamouri@chromium.org changed reviewers: + hiroshige@chromium.org
philipj@, PTAL. All comments should be resolved. hiroshige@, could you PTAL at the usage of tasks with oilpan and whether it can lead to race conditions where `this` is destroyed before the task is run. dalecurtis@, I looked at WMPI and indeed play() call notifies the delegate but it does that immediately and doesn't wait for play to actually happen. Or did you mean that we could wait for play to actually start before notifying? https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:42: function playWithUserGesture() On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > > 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. > > Imported w-p-t tests all live in LayoutTests/imported/web-platform-tests/html/semantics/embedded-content/media-elements/, but most interesting aren't run because they need wptserve. w-p-t tests anywhere else are ad-hoc imports with no sync process. > > For this, simply writing the tests in the style of w-p-t where this test already is would be OK. I think that if we don't write w-p-t now, it just won't ever be prioritized, so if you like I can take a shot at converting these tests and submitted what can be submitted upstream. Let me know :) I would be interested to help converting media tests to be upstreamable. Switching to the testharness.js will be a first step. I would recommend to have a chromium/ repository for the tests that can't be upstream'd (this is a pattern used by SW and others). I don't think blocking this CL on converting the test would help though I can start a CL to convert the test to testharness. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:85: { "NotAllowedError", "The request is not allowed by the user agent or the platform in the current context.", 0 }, On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > On 2016/02/02 09:56:33, philipj_UTC7 wrote: > > Oh, somehow I failed to notice or just forgot that > > https://github.com/heycam/webidl/pull/85 was still open. > > > > Document that this was added for HTML and Media Session, assuming you agree with > > using it for activate() also? > > Ping. Done. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1973: if (m_error && m_error->code() == MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) { On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > > 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. > > 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. Done. (for me to keep track) https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3592: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, this))); On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > > 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. > > Please do, this looks like a potential crash. I asked haraken@. He gave me a hint and I'm adding hiroshige@ as a reviewer. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3603: scheduleEvent(EventTypeNames::playing); On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > > 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? > > 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.) Done. https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1943: message = "play() must be initiated by a user gesture."; On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > Perhaps "can only" to match the console message? Done. https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1958: playInternal(); On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > play() already calls playInternal() Not sure why that was here. https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:55: class MediaError; On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > revert Why?
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576283003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576283003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1576283003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3616: BLINK_FROM_HERE, new Task(threadSafeBind(&HTMLMediaElement::resolvePlayPromises, AllowCrossThreadAccess(this)))); > at the usage of tasks with oilpan and whether it can lead to > race conditions where `this` is destroyed before the task is run. (1) The code seems single threaded, because scheduleResolvePlayPromises() post a task to the current thread (==main thread?), right? If so, then we can (or should) use bind() instead of threadSafeBind() here. (2) [A] Do we want to keep |*this| alive until the posted task is executed? or [B] Do we allow |*this| to be deleted before the task is executed and in that case just omit the task execution? In case of [A]: Basically, bind() does NOT keep a reference if we pass raw pointers (T*) if T is NOT on Oilpan heap, and DOES keep a reference if T IS on-heap (i.e. GarbageCollected etc.). So we have to pass a reference explicitly, perhaps PassRefPtrWillBeRawPtr<HTMLMediaElement>(this) or something. In case of [B]: We have to use weak pointers. Perhaps WeakPtrWillBeWeakPersistent<HTMLMediaElement>? But WTF::Function doesn't have sufficient support for passing WeakPersistent. I'll create a CL for adding this support.
As discussed via chat, the delegate does indeed notify right away. I was more just pointing out that we already have the state available rather than saying we should use the delegate notifications as the signal. For true correctness you'd want to only answer the promise once: if (video) VideoFrameCompositor received it's first frame. if (audio) RendererWebAudioDeviceImpl received it's first Render() call.
hiroshige@, PTAL. Regarding the real play callback, I think this should be in a follow-up. https://codereview.chromium.org/1576283003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3616: BLINK_FROM_HERE, new Task(threadSafeBind(&HTMLMediaElement::resolvePlayPromises, AllowCrossThreadAccess(this)))); On 2016/02/18 at 18:32:41, hiroshige wrote: > > at the usage of tasks with oilpan and whether it can lead to > > race conditions where `this` is destroyed before the task is run. > > (1) The code seems single threaded, because scheduleResolvePlayPromises() post a task to the current thread (==main thread?), right? > If so, then we can (or should) use bind() instead of threadSafeBind() here. You are correct, it doesn't need to be thread safe but haraken@ told me that it might be the solution in order to keep the object around. I was probably not clear when I explained my issue. Glad to hear there is a better solution :) > (2) > [A] Do we want to keep |*this| alive until the posted task is executed? > or > [B] Do we allow |*this| to be deleted before the task is executed and in that case just omit the task execution? > > In case of [A]: > Basically, bind() does NOT keep a reference if we pass raw pointers (T*) if T is NOT on Oilpan heap, and DOES keep a reference if T IS on-heap (i.e. GarbageCollected etc.). > So we have to pass a reference explicitly, perhaps PassRefPtrWillBeRawPtr<HTMLMediaElement>(this) or something. > > In case of [B]: > We have to use weak pointers. > Perhaps WeakPtrWillBeWeakPersistent<HTMLMediaElement>? > But WTF::Function doesn't have sufficient support for passing WeakPersistent. > I'll create a CL for adding this support. I think keeping the object alive is fine, it sounds more consistent and probably will avoid leaking or GC behaviour. I've applied solution A. PTAL.
https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:55: class MediaError; On 2016/02/18 17:06:06, Mounir Lamouri (slow) wrote: > On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > > revert > > Why? Oops, I was looking at the diff between PS6 and PS8, which makes it look like this was unnecessarily added, where actually you're just not removing it any more.
Recent code changes look good, see comments. Looks like the potential crash also was resolved. I still need to look closely at the tests. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:42: function playWithUserGesture() On 2016/02/18 17:06:06, Mounir Lamouri (slow) wrote: > On 2016/02/04 at 10:54:20, philipj_UTC7 wrote: > > On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > > > 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. > > > > Imported w-p-t tests all live in > LayoutTests/imported/web-platform-tests/html/semantics/embedded-content/media-elements/, > but most interesting aren't run because they need wptserve. w-p-t tests anywhere > else are ad-hoc imports with no sync process. > > > > For this, simply writing the tests in the style of w-p-t where this test > already is would be OK. I think that if we don't write w-p-t now, it just won't > ever be prioritized, so if you like I can take a shot at converting these tests > and submitted what can be submitted upstream. Let me know :) > > I would be interested to help converting media tests to be upstreamable. > Switching to the testharness.js will be a first step. I would recommend to have > a chromium/ repository for the tests that can't be upstream'd (this is a pattern > used by SW and others). > > I don't think blocking this CL on converting the test would help though I can > start a CL to convert the test to testharness. I was hoping to avoid reviewing these tests in their current form at all. Will take a look, then. https://codereview.chromium.org/1576283003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:174: internals.setMediaElementNetworkState(mediaElement, 6 /* NetworkStateDecodeError */); On 2016/02/03 19:28:57, Mounir Lamouri (slow) wrote: > 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. It's the use of setMediaElementNetworkState to signal an error that I'm not a fan of, not the pass condition for how that error is handled. But, OK, realistically I'm not going to try to kill these extra uses of WebMediaPlayer::NetworkState any time soon, and to introduce internals.triggerMediaElementDecodeError() would internally do the same thing. So... very well. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:85: // Part of WebIDL. Used by HTML and Media Session API. I wanted to say "the above are also part of WebIDL", but apparently not. Filed: https://github.com/w3c/push-api/issues/186 In any event most of the above are part of WebIDL. Putting things that aren't at the end and adding a comment advising against adding any more without a WebIDL issue might be good, but not your job if you don't want it.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
Spent some quality time on the tests. In addition to addressing issues, can you also check if all of the cases in my "one test per place that the spec either resolves or rejects" are covered? https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:4: <script src=video-test.js></script> Can you add a TODO(mlamouri) in the style of https://codereview.chromium.org/1715303002/, assuming you approve? It currently takes mindreader skills to know that I have any kind of ambition to move media tests over to testharness.js and upstream to w-p-t. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:12: // first function in the array is ran. A test is considered done when the s/ran/run/ here and below https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: // Each test should start by prting its name in order to facilitate reading s/prting/printing/ https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:35: return mediaElement.play().then(function() { The return value is never used, skip return to make that clear. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:36: consoleWrite("Promise Resolved"); Verify arguments.length == 1 and arguments[0]===undefined, if that is in fact what happens. The spec does say "resolve it with undefined" and it looks like script-created promises that resolve with no argument still call the callback with 1 argument (undefined). Also the capitalization of Resolved and Failed looks a bit random in the output. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:38: consoleWrite("Promise Failed with " + e.name); Verify arguments.length == 1, e instanceof DOMException and also print e.message. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:44: if (!window.eventSender) { If we're not running under LayoutTests we will have already choked on internals. Maybe bail out in start() if that's valuable? https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:79: play(); Verify the conditions for "currently loading" that this is intended to test. Unless there is a bug, networkState is NETWORK_NO_SOURCE here, and it's not really true that it's loading, the src attribute hasn't even been inspected yet, rather we're inside the resource selection algorithm awaiting a stable state (step 4). Roughly speaking, I think there should be one test per place that the spec either resolves or rejects promises: 1. Two for NotAllowedError and NotSupportedError rejections at the top of play() 2. One for "The media element is already playing" resolving towards the end of play() 3. Four for "notify about playing", one for each call site 4. One for the media element load algorithm rejection 5. One for the dedicated media source failure steps 6. One for the internal pause steps rejection This playLoading() test doesn't fall into any of these buckets, so unless it's anticipating a particular bug I think it would be fine to remove it. It could probably be tweaked into one of the "notify about playing" cases, namely the one for "If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA" https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:82: // Test that play() on an element that is already loaded returns a This can be rephrased as testing the "notify about playing" case inside play(). Waiting for canplay and calling the test playWhenCanPlay would make the test as strict possible. For this test, because the promise is resolved while still inside play(), verify that this is the case with a script-created promise right after the call to play() that is expected to be resolved after. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:102: mediaElement.load(); Calling load() in addition to setting src doesn't do anything useful. AFAICT, the load() call can be dropped here and in all cases below, except loadRejectPendingPromises(). https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:106: // returns a rejected promise if there is no user gesture. Looks like the description should be switched with the below test, this is the opposite of what is being tested. (This test falls outside the buckets I listed, but that makes sense because it's testing that the user gesture setting doesn't simply always reject or something silly like that.) https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:120: // Test that play() on an element when media playback requires a gesture Switch description with above. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:143: var mediaFile = findMediaFile("audio", "content/garbage"); content/garbage doesn't exist. "data:," is a fine URL for getting into MEDIA_ERR_SRC_NOT_SUPPORTED. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:175: play(); This doesn't map to any real-world situation AFAICT. If there's a decode error as you reach loadedmetadata, how does the play() actually succeed? Per spe the promise should rather be left pending until a seek or something causes it to succeed. That this test can succeed suggests that we've put the HTMLMediaElement in an inconsistent state. It doesn't look like there are any tests for MEDIA_ERR_DECODE with plain media elements that you could adapt, so I guess this is OK. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:179: // Test that play() returns a resolved promise if called after the IMHO it would be more interesting to see what happens if the play promise is already pending when the network error happens. The answer is still nothing, of course. Maybe move these two tests to the end with comments as they're not testing anything in the spec? https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:212: var mediaFile = findMediaFile("audio", "content/garbage"); data:, here and everywhere content/garbage is used. Or say content/404 if you're explicitly testing that somewhere. Or load an existing file with garbage if that's interesting, but I'd say go the quickest route to MEDIA_ERR_SRC_NOT_SUPPORTED. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:252: // Test that play() returns a rejected promise if the element had an You already test that the promise rejects when error is MEDIA_ERR_SRC_NOT_SUPPORTED and that it's really checking the error attribute with playSrcChangedAfterError. This is only testing setting src really synchronously invokes the media element load algorithm and clears the error, which is more like it's testing the src attribute setter than the play() promise. I suggest keeping only one of this and above test to guard against that possible bug of using something other than the error attribute. This test, being the one with stricter timing, might be the best to keep. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:273: // that it actually happens later. It's unclear why. This sounds like it could be the task ordering problem that I wrote an async_test for below. Can you investigate? It seems very unlikely that listening for the event should change when it's fired. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:280: // words, pause() doesn't cancel play() because it was resolved Right, this is an important case that will potentially change when the playing event is delayed until playback really begins. If you change playLoaded() as suggested there will already be a test to verify the timing of the play() promise resolving for this case, so if you find this test redundant I won't mind if you remove it. (The timing is the relevant bit, pause() is just used to test it indirectly here, it could just as well have been load().) https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:282: function playFollowedByPauseWhenLoaded() I suggest avoiding "loaded" as there's no readyState that means "fully loaded". readyState>=HAVE_FUTURE_DATA is the relevant condition, so playAndPauseWhenCanPlay could be a more fitting name. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:306: function playFollowedByPauseWhenLoading() playAndPauseBeforeCanPlay if you change the naming of the above. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:313: run("mediaElement.src = '" + mediaFile + "'"); This test shouldn't need to set a src at all. Trying to catch the element in a readyState >HAVE_NOTHING but <HAVE_FUTURE_DATA is likely to be racy, there have been many flaky media tests due to this kind of thing. You could make a copy of the below test and call it pauseRejectsPendingPromises(). https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:324: function loadRejectPendingPromises() Rejects with an s https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:337: // Test that changing the src rejects the pending play() promises. Note that this is a paranoid test or remove it. That setting the src attribute invokes the media element load algorithm should be covered by many other tests. It doesn't hurt, though, other than taking more time to run of course. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:363: consoleWrite("Should be after the play() promise is resolved"); I think this test is not quite right. Per spec the event is dispatched in the same task that resolves the promise, but the event is dispatched first. A script-created promise resolved in that event handler is still resolved before the play() promise, and therefore its callbacks should run first, unless there are some rules about promise resolution order that I'm not aware of. What you could perhaps do is to create and resolve a second promise here. We should at this point be after the play() promise is resolved but before its callback i run, and so a newly resolved promise should run after it, but still in the same task. Another bug we'll have is that the task that is queued resolve/reject promises won't be on the correct task source, and so the ordering relative to media element events can be wrong in this way: async_test(function() { var v = document.createElement('video'); v.src = 'data:,'; v.onerror = this.step_func(function() { assert_equals(v.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED); var order = []; var push = this.step_func(function(event) { order.push(event); if (order.length == 3) { assert_array_equals(order, ["volumechange", "rejection", "volumechange"]); this.done(); } }); v.onvolumechange = push.bind(this, "volumechange"); v.volume = 0.1; v.play().catch(push.bind(this, "rejection")); v.volume = 0.2; }); }); The actual order will be ["volumechange", "volumechange", "rejection"] because the timer to handle events is started by the first change to volume, and both events are handled at the same time.
https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:75: #include "platform/Task.h" We can remove this #include if we omit new Task()'s below. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3615: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, PassRefPtrWillBeRawPtr<HTMLMediaElement>(this)))); bind() usage looks good. BTW, could you omit new Task() here? i.e. BLINK_FROM_HERE, WTF::bind(&HTMLMediaElement::resolvePlayPromises, PassRefPtrWillBeRawPtr<HTMLMediaElement>(this))); I'm doing refactoring that removes |new Task()| calls. (CL: https://codereview.chromium.org/1713143002/) https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3621: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::rejectPlayPromises, PassRefPtrWillBeRawPtr<HTMLMediaElement>(this), code, message))); ditto.
Comments applied. PTAL. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:4: <script src=video-test.js></script> On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Can you add a TODO(mlamouri) in the style of https://codereview.chromium.org/1715303002/, assuming you approve? It currently takes mindreader skills to know that I have any kind of ambition to move media tests over to testharness.js and upstream to w-p-t. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:12: // first function in the array is ran. A test is considered done when the On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > s/ran/run/ here and below Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:18: // Each test should start by prting its name in order to facilitate reading On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > s/prting/printing/ Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:35: return mediaElement.play().then(function() { On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > The return value is never used, skip return to make that clear. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:36: consoleWrite("Promise Resolved"); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Verify arguments.length == 1 and arguments[0]===undefined, if that is in fact what happens. The spec does say "resolve it with undefined" and it looks like script-created promises that resolve with no argument still call the callback with 1 argument (undefined). Done. > Also the capitalization of Resolved and Failed looks a bit random in the output. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:38: consoleWrite("Promise Failed with " + e.name); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Verify arguments.length == 1, e instanceof DOMException and also print e.message. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:44: if (!window.eventSender) { On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > If we're not running under LayoutTests we will have already choked on internals. Maybe bail out in start() if that's valuable? Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:79: play(); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Verify the conditions for "currently loading" that this is intended to test. Unless there is a bug, networkState is NETWORK_NO_SOURCE here, and it's not really true that it's loading, the src attribute hasn't even been inspected yet, rather we're inside the resource selection algorithm awaiting a stable state (step 4). > > Roughly speaking, I think there should be one test per place that the spec either resolves or rejects promises: > 1. Two for NotAllowedError and NotSupportedError rejections at the top of play() Already there. > 2. One for "The media element is already playing" resolving towards the end of play() Added. > 3. Four for "notify about playing", one for each call site Done. > 4. One for the media element load algorithm rejection Already there. > 5. One for the dedicated media source failure steps Already there. > 6. One for the internal pause steps rejection Already there. > This playLoading() test doesn't fall into any of these buckets, so unless it's anticipating a particular bug I think it would be fine to remove it. It could probably be tweaked into one of the "notify about playing" cases, namely the one for "If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA" Done. Kind of. I think this test was already testing this case. Not sure how to make it more obvious. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:82: // Test that play() on an element that is already loaded returns a On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > This can be rephrased as testing the "notify about playing" case inside play(). Waiting for canplay and calling the test playWhenCanPlay would make the test as strict possible. > > For this test, because the promise is resolved while still inside play(), verify that this is the case with a script-created promise right after the call to play() that is expected to be resolved after. Done, apart from the promise thing. The play() promise is resolved in a task so it doesn't sound right. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:102: mediaElement.load(); On 2016/02/22 at 06:34:25, philipj_UTC7 wrote: > Calling load() in addition to setting src doesn't do anything useful. AFAICT, the load() call can be dropped here and in all cases below, except loadRejectPendingPromises(). Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:106: // returns a rejected promise if there is no user gesture. On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Looks like the description should be switched with the below test, this is the opposite of what is being tested. > > (This test falls outside the buckets I listed, but that makes sense because it's testing that the user gesture setting doesn't simply always reject or something silly like that.) Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:120: // Test that play() on an element when media playback requires a gesture On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Switch description with above. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:143: var mediaFile = findMediaFile("audio", "content/garbage"); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > content/garbage doesn't exist. "data:," is a fine URL for getting into MEDIA_ERR_SRC_NOT_SUPPORTED. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:175: play(); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > This doesn't map to any real-world situation AFAICT. If there's a decode error as you reach loadedmetadata, how does the play() actually succeed? Per spe the promise should rather be left pending until a seek or something causes it to succeed. That this test can succeed suggests that we've put the HTMLMediaElement in an inconsistent state. > > It doesn't look like there are any tests for MEDIA_ERR_DECODE with plain media elements that you could adapt, so I guess this is OK. Ack. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:179: // Test that play() returns a resolved promise if called after the On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > IMHO it would be more interesting to see what happens if the play promise is already pending when the network error happens. The answer is still nothing, of course. Maybe move these two tests to the end with comments as they're not testing anything in the spec? I've added two comments. I guess the reason why I can't keep the promise pending is that testing that nothing happens is flaky and it's not worth the trouble given what is being tested here. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:212: var mediaFile = findMediaFile("audio", "content/garbage"); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > data:, here and everywhere content/garbage is used. Or say content/404 if you're explicitly testing that somewhere. Or load an existing file with garbage if that's interesting, but I'd say go the quickest route to MEDIA_ERR_SRC_NOT_SUPPORTED. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:252: // Test that play() returns a rejected promise if the element had an On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > You already test that the promise rejects when error is MEDIA_ERR_SRC_NOT_SUPPORTED and that it's really checking the error attribute with playSrcChangedAfterError. This is only testing setting src really synchronously invokes the media element load algorithm and clears the error, which is more like it's testing the src attribute setter than the play() promise. I suggest keeping only one of this and above test to guard against that possible bug of using something other than the error attribute. This test, being the one with stricter timing, might be the best to keep. There are 4 src_not_supported related tests: 1. play() on a src_not_supported file, before the error fires. 2. play() on a src_not_supported file, after the error fires. 3. play() on a file that was src_not_supported. 4. play() on a file that was src_not_supported and just changed. I agree that it might be similar to some other but it still slightly different. If we remove #2 or #3 (which #4 is close to), it will be unclear what the error is if #4 fails. With #4 failing and #2 and #3 passing, it will be clear to developers that there is a race somewhere. I guess it's a different approach: my goal isn't to test the spec but to make sure that any regression is caught and this issue is something I had while writing the code. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:273: // that it actually happens later. It's unclear why. On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > This sounds like it could be the task ordering problem that I wrote an async_test for below. Can you investigate? It seems very unlikely that listening for the event should change when it's fired. Hmm, not sure if this comment is clear. Also it's not actually a bug: 'playing' gets fired but I wasn't expecting this. I assume this is because play() sets pause to false and when src is set and the data loaded, playback starts. Am I right? I removed the TODO. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:280: // words, pause() doesn't cancel play() because it was resolved On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Right, this is an important case that will potentially change when the playing event is delayed until playback really begins. If you change playLoaded() as suggested there will already be a test to verify the timing of the play() promise resolving for this case, so if you find this test redundant I won't mind if you remove it. (The timing is the relevant bit, pause() is just used to test it indirectly here, it could just as well have been load().) I tend to prefer keeping tests that look redundant. My experience shows that they often end up useful in unexpected ways. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:282: function playFollowedByPauseWhenLoaded() On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > I suggest avoiding "loaded" as there's no readyState that means "fully loaded". readyState>=HAVE_FUTURE_DATA is the relevant condition, so playAndPauseWhenCanPlay could be a more fitting name. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:306: function playFollowedByPauseWhenLoading() On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > playAndPauseBeforeCanPlay if you change the naming of the above. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:313: run("mediaElement.src = '" + mediaFile + "'"); On 2016/02/22 at 06:34:25, philipj_UTC7 wrote: > This test shouldn't need to set a src at all. Trying to catch the element in a readyState >HAVE_NOTHING but <HAVE_FUTURE_DATA is likely to be racy, there have been many flaky media tests due to this kind of thing. You could make a copy of the below test and call it pauseRejectsPendingPromises(). Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:324: function loadRejectPendingPromises() On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Rejects with an s Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:337: // Test that changing the src rejects the pending play() promises. On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > Note that this is a paranoid test or remove it. That setting the src attribute invokes the media element load algorithm should be covered by many other tests. It doesn't hurt, though, other than taking more time to run of course. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:363: consoleWrite("Should be after the play() promise is resolved"); On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > I think this test is not quite right. Per spec the event is dispatched in the same task that resolves the promise, but the event is dispatched first. A script-created promise resolved in that event handler is still resolved before the play() promise, and therefore its callbacks should run first, unless there are some rules about promise resolution order that I'm not aware of. > > What you could perhaps do is to create and resolve a second promise here. We should at this point be after the play() promise is resolved but before its callback i run, and so a newly resolved promise should run after it, but still in the same task. > > Another bug we'll have is that the task that is queued resolve/reject promises won't be on the correct task source, and so the ordering relative to media element events can be wrong in this way: > > async_test(function() { > var v = document.createElement('video'); > v.src = 'data:,'; > v.onerror = this.step_func(function() { > assert_equals(v.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED); > var order = []; > var push = this.step_func(function(event) { > order.push(event); > if (order.length == 3) { > assert_array_equals(order, ["volumechange", "rejection", "volumechange"]); > this.done(); > } > }); > v.onvolumechange = push.bind(this, "volumechange"); > v.volume = 0.1; > v.play().catch(push.bind(this, "rejection")); > v.volume = 0.2; > }); > }); > > The actual order will be ["volumechange", "volumechange", "rejection"] because the timer to handle events is started by the first change to volume, and both events are handled at the same time. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:85: // Part of WebIDL. Used by HTML and Media Session API. On 2016/02/19 at 08:21:48, philipj_UTC7 wrote: > I wanted to say "the above are also part of WebIDL", but apparently not. Filed: > https://github.com/w3c/push-api/issues/186 > > In any event most of the above are part of WebIDL. Putting things that aren't at the end and adding a comment advising against adding any more without a WebIDL issue might be good, but not your job if you don't want it. Removed. "Part of WebIDL.". Though, I've added the ordering comment because I tried to append this after "DataCloneError" and it was failing. It seems that some code expect ordering so we shouldn't break this assumption. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:75: #include "platform/Task.h" On 2016/02/22 at 17:43:10, hiroshige wrote: > We can remove this #include if we omit new Task()'s below. Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3615: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::resolvePlayPromises, PassRefPtrWillBeRawPtr<HTMLMediaElement>(this)))); On 2016/02/22 at 17:43:10, hiroshige wrote: > bind() usage looks good. > > BTW, could you omit new Task() here? i.e. > BLINK_FROM_HERE, WTF::bind(&HTMLMediaElement::resolvePlayPromises, PassRefPtrWillBeRawPtr<HTMLMediaElement>(this))); > I'm doing refactoring that removes |new Task()| calls. > (CL: https://codereview.chromium.org/1713143002/) Done. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3621: BLINK_FROM_HERE, new Task(WTF::bind(&HTMLMediaElement::rejectPlayPromises, PassRefPtrWillBeRawPtr<HTMLMediaElement>(this), code, message))); On 2016/02/22 at 17:43:10, hiroshige wrote: > ditto. Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576283003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576283003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM, but playRaceWithSrcChangeError() needs some investigation. Hopefully it's just me being wrong again :) https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:82: // Test that play() on an element that is already loaded returns a On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > This can be rephrased as testing the "notify about playing" case inside > play(). Waiting for canplay and calling the test playWhenCanPlay would make the > test as strict possible. > > > > For this test, because the promise is resolved while still inside play(), > verify that this is the case with a script-created promise right after the call > to play() that is expected to be resolved after. > > Done, apart from the promise thing. The play() promise is resolved in a task so > it doesn't sound right. Sorry about that, you're right. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:179: // Test that play() returns a resolved promise if called after the On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > IMHO it would be more interesting to see what happens if the play promise is > already pending when the network error happens. The answer is still nothing, of > course. Maybe move these two tests to the end with comments as they're not > testing anything in the spec? > > I've added two comments. I guess the reason why I can't keep the promise pending > is that testing that nothing happens is flaky and it's not worth the trouble > given what is being tested here. Agree, this is OK for now. We should probably revisit the spec and do something better with these errors anyway. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:252: // Test that play() returns a rejected promise if the element had an On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > You already test that the promise rejects when error is > MEDIA_ERR_SRC_NOT_SUPPORTED and that it's really checking the error attribute > with playSrcChangedAfterError. This is only testing setting src really > synchronously invokes the media element load algorithm and clears the error, > which is more like it's testing the src attribute setter than the play() > promise. I suggest keeping only one of this and above test to guard against that > possible bug of using something other than the error attribute. This test, being > the one with stricter timing, might be the best to keep. > > There are 4 src_not_supported related tests: > 1. play() on a src_not_supported file, before the error fires. > 2. play() on a src_not_supported file, after the error fires. > 3. play() on a file that was src_not_supported. > 4. play() on a file that was src_not_supported and just changed. > > I agree that it might be similar to some other but it still slightly different. > If we remove #2 or #3 (which #4 is close to), it will be unclear what the error > is if #4 fails. With #4 failing and #2 and #3 passing, it will be clear to > developers that there is a race somewhere. > > I guess it's a different approach: my goal isn't to test the spec but to make > sure that any regression is caught and this issue is something I had while > writing the code. Aha, well I think you've found a bit of a problem here. See next comment. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:273: // that it actually happens later. It's unclear why. On 2016/02/23 16:48:59, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > This sounds like it could be the task ordering problem that I wrote an > async_test for below. Can you investigate? It seems very unlikely that listening > for the event should change when it's fired. > > Hmm, not sure if this comment is clear. Also it's not actually a bug: 'playing' > gets fired but I wasn't expecting this. I assume this is because play() sets > pause to false and when src is set and the data loaded, playback starts. Am I > right? > > I removed the TODO. Right, setting the src should clear the error, and then there's nothing stopping the play from succeeding. So why is the play() promise rejected? First, can you assert that the state is as expected right after setting src / before calling play. readyState should be HAVE_NOTHING and the error should be null. It's HTMLMediaElement::noneSupported() that's rejecting this promise, but how did we get there? Unless I'm now extra confused, I think for this test, the promise should be resolved. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:280: // words, pause() doesn't cancel play() because it was resolved On 2016/02/23 16:49:00, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > Right, this is an important case that will potentially change when the playing > event is delayed until playback really begins. If you change playLoaded() as > suggested there will already be a test to verify the timing of the play() > promise resolving for this case, so if you find this test redundant I won't mind > if you remove it. (The timing is the relevant bit, pause() is just used to test > it indirectly here, it could just as well have been load().) > > I tend to prefer keeping tests that look redundant. My experience shows that > they often end up useful in unexpected ways. Acknowledged. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:337: // Test that changing the src rejects the pending play() promises. On 2016/02/23 16:48:59, Mounir Lamouri wrote: > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > Note that this is a paranoid test or remove it. That setting the src attribute > invokes the media element load algorithm should be covered by many other tests. > It doesn't hurt, though, other than taking more time to run of course. > > Done. Not done, but that's OK :) https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:88: // Please add new entries at the end of this list to guarantee any ordering There's a comment at the top ExceptionCode.h that says to keep the lists in sync. Can you copy that to the top of this list as well? It's not really that the order cannot change, it just has to change in multiple places. Unless there's code comparing the ExceptionCode integer values? https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:34: function play() Maybe consoleWrite something so that one can tell from the output where play() is called? https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:37: consoleWrite("arguments.length: " + arguments.length); 100% optional nit: If you can make it output just "Promise resolved with undefined" if everything is as expected that would be nice. Same below. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:83: play(); For documentation purposes, can you do `testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_NOTHING)`? https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:100: testExpected(HTMLMediaElement.HAVE_ENOUGH_DATA, mediaElement.readyState); This could be flaky, readyState may only have reached HAVE_FUTURE_DATA. So testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_FUTURE_DATA, ">="). If you put "mediaElement.readyState" first and quoted the output should be nicer. Same for mediaElement.paused and any other instance in this test. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:123: waitForEvent('canplaythrough', function() { For good measure, when depending on canplaythrough, also set preload="auto". That's the current default, but it's not what Gecko does and it might change. Simply setting mediaElement.autoplay = true should also work. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:341: function loadRejectoPendingPromises() Now it says Rejects with an o :)
lgtm for bind() usage.
PTAL. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:273: // that it actually happens later. It's unclear why. On 2016/02/24 at 09:38:27, philipj_UTC7 wrote: > On 2016/02/23 16:48:59, Mounir Lamouri wrote: > > On 2016/02/22 at 06:34:24, philipj_UTC7 wrote: > > > This sounds like it could be the task ordering problem that I wrote an > > async_test for below. Can you investigate? It seems very unlikely that listening > > for the event should change when it's fired. > > > > Hmm, not sure if this comment is clear. Also it's not actually a bug: 'playing' > > gets fired but I wasn't expecting this. I assume this is because play() sets > > pause to false and when src is set and the data loaded, playback starts. Am I > > right? > > > > I removed the TODO. > > Right, setting the src should clear the error, and then there's nothing stopping the play from succeeding. So why is the play() promise rejected? First, can you assert that the state is as expected right after setting src / before calling play. readyState should be HAVE_NOTHING and the error should be null. > > It's HTMLMediaElement::noneSupported() that's rejecting this promise, but how did we get there? Unless I'm now extra confused, I think for this test, the promise should be resolved. As discussed on IRC, the issue was that the tasks resolving the promises needed to be cancelled which did not happen. There is no simple way to add this so I used CancellableTaskFactory with an error code as a member. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:88: // Please add new entries at the end of this list to guarantee any ordering On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > There's a comment at the top ExceptionCode.h that says to keep the lists in sync. Can you copy that to the top of this list as well? It's not really that the order cannot change, it just has to change in multiple places. Unless there's code comparing the ExceptionCode integer values? Done. I will check what the error was exactly. It might be that someone was comparing ExceptionCode. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:34: function play() On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > Maybe consoleWrite something so that one can tell from the output where play() is called? Done. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:37: consoleWrite("arguments.length: " + arguments.length); On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > 100% optional nit: If you can make it output just "Promise resolved with undefined" if everything is as expected that would be nice. Same below. I prefer to keep the consoleWrite() for the arguments length so it will be clearer if that regress. This is going to move to testharness soon anyway ;) https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:83: play(); On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > For documentation purposes, can you do `testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_NOTHING)`? Done. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:100: testExpected(HTMLMediaElement.HAVE_ENOUGH_DATA, mediaElement.readyState); On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > This could be flaky, readyState may only have reached HAVE_FUTURE_DATA. So testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_FUTURE_DATA, ">="). > > If you put "mediaElement.readyState" first and quoted the output should be nicer. Same for mediaElement.paused and any other instance in this test. Done^2. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:123: waitForEvent('canplaythrough', function() { On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > For good measure, when depending on canplaythrough, also set preload="auto". That's the current default, but it's not what Gecko does and it might change. Simply setting mediaElement.autoplay = true should also work. I used preload = "auto". Setting autoplay sounds like a recipe to get race conditions given that I later call play() ;) https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:341: function loadRejectoPendingPromises() On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > Now it says Rejects with an o :) Hmm, these two letters are not even on the same hand on my keyboard... :o
LGTM to land assuming that the fix actually changed the test expectations, otherwise there's something more going on. https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:88: // Please add new entries at the end of this list to guarantee any ordering On 2016/02/25 11:07:35, Mounir Lamouri wrote: > On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > > There's a comment at the top ExceptionCode.h that says to keep the lists in > sync. Can you copy that to the top of this list as well? It's not really that > the order cannot change, it just has to change in multiple places. Unless > there's code comparing the ExceptionCode integer values? > > Done. I will check what the error was exactly. It might be that someone was > comparing ExceptionCode. I also meant to imply that this comment at the bottom shouldn't be here, on the assumption that it is actually OK to change the order. (I guess you're investigating, just commenting so this warning isn't left without saying why order cannot change.) https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:37: consoleWrite("arguments.length: " + arguments.length); On 2016/02/25 11:07:35, Mounir Lamouri wrote: > On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > > 100% optional nit: If you can make it output just "Promise resolved with > undefined" if everything is as expected that would be nice. Same below. > > I prefer to keep the consoleWrite() for the arguments length so it will be > clearer if that regress. This is going to move to testharness soon anyway ;) Acknowledged. https://codereview.chromium.org/1576283003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:123: waitForEvent('canplaythrough', function() { On 2016/02/25 11:07:35, Mounir Lamouri wrote: > On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > > For good measure, when depending on canplaythrough, also set preload="auto". > That's the current default, but it's not what Gecko does and it might change. > Simply setting mediaElement.autoplay = true should also work. > > I used preload = "auto". Setting autoplay sounds like a recipe to get race > conditions given that I later call play() ;) Since you wait for the playing event it wouldn't actually be racy, but also a bit too clever so it's just as well to not use autoplay. https://codereview.chromium.org/1576283003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/1576283003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise.html:42: consoleWrite("Promise failed with " + e.name + ": " + e.message); Off-topic: I noticed in another review that DOMException in fact stringifies to exactly what you're using here, and so this could be simply "+ e". But then it seems like this isn't actually defined in any spec, so: https://github.com/heycam/webidl/issues/93 https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt:123: Promise failed with NotSupportedError: Failed to load because no supported source was found. This ought to change to a resolved promise, or the cancelable task fix wasn't enough. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:444: m_playPromiseResolveTask->cancel(); Are you sure that this is needed? I see no other calls to CancellableTaskFactory::cancel() in destructors. CancellableTaskFactory::CancellableTask::run() does nothing if the taskFactory has gone away. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3616: void HTMLMediaElement::scheduleResolvePlayPromises() Can you document here and in the reject counterpart that the early return only works on the assumption that (per spec) there can never be a case where two tasks are in the queue, and a new play promise is added after the first has run but before the second? In other words, it's always safe to ignore the second task. I believe this guarantee should hold, but it's certainly not an obvious thing. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3627: // TODO(mlamouri): because cancellable tasks can't take parameters, the Blank line to match style above? https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:457: // TODO(mlamouri): this is used for cancellabel tasks because we can't pass Typo. Amusing that we have "cancelable" in DOM but CancellableTaskFactory. Let a hundred spellings bloom.
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1576283003/#ps240001 (title: " ")
https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/DOMException.cpp (right): https://codereview.chromium.org/1576283003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/DOMException.cpp:88: // Please add new entries at the end of this list to guarantee any ordering On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > On 2016/02/25 11:07:35, Mounir Lamouri wrote: > > On 2016/02/24 at 09:38:28, philipj_UTC7 wrote: > > > There's a comment at the top ExceptionCode.h that says to keep the lists in > > sync. Can you copy that to the top of this list as well? It's not really that > > the order cannot change, it just has to change in multiple places. Unless > > there's code comparing the ExceptionCode integer values? > > > > Done. I will check what the error was exactly. It might be that someone was > > comparing ExceptionCode. > > I also meant to imply that this comment at the bottom shouldn't be here, on the assumption that it is actually OK to change the order. (I guess you're investigating, just commenting so this warning isn't left without saying why order cannot change.) Removed. I'm going to investigate why that was needed but I don't want to block this CL any more. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt:123: Promise failed with NotSupportedError: Failed to load because no supported source was found. On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > This ought to change to a resolved promise, or the cancelable task fix wasn't enough. That's correct. I have no idea why I ended up not uploading this change even though all the other changes were updated. Probably try to be smart and clean in my local commits :) This is fixed. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:444: m_playPromiseResolveTask->cancel(); On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > Are you sure that this is needed? I see no other calls to CancellableTaskFactory::cancel() in destructors. CancellableTaskFactory::CancellableTask::run() does nothing if the taskFactory has gone away. It's used by FrameView::dispose(). Though, I'm not sure it's needed too. I will remove these given that if it's needed, we will be aware of it sooner than later ;) https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3616: void HTMLMediaElement::scheduleResolvePlayPromises() On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > Can you document here and in the reject counterpart that the early return only works on the assumption that (per spec) there can never be a case where two tasks are in the queue, and a new play promise is added after the first has run but before the second? In other words, it's always safe to ignore the second task. I believe this guarantee should hold, but it's certainly not an obvious thing. Done. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3627: // TODO(mlamouri): because cancellable tasks can't take parameters, the On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > Blank line to match style above? Done. Not sure which style though. https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:457: // TODO(mlamouri): this is used for cancellabel tasks because we can't pass On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > Typo. Amusing that we have "cancelable" in DOM but CancellableTaskFactory. Let a hundred spellings bloom. :)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576283003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576283003/240001
https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1576283003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3627: // TODO(mlamouri): because cancellable tasks can't take parameters, the On 2016/02/25 14:40:06, Mounir Lamouri wrote: > On 2016/02/25 at 14:06:48, philipj_UTC7 wrote: > > Blank line to match style above? > > Done. Not sure which style though. Uh... when viewing the diff between PS10 and PS12 it looked like there was a blank line after the early return in scheduleResolvePlayPromises(), but there wasn't, that line didn't have a line number. Oh well.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576283003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576283003/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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__g3yI... This is also adding a new exception type called NotAllowedError. BUG=579541 ========== to ========== 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__g3yI... This is also adding a new exception type called NotAllowedError. BUG=579541 Committed: https://crrev.com/dd99205e1045321b1812f64c5838e641cda7a2e9 Cr-Commit-Position: refs/heads/master@{#377608} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/dd99205e1045321b1812f64c5838e641cda7a2e9 Cr-Commit-Position: refs/heads/master@{#377608} |
