Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1482393003:
Clear m_autoplaying when starting auto playback. (Closed)
|
Created:
5 years ago by liberato (no reviews please) Modified:
5 years ago Reviewers:
philipj_slow CC:
chromium-reviews, blink-reviews-html_chromium.org, mlamouri+watch-blink_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, blink-reviews, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear m_autoplaying when starting auto playback.
Previously, m_autoplaying wasn't being cleared when auto playback
started. A change in ready state could re-trigger autoplay. For
example, seeking after the media has autoplayed to completion
would begin playback immediately after the seek finished.
BUG=563518
Committed: https://crrev.com/c12dd870a781242cbe6098b3c240cd806ae87740
Cr-Commit-Position: refs/heads/master@{#362731}
Patch Set 1 #
Total comments: 10
Patch Set 2 : cl feedback #
Total comments: 6
Patch Set 3 : cl feedback #
Messages
Total messages: 14 (6 generated)
Description was changed from ========== Clear m_autoplaying when starting auto playback. Previously, m_autoplaying wasn't being cleared when auto playback started. A change in ready state could re-trigger autoplay. For example, seeking after the media has autoplayed to completion would begin playback immediately after the seek finished. BUG=563518 ========== to ========== Clear m_autoplaying when starting auto playback. Previously, m_autoplaying wasn't being cleared when auto playback started. A change in ready state could re-trigger autoplay. For example, seeking after the media has autoplayed to completion would begin playback immediately after the seek finished. BUG=563518 ==========
liberato@chromium.org changed reviewers: + philipj@opera.com
i based the test on media/autoplay.html . it seems to be an instance of an imported w3c test. thanks -fl
https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html (right): https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html:2: <title>seek-stops-autoplay</title> I think a more accurate title would be "autoplay clears the autoplaying flag", since the seek itself isn't what prevents autoplay from happening a second time. If you want to submit the test to web-platform-tests as well, I'd be happy to review+merge there too. Totally optional, though, and we don't yet import those media tests because the need a web server to run :( https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html:8: function seek_stops_autoplay_test(tagName, src) Since you're only calling this once, you could actually just drop it, have async_test as the outermost level and inline the arguments. https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html:21: assert_true(e.paused); Also assert_equals(e.readyState, e.HAVE_ENOUGH_DATA). Otherwise, the media element wouldn't have reached the steps where it would previously autoplay. Make sure that the test reliably fails without your changes so that there's no other issue that would prevent it from catching a regression. https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1997: m_autoplaying = false; This should actually stay, and if removing it doesn't cause any tests to fail, then we have a problem! https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2947: m_autoplaying = false; This is not the place where it's cleared in the spec, right after scheduleEvent(EventTypeNames::playing) in HTMLMediaElement::setReadyState should do it.
https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html (right): https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html:2: <title>seek-stops-autoplay</title> On 2015/12/01 09:45:39, philipj wrote: > I think a more accurate title would be "autoplay clears the autoplaying flag", > since the seek itself isn't what prevents autoplay from happening a second time. > > If you want to submit the test to web-platform-tests as well, I'd be happy to > review+merge there too. Totally optional, though, and we don't yet import those > media tests because the need a web server to run :( done, and yeah i'd like to do send to web platform tests. https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html:8: function seek_stops_autoplay_test(tagName, src) On 2015/12/01 09:45:39, philipj wrote: > Since you're only calling this once, you could actually just drop it, have > async_test as the outermost level and inline the arguments. Done. https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/seek-stops-autoplay.html:21: assert_true(e.paused); On 2015/12/01 09:45:39, philipj wrote: > Also assert_equals(e.readyState, e.HAVE_ENOUGH_DATA). Otherwise, the media > element wouldn't have reached the steps where it would previously autoplay. Make > sure that the test reliably fails without your changes so that there's no other > issue that would prevent it from catching a regression. ran ~250 times before and ~250 after. 100% fail / 100% pass. https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1997: m_autoplaying = false; On 2015/12/01 09:45:39, philipj wrote: > This should actually stay, and if removing it doesn't cause any tests to fail, > then we have a problem! Done. https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1482393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2947: m_autoplaying = false; On 2015/12/01 09:45:39, philipj wrote: > This is not the place where it's cleared in the spec, right after > scheduleEvent(EventTypeNames::playing) in HTMLMediaElement::setReadyState should > do it. Done.
lgtm % nits If you make a PR for web-platform-tests and assign or ping me, I should notice. https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html (right): https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html:2: <title>autoplay-clears-autoplay-flag</title> Please make the title human readable, perhaps "autoplay clears the autoplaying flag" (note the -ing) https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html:10: var e = document.createElement('audio'); You can call it `a` to `audio`, it was previously e as it was either an audio or video element. https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html:24: }, 'audio.autoplay-clears-autoplay-flag'); When there's just one test in a file, with testharness.js you can omit the title here, as it defaults to document.title. Not everyone loves this, but I like that the title is then at the top of the file.
i should have some time tomorrow or Friday to try upstreaming this. thanks -fl https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html (right): https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html:2: <title>autoplay-clears-autoplay-flag</title> On 2015/12/02 09:50:14, philipj wrote: > Please make the title human readable, perhaps "autoplay clears the autoplaying > flag" (note the -ing) done, and renamed test to autoplay-clears-autoplaying-flag.html. left out 'the' in the filename. https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html:10: var e = document.createElement('audio'); On 2015/12/02 09:50:14, philipj wrote: > You can call it `a` to `audio`, it was previously e as it was either an audio or > video element. i will hang this comment up on my wall. done. https://codereview.chromium.org/1482393003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-clears-autoplay-flag.html:24: }, 'audio.autoplay-clears-autoplay-flag'); On 2015/12/02 09:50:14, philipj wrote: > When there's just one test in a file, with testharness.js you can omit the title > here, as it defaults to document.title. Not everyone loves this, but I like that > the title is then at the top of the file. Done.
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/1482393003/#ps40001 (title: "cl feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1482393003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1482393003/40001
Message was sent while issue was closed.
Description was changed from ========== Clear m_autoplaying when starting auto playback. Previously, m_autoplaying wasn't being cleared when auto playback started. A change in ready state could re-trigger autoplay. For example, seeking after the media has autoplayed to completion would begin playback immediately after the seek finished. BUG=563518 ========== to ========== Clear m_autoplaying when starting auto playback. Previously, m_autoplaying wasn't being cleared when auto playback started. A change in ready state could re-trigger autoplay. For example, seeking after the media has autoplayed to completion would begin playback immediately after the seek finished. BUG=563518 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Clear m_autoplaying when starting auto playback. Previously, m_autoplaying wasn't being cleared when auto playback started. A change in ready state could re-trigger autoplay. For example, seeking after the media has autoplayed to completion would begin playback immediately after the seek finished. BUG=563518 ========== to ========== Clear m_autoplaying when starting auto playback. Previously, m_autoplaying wasn't being cleared when auto playback started. A change in ready state could re-trigger autoplay. For example, seeking after the media has autoplayed to completion would begin playback immediately after the seek finished. BUG=563518 Committed: https://crrev.com/c12dd870a781242cbe6098b3c240cd806ae87740 Cr-Commit-Position: refs/heads/master@{#362731} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c12dd870a781242cbe6098b3c240cd806ae87740 Cr-Commit-Position: refs/heads/master@{#362731} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
