Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1293253003:
Fix autoplay during prerendering for media elements on Android. (Closed)
|
Created:
5 years, 4 months ago by DaleCurtis Modified:
5 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, nasko+codewatch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org, mlamouri+watch-media_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@defer_media Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix autoplay during prerendering for media elements on Android.
If the user gesture for autoplay is disabled manually or via an
experiment, prerendered media will play in a hidden frame.
Within WebMediaPlayerImpl this behavior is deferred at load time
via a callback, however WebMediaPlayerAndroid was never given
this callback, so it would try to play media. Generally this
fails because by default autoplay requires a user gesture.
The correct fix is to refactor WMPA such that it uses a params
structure like WMPI and can invoke the deferral callback when
load() is called.
BUG=522157
TEST=clank no longer plays hidden a/v when user gesture disabled.
Committed: https://crrev.com/98fc72cda10bf328ad55e460f6a7b06a356d5b3d
Cr-Commit-Position: refs/heads/master@{#344676}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Comments. #
Total comments: 2
Patch Set 3 : Comments. #Patch Set 4 : Rebase. #Patch Set 5 : Rename test class. #
Messages
Total messages: 28 (10 generated)
dalecurtis@chromium.org changed reviewers: + creis@chromium.org, qinmin@chromium.org
creis for content/ qinmin for media/ https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... chrome/browser/media/defer_background_media_browsertest.cc:16: class MediaBrowserTest : public InProcessBrowserTest { I can't actually compile the chrome browser tests on Android, so I assume these aren't running anywhere today -- but I've fixed it up such that it could run at least.
I'm not very familiar with this code, in case you want to find someone more knowledgable with it. It looks like a reasonable change to me, though, so I'm happy to rubber stamp it after the minor questions below. https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... content/renderer/media/android/webmediaplayer_android.cc:274: DVLOG(1) << __FUNCTION__ << "(" << load_type << ", " << url << ", " Is this log useful in a particular case? https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... content/renderer/media/android/webmediaplayer_android.h:360: media::WebMediaPlayerParams::DeferLoadCB defer_load_cb_; Maybe add a comment using the word "callback," to help clarify what this means? https://codereview.chromium.org/1293253003/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1293253003/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2039: media::WebMediaPlayerParams::Context3DCB context_3d_cb; Do these default constructors do the right thing on Android?
https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... chrome/browser/media/defer_background_media_browsertest.cc:16: class MediaBrowserTest : public InProcessBrowserTest { On 2015/08/18 23:14:42, DaleCurtis wrote: > I can't actually compile the chrome browser tests on Android, so I assume these > aren't running anywhere today -- but I've fixed it up such that it could run at > least. android dosn't run browser_tests, it runs chrome_test_apk or chrome_public_test_apk
https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... chrome/browser/media/defer_background_media_browsertest.cc:16: class MediaBrowserTest : public InProcessBrowserTest { On 2015/08/19 21:31:14, qinmin(OOO_till_8-19) wrote: > On 2015/08/18 23:14:42, DaleCurtis wrote: > > I can't actually compile the chrome browser tests on Android, so I assume > these > > aren't running anywhere today -- but I've fixed it up such that it could run > at > > least. > > android dosn't run browser_tests, it runs chrome_test_apk or > chrome_public_test_apk Hmm, chrome_test_apk is not a target and I can't find any variations of that either. chrome_public_test_apk is a target, but I don't see any way of invoking that. Do you have a wiki doc on this anywhere?
https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... chrome/browser/media/defer_background_media_browsertest.cc:16: class MediaBrowserTest : public InProcessBrowserTest { On 2015/08/20 17:59:22, DaleCurtis wrote: > On 2015/08/19 21:31:14, qinmin(OOO_till_8-19) wrote: > > On 2015/08/18 23:14:42, DaleCurtis wrote: > > > I can't actually compile the chrome browser tests on Android, so I assume > > these > > > aren't running anywhere today -- but I've fixed it up such that it could run > > at > > > least. > > > > android dosn't run browser_tests, it runs chrome_test_apk or > > chrome_public_test_apk > > Hmm, chrome_test_apk is not a target and I can't find any variations of that > either. chrome_public_test_apk is a target, but I don't see any way of invoking > that. Do you have a wiki doc on this anywhere? And right after I posted this, I found the right article: https://code.google.com/p/chromium/wiki/AndroidTestInstructions
Thanks Charlie, your review is sufficient. I know the WMPI code well and qinmin@ knows the WMPA code well, so we should have all our bases covered :) https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1293253003/diff/1/chrome/browser/media/defer_... chrome/browser/media/defer_background_media_browsertest.cc:16: class MediaBrowserTest : public InProcessBrowserTest { On 2015/08/20 18:00:24, DaleCurtis wrote: > On 2015/08/20 17:59:22, DaleCurtis wrote: > > On 2015/08/19 21:31:14, qinmin(OOO_till_8-19) wrote: > > > On 2015/08/18 23:14:42, DaleCurtis wrote: > > > > I can't actually compile the chrome browser tests on Android, so I assume > > > these > > > > aren't running anywhere today -- but I've fixed it up such that it could > run > > > at > > > > least. > > > > > > android dosn't run browser_tests, it runs chrome_test_apk or > > > chrome_public_test_apk > > > > Hmm, chrome_test_apk is not a target and I can't find any variations of that > > either. chrome_public_test_apk is a target, but I don't see any way of > invoking > > that. Do you have a wiki doc on this anywhere? > > And right after I posted this, I found the right article: > > https://code.google.com/p/chromium/wiki/AndroidTestInstructions Doesn't look like those targets include any type of browser tests, so this fix is kind of moot, but I'll leave it in case we ever start running them on Android. https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... content/renderer/media/android/webmediaplayer_android.cc:274: DVLOG(1) << __FUNCTION__ << "(" << load_type << ", " << url << ", " On 2015/08/18 23:57:42, Charlie Reis wrote: > Is this log useful in a particular case? Done. https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/1293253003/diff/1/content/renderer/media/andr... content/renderer/media/android/webmediaplayer_android.h:360: media::WebMediaPlayerParams::DeferLoadCB defer_load_cb_; On 2015/08/18 23:57:42, Charlie Reis wrote: > Maybe add a comment using the word "callback," to help clarify what this means? Done. https://codereview.chromium.org/1293253003/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1293253003/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2039: media::WebMediaPlayerParams::Context3DCB context_3d_cb; On 2015/08/18 23:57:42, Charlie Reis wrote: > Do these default constructors do the right thing on Android? The first will just be nullptr, the second an unbound callback. They're unused on Android in any case.
Thanks, LGTM.
lgtm % comment https://codereview.chromium.org/1293253003/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1293253003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:116: #include "media/blink/webmediaplayer_params.h" this can be removed now
Thanks for review! https://codereview.chromium.org/1293253003/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1293253003/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:116: #include "media/blink/webmediaplayer_params.h" On 2015/08/20 19:47:25, qinmin wrote: > this can be removed now Done.
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1293253003/#ps40001 (title: "Comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1293253003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1293253003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1293253003/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1293253003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1293253003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/98fc72cda10bf328ad55e460f6a7b06a356d5b3d Cr-Commit-Position: refs/heads/master@{#344676}
Message was sent while issue was closed.
stgao@chromium.org changed reviewers: + stgao@chromium.org
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1305973002/ by yukishiino@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg... This CL seems redefining MediaBrowserTest, which is already defined in chrome/browser/media/media_browsertest.h and causing build breakage..
Message was sent while issue was closed.
This CL broke compile on http://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg%... Reverting this CL passed the try-job: https://codereview.chromium.org/1304053002/ Reverting other CLs in the range still failed the try-job, eg.: https://codereview.chromium.org/1306773002/ https://codereview.chromium.org/1305963002/
Sorry about the ODR violation, renamed the test class and am resubmitting. Thanks!
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1293253003/#ps80001 (title: "Rename test class.")
The CQ bit was unchecked by dalecurtis@chromium.org
Actually, looks like the CQ can't handle resubmits of this change, so I'll fork it into a new one. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
