Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2199363002:
Change play promises behaviour in the load algorithm. (Closed)
|
Created:
4 years, 4 months ago by mlamouri (slow - plz ping) Modified:
4 years, 4 months ago Reviewers:
foolip 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. |
DescriptionChange play promises behaviour in the load algorithm.
This is implementing the changes from the following whatwg/html PR:
https://github.com/whatwg/html/pull/1621
BUG=592158, 630622
R=foolip@chromium.org
Committed: https://crrev.com/e2c683031c305abcb6dab162832f95fa9b71545e
Cr-Commit-Position: refs/heads/master@{#409793}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review comments #
Total comments: 10
Patch Set 3 : nits + ordering tests #Patch Set 4 : make it match the spec #
Total comments: 4
Patch Set 5 : comments #
Messages
Total messages: 28 (17 generated)
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/v2/patch-status/codereview.chromium.or...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:818: UseCounter::count(document(), UseCounter::HTMLMediaElementLoadNetworkEmptyNotPaused); Can you remove this counter (also from UseCounter.h) since we've concluded that option isn't going to work out? https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1405: if (m_playPromiseResolveTask->isPending()) { Can you update the documentation of HTMLMediaElement::invokeLoadAlgorithm to match the spec change and if it then looks more natural, move this out to where the documentation is? At least the resolveScheduledPlayPromises() and rejectScheduledPlayPromises() calls I guess would end up outside this method. Then there's the question of order that came up in the spec review. We already have the task order problem documented in the TODO in HTMLMediaElement::scheduleResolvePlayPromises, so I think it's OK if we can't exactly match the spec, but we should write the spec in a way that we think will at least be possible later to implement. How would things have to change for "in the order that the corresponding tasks were queued" to be implementable, or do you think the spec should say something different?
PTAL https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:818: UseCounter::count(document(), UseCounter::HTMLMediaElementLoadNetworkEmptyNotPaused); On 2016/08/03 at 14:32:56, foolip wrote: > Can you remove this counter (also from UseCounter.h) since we've concluded that option isn't going to work out? Sounds good but I would prefer to do that in a follow-up because it would require metrics changes (histograms.xml) and it's not quite related to this change. https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1405: if (m_playPromiseResolveTask->isPending()) { On 2016/08/03 at 14:32:56, foolip wrote: > Can you update the documentation of HTMLMediaElement::invokeLoadAlgorithm to match the spec change and if it then looks more natural, move this out to where the documentation is? At least the resolveScheduledPlayPromises() and rejectScheduledPlayPromises() calls I guess would end up outside this method. > > Then there's the question of order that came up in the spec review. We already have the task order problem documented in the TODO in HTMLMediaElement::scheduleResolvePlayPromises, so I think it's OK if we can't exactly match the spec, but we should write the spec in a way that we think will at least be possible later to implement. > > How would things have to change for "in the order that the corresponding tasks were queued" to be implementable, or do you think the spec should say something different? 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/v2/patch-status/codereview.chromium.or...
lgtm % nits, feel free to land once the spec change is merged and any final changes from that are reflected here. https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:818: UseCounter::count(document(), UseCounter::HTMLMediaElementLoadNetworkEmptyNotPaused); On 2016/08/03 15:57:55, Mounir Lamouri wrote: > On 2016/08/03 at 14:32:56, foolip wrote: > > Can you remove this counter (also from UseCounter.h) since we've concluded > that option isn't going to work out? > > Sounds good but I would prefer to do that in a follow-up because it would > require metrics changes (histograms.xml) and it's not quite related to this > change. Acknowledged. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:242: assert_false(audio.paused); Ahem, I neglected to read the tests the first time around. I think this assert needs to be inside the setTimeout to show that this is testing what it claims to. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:769: // resolve or reject those promises in the order the corresponding tasks Write a TODO about the order or change the spec if it's unlikely to ever happen? https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:796: rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request."); If this moves down in the spec, I guess the else branch can also DCHECK that there are no pending play promises.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I've added ordering tests. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:242: assert_false(audio.paused); On 2016/08/03 at 16:31:51, foolip wrote: > Ahem, I neglected to read the tests the first time around. I think this assert needs to be inside the setTimeout to show that this is testing what it claims to. Done. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:769: // resolve or reject those promises in the order the corresponding tasks On 2016/08/03 at 16:31:51, foolip wrote: > Write a TODO about the order or change the spec if it's unlikely to ever happen? I'm not sure what you mean. This is in order, we append the promises. I will add a test to make this clear. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:796: rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request."); On 2016/08/03 at 16:31:51, foolip wrote: > If this moves down in the spec, I guess the else branch can also DCHECK that there are no pending play promises. I guess you are referring to the spec that says if the paused attribute is false? We don't have this branch. I will keep this comment pending until we decide what to do on the spec side.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mlamouri@chromium.org
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2199363002/#ps60001 (title: "make it match the spec")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Feel free to let this continue through CQ and send a follow-up (or not, if I'm wrong.) https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:769: // resolve or reject those promises in the order the corresponding tasks On 2016/08/04 11:16:16, Mounir Lamouri wrote: > On 2016/08/03 at 16:31:51, foolip wrote: > > Write a TODO about the order or change the spec if it's unlikely to ever > happen? > > I'm not sure what you mean. This is in order, we append the promises. I will add > a test to make this clear. New tests LGTM. The tricky case I was thinking about is if there are both pending resolves and rejections interleaved in the task queue. When "readyState attribute has the value HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA" it should be possible to synchronously enqueue tasks to resolve by calling play(), and synchronously enqueue tasks to reject by calling pause(). So a play();pause();play();pause() series of calls? Ideally, we should of course change both play() and pause() in the spec to only resolve play() promises once really playing (i.e. never sync) in which case the problem would go away. So the priority of fixing such a TODO would be very low. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:796: rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request."); On 2016/08/04 11:16:16, Mounir Lamouri wrote: > On 2016/08/03 at 16:31:51, foolip wrote: > > If this moves down in the spec, I guess the else branch can also DCHECK that > there are no pending play promises. > > I guess you are referring to the spec that says if the paused attribute is > false? We don't have this branch. I will keep this comment pending until we > decide what to do on the spec side. Yeah, I mean you could DCHECK something in an else branch below, if you think it could catch problems. https://codereview.chromium.org/2199363002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2199363002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:241: setTimeout(function() { This needs t.step_func so that the right test fails if the assert fails. (This is sometimes nicer when using promise_test.) https://codereview.chromium.org/2199363002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:310: setTimeout(function() { t.step_func wrapper
The CQ bit was unchecked by mlamouri@chromium.org
The CQ bit was checked by mlamouri@chromium.org
https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:769: // resolve or reject those promises in the order the corresponding tasks On 2016/08/04 at 13:47:37, foolip wrote: > On 2016/08/04 11:16:16, Mounir Lamouri wrote: > > On 2016/08/03 at 16:31:51, foolip wrote: > > > Write a TODO about the order or change the spec if it's unlikely to ever > > happen? > > > > I'm not sure what you mean. This is in order, we append the promises. I will add > > a test to make this clear. > > New tests LGTM. The tricky case I was thinking about is if there are both pending resolves and rejections interleaved in the task queue. When "readyState attribute has the value HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA" it should be possible to synchronously enqueue tasks to resolve by calling play(), and synchronously enqueue tasks to reject by calling pause(). So a play();pause();play();pause() series of calls? > > Ideally, we should of course change both play() and pause() in the spec to only resolve play() promises once really playing (i.e. never sync) in which case the problem would go away. So the priority of fixing such a TODO would be very low. Added TODO. https://codereview.chromium.org/2199363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:796: rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request."); On 2016/08/04 at 13:47:37, foolip wrote: > On 2016/08/04 11:16:16, Mounir Lamouri wrote: > > On 2016/08/03 at 16:31:51, foolip wrote: > > > If this moves down in the spec, I guess the else branch can also DCHECK that > > there are no pending play promises. > > > > I guess you are referring to the spec that says if the paused attribute is > > false? We don't have this branch. I will keep this comment pending until we > > decide what to do on the spec side. > > Yeah, I mean you could DCHECK something in an else branch below, if you think it could catch problems. Added a DCHECK. https://codereview.chromium.org/2199363002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-play-promise.html (right): https://codereview.chromium.org/2199363002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:241: setTimeout(function() { On 2016/08/04 at 13:47:37, foolip wrote: > This needs t.step_func so that the right test fails if the assert fails. (This is sometimes nicer when using promise_test.) Done. https://codereview.chromium.org/2199363002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-play-promise.html:310: setTimeout(function() { On 2016/08/04 at 13:47:37, foolip wrote: > t.step_func wrapper Done.
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2199363002/#ps80001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Change play promises behaviour in the load algorithm. This is implementing the changes from the following whatwg/html PR: https://github.com/whatwg/html/pull/1621 BUG=592158,630622 R=foolip@chromium.org ========== to ========== Change play promises behaviour in the load algorithm. This is implementing the changes from the following whatwg/html PR: https://github.com/whatwg/html/pull/1621 BUG=592158,630622 R=foolip@chromium.org Committed: https://crrev.com/e2c683031c305abcb6dab162832f95fa9b71545e Cr-Commit-Position: refs/heads/master@{#409793} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e2c683031c305abcb6dab162832f95fa9b71545e Cr-Commit-Position: refs/heads/master@{#409793} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
