Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2047433003:
Update autoplay settings when moving media elements between documents (Closed)
|
Created:
4 years, 6 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 6 months ago Reviewers:
mlamouri (slow - plz ping) CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate autoplay settings when moving media elements between documents
BUG=617098
Committed: https://crrev.com/21d18f7782b3105e0b3efe16ae5e07f450cf2d33
Cr-Commit-Position: refs/heads/master@{#398852}
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed comments, added layout tests #
Total comments: 6
Patch Set 3 : fixed layout tests #
Total comments: 4
Patch Set 4 : fixed nits in tests #
Messages
Total messages: 19 (7 generated)
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/2047433003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2047433003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:493: || m_autoplayHelper->isExperimentEnabled()) { I think you should do something like: ``` bool oldDocumentRequiresUserGesture = <what you did but for the old document> bool newDocumentRequiresUserGesture = <what you did> if (newDocumentRequiresUserGesture && !oldDocumentRequiresUserGesture) m_lockedPendingUserGesture = true; ``` I'm afraid what you wrote here would re-set to true the lock after the document move regardless of the context.
PTAL. Addressed comments and added layout tests. https://codereview.chromium.org/2047433003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2047433003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:493: || m_autoplayHelper->isExperimentEnabled()) { On 2016/06/07 15:40:28, Mounir Lamouri (slow) wrote: > I think you should do something like: > ``` > bool oldDocumentRequiresUserGesture = <what you did but for the old document> > bool newDocumentRequiresUserGesture = <what you did> > if (newDocumentRequiresUserGesture && !oldDocumentRequiresUserGesture) > m_lockedPendingUserGesture = true; > ``` > > I'm afraid what you wrote here would re-set to true the lock after the document > move regardless of the context. Done.
Thanks! look good with a couple of changes to the test. Nice idea to have a virtual test suite :) https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html (right): https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html:10: test(function() { You should use a async_test here. https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html:16: v.play().then( You will have to use t.test_func_done() to wrap the "function(e) {" here. https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html:21: console.warn("caught expected exception"); I think it's wrong to do console.warn() on testharness.js tests and assume that it will show and the file comparison will make the test pass or fail. You should have reall assert_*. Maybe you could use window.internals to check if the flag is on/off? I know it makes things a bit less nice but I think it's slightly better :)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
PTAL. Fixed tests. https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html (right): https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html:10: test(function() { On 2016/06/07 19:59:01, Mounir Lamouri (slow) wrote: > You should use a async_test here. Done. https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html:16: v.play().then( On 2016/06/07 19:59:01, Mounir Lamouri (slow) wrote: > You will have to use t.test_func_done() to wrap the "function(e) {" here. Done. https://codereview.chromium.org/2047433003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay/autoplay-document-move.html:21: console.warn("caught expected exception"); On 2016/06/07 19:59:02, Mounir Lamouri (slow) wrote: > I think it's wrong to do console.warn() on testharness.js tests and assume that > it will show and the file comparison will make the test pass or fail. You should > have reall assert_*. Maybe you could use window.internals to check if the flag > is on/off? I know it makes things a bit less nice but I think it's slightly > better :) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047433003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with change to test file https://codereview.chromium.org/2047433003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-document-move.html (right): https://codereview.chromium.org/2047433003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-document-move.html:25: function(e) { v.play().then( t.step_func_done, t.step_func_done(function() { assert_unreached(...); }) ); https://codereview.chromium.org/2047433003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-document-move.html:46: }); ditto
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2047433003/#ps80001 (title: "fixed nits in tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047433003/80001
https://codereview.chromium.org/2047433003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-document-move.html (right): https://codereview.chromium.org/2047433003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-document-move.html:25: function(e) { On 2016/06/09 10:57:58, Mounir Lamouri (slow) wrote: > v.play().then( > t.step_func_done, > t.step_func_done(function() { assert_unreached(...); }) > ); Done. https://codereview.chromium.org/2047433003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-document-move.html:46: }); On 2016/06/09 10:57:58, Mounir Lamouri (slow) wrote: > ditto Done.
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Update autoplay settings when moving media elements between documents BUG=617098 ========== to ========== Update autoplay settings when moving media elements between documents BUG=617098 Committed: https://crrev.com/21d18f7782b3105e0b3efe16ae5e07f450cf2d33 Cr-Commit-Position: refs/heads/master@{#398852} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/21d18f7782b3105e0b3efe16ae5e07f450cf2d33 Cr-Commit-Position: refs/heads/master@{#398852} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
