Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 4 months ago by DaleCurtis Modified:
5 years, 1 month ago Reviewers:
Lei Zhang, gunsch, tommycli, soniahibberd337, Charlie Reis, mmenke CC:
chromium-reviews, ncarter (slow) Base URL:
http://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefer media playback in background tabs.
Videos which autoplay in the background will now have their load
deferred until the tab is visible for the first time -- this avoids
autoplay during session restore and premature playback.
Once a tab / RenderFrame has ever played media before, it's allowed
to continue to autoplay/autoload indefinitely; this is to support
playlist type applications.
BUG=509135
TEST=manual verification that background tabs don't autoplay;
playlist type media continues to work.
Committed: https://crrev.com/0f9e7f4c43221c205982328021138a7ea54261ef
Cr-Commit-Position: refs/heads/master@{#344092}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add sticky playback bit. #
Total comments: 5
Patch Set 3 : Remove prerender loader. #Patch Set 4 : Add test. #
Total comments: 11
Patch Set 5 : Fix build.gn #
Total comments: 4
Patch Set 6 : Comments. #
Created: 5 years, 4 months ago
Dependent Patchsets: Messages
Total messages: 47 (11 generated)
dalecurtis@chromium.org changed reviewers: + thestig@chromium.org, tommycli@chromium.org
I thought about compentizing https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/rendere... but since we'll want an idle handler in there and the code is so small, the overhead of componentization seems unnecessary. I couldn't find a good way to add a test for this either, the functionality required to set Visible/Hidden states is not exposed to chrome/ -- in content/ you can fake messages to the widget host, but chrome/ doesn't know about those internal content/ details. Notably, this only fixes deferred load of background tabs, not the foreground tab if it has any video playing.
The CQ bit was checked by dalecurtis@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/1292433002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292433002/1
On 2015/08/12 22:26:26, DaleCurtis wrote: > I thought about compentizing > https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/rendere... > but since we'll want an idle handler in there and the code is so small, the > overhead of componentization seems unnecessary. > > I couldn't find a good way to add a test for this either, the functionality > required to set Visible/Hidden states is not exposed to chrome/ -- in content/ > you can fake messages to the widget host, but chrome/ doesn't know about those > internal content/ details. > > Notably, this only fixes deferred load of background tabs, not the foreground > tab if it has any video playing. Just realized we want a sticky play bit or this will break playlists. So please hold off on review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1292433002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:723: if (!render_frame->IsHidden()) { nit, how about we structure this block consistently: if (cond1) { new Foo; return; } if (cond2) { new Bar; return; } closure.Run();
dalecurtis@chromium.org changed reviewers: + ncarter@chromium.org
dalecurtis@chromium.org changed reviewers: + gunsch@chromium.org
+nick for content/ changes. +gunsch for chromecast/ changes. The change now includes a sticky-play bit per RenderFrame which is set using the WebMediaPlayerDelgate information. https://codereview.chromium.org/1292433002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:723: if (!render_frame->IsHidden()) { On 2015/08/13 19:11:47, Lei Zhang wrote: > nit, how about we structure this block consistently: > > if (cond1) { > new Foo; > return; > } > > if (cond2) { > new Bar; > return; > } > > closure.Run(); Done.
Also if anyone knows of a good way to write a chrome/ test which can simulate WasHidden/WasVisible, that'd be awesome...
Really cool approach! https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:712: // Chromium for Android doesn't support prerender yet. Although we don't have prerender on Android, it seems like the media deferral should apply to Android also. https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:716: if (prerender::PrerenderHelper::IsPrerendering(render_frame)) { Does this mean that pages that are pre-rendered are exempt from the MediaLoadDeferrer? Or is it chained somehow?
On 2015/08/13 21:45:42, DaleCurtis wrote: > Also if anyone knows of a good way to write a chrome/ test which can simulate > WasHidden/WasVisible, that'd be awesome... Maybe a browser_test using ui_test_utils::NavigateToURL() with NEW_BACKGROUND_TAB?
https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:712: // Chromium for Android doesn't support prerender yet. On 2015/08/13 21:50:07, tommycli wrote: > Although we don't have prerender on Android, it seems like the media deferral > should apply to Android also. Android doesn't use WebMediaPlayerImpl and ignores the provided deferral callback -- so this approach won't work currently for Android. However, since autoplay requires a user gesture there, it's also not important. My team is pursuing a project right now to move Android over to WebMediaPlayerImpl, at that point it would benefit from this code. Since it doesn't matter and this code is never called, what I'll do is move the #if around such that only prerender is excluded. https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:716: if (prerender::PrerenderHelper::IsPrerendering(render_frame)) { On 2015/08/13 21:50:06, tommycli wrote: > Does this mean that pages that are pre-rendered are exempt from the > MediaLoadDeferrer? Or is it chained somehow? Hmm, I was counting on prerender not taking affect for invislbe pages, but it is possible to do the following to trigger playback in a background tab: <link rel="prerender" href="buck2.webm"> <meta http-equiv="refresh" content="2; URL='buck2.webm'"/> I wonder if it's worth chaining or if we can actually just remove the IsPrerenderingPath() completely. Locally that works fine because a prerender frame is hidden, +mmenke for commentary to see if we can just delete the PrerenderMediaLoadDeferrer().
dalecurtis@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for real this time, see previous comment.
On 2015/08/13 21:52:00, Lei Zhang wrote: > On 2015/08/13 21:45:42, DaleCurtis wrote: > > Also if anyone knows of a good way to write a chrome/ test which can simulate > > WasHidden/WasVisible, that'd be awesome... > > Maybe a browser_test using ui_test_utils::NavigateToURL() with > NEW_BACKGROUND_TAB? To followup on the browser test issue. Yes, what Lei said would work. In fact, a PluginPowerSaverBrowserTest does something like that: https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/pl...
https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:716: if (prerender::PrerenderHelper::IsPrerendering(render_frame)) { On 2015/08/13 22:24:30, DaleCurtis wrote: > On 2015/08/13 21:50:06, tommycli wrote: > > Does this mean that pages that are pre-rendered are exempt from the > > MediaLoadDeferrer? Or is it chained somehow? > > Hmm, I was counting on prerender not taking affect for invislbe pages, but it is > possible to do the following to trigger playback in a background tab: > > <link rel="prerender" href="buck2.webm"> > <meta http-equiv="refresh" content="2; URL='buck2.webm'"/> > > I wonder if it's worth chaining or if we can actually just remove the > IsPrerenderingPath() completely. Locally that works fine because a prerender > frame is hidden, +mmenke for commentary to see if we can just delete the > PrerenderMediaLoadDeferrer(). Prerendered frames should always be hidden. I don't know much about visibility issues, but the new code looks to me like it should cover the prerender case, and if it does, the old prerender code here can probably be removed - which is great. The less special case code prerender has, the better. Just want to make sure we have tests to protect against regressions, if the new behavior is ever changes (Which we should already have)
Awesome, I'll go ahead and remove the code and work on some tests for this behavior. One last thing, tying the play state to RenderFrame like I have means it survives navigation, which is unintended. I'm looking for a good place to clear it now. Suggestions welcome :) ...including suggestions of a better storage area (a la something like WebContentsUserData like we have on the browser side).
Actually after chatting with YouTube, they need playback to survive navigation for playlists since there's a limit to the number of "plays" before a navigation is forced. Latest patch set removes the prerender code. Tests coming shortly.
rs lgtm for chromecast/
Okay, everything should be good to go. I've added a browser test for the current behavior as best I can. Testing the absence of playback is pretty hard it turns out :|
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:5: #include "base/strings/stringprintf.h" not used https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. Add bug # if there is one https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:28: content::WebContents* background_contents = Also get the current tab via GetActiveWebContents() before opening a new one and check that != background_contents as a sanity check? https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:58: #endif // !defined(OS_ANDROID)
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. Does this mean if we prerender on android, we'll play any audio?
lgtm awesome
I'm OOO for a couple days, so I'll wait to address feedback until nick@ comments, since I'm not 100% confident about the ins and outs of tying values to the RenderFrame. https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. On 2015/08/14 14:57:35, mmenke wrote: > Does this mean if we prerender on android, we'll play any audio? The DeferMediaLoad() call is unused on Android (see the dropped callback in the WebMediaPlayerAndroid constructor) so if you were to enable prerendering on Android you'd need to fix that -- though for user who haven't disabled it, the autoplay requirement of a user gesture prevents playback anyways.
dalecurtis@chromium.org changed reviewers: + creis@chromium.org - ncarter@chromium.org
Swapping the OOO ncarter1@ for creis@.
Just a minor question and nit. I do think Nick would be more familiar with this code than me, but I don't see a big problem with the approach since RenderFrame seems to know a lot about media already. https://codereview.chromium.org/1292433002/diff/80001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1292433002/diff/80001/content/public/renderer... content/public/renderer/content_renderer_client.h:156: bool render_frame_has_played_media_before, Maybe |has_played_media_before|? The render frame part seems implicit from the first parameter, and you can tie it together with a comment above. https://codereview.chromium.org/1292433002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1292433002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:1017: bool has_played_media_; Is this something that can live in one of the various media-related classes above, or does it really need to be on RenderFrame? (If it does stay, we should list it close to those members and not down here at the bottom.)
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. On 2015/08/14 17:37:35, DaleCurtis wrote: > On 2015/08/14 14:57:35, mmenke wrote: > > Does this mean if we prerender on android, we'll play any audio? > > The DeferMediaLoad() call is unused on Android (see the dropped callback in the > WebMediaPlayerAndroid constructor) so if you were to enable prerendering on > Android you'd need to fix that -- though for user who haven't disabled it, the > autoplay requirement of a user gesture prevents playback anyways. I believe prerendering is enabled on Android, or at least some stuff directly uses the prerendering mechanism - see, for example, chrome/browser/android/omnibox/omnibox_prerender.h.
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:5: #include "base/strings/stringprintf.h" On 2015/08/14 02:21:18, Lei Zhang wrote: > not used Done. https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. On 2015/08/18 17:36:04, mmenke wrote: > On 2015/08/14 17:37:35, DaleCurtis wrote: > > On 2015/08/14 14:57:35, mmenke wrote: > > > Does this mean if we prerender on android, we'll play any audio? > > > > The DeferMediaLoad() call is unused on Android (see the dropped callback in > the > > WebMediaPlayerAndroid constructor) so if you were to enable prerendering on > > Android you'd need to fix that -- though for user who haven't disabled it, the > > autoplay requirement of a user gesture prevents playback anyways. > > I believe prerendering is enabled on Android, or at least some stuff directly > uses the prerendering mechanism - see, for example, > chrome/browser/android/omnibox/omnibox_prerender.h. Hmm, that's broken for Media Source on Android then and only working because we require a user gesture for media playback. For src= playback it looks like the Android MediaPlayer path does seem to try and check for prerendering via an alternate mechanism: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... However that check does not work, any user which flag flips the user gesture requirement to off is getting autoplay on prerendered pages. I just confirmed this by creating a prerender test page and navigating to http://commondatastorage.googleapis.com/dalecurtis-shared/prerender.html Filed http://crbug.com/522157 -- which I'll try to fix after landing this change. https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:28: content::WebContents* background_contents = On 2015/08/14 02:21:18, Lei Zhang wrote: > Also get the current tab via GetActiveWebContents() before opening a new one and > check that != background_contents as a sanity check? Done. https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/de... chrome/browser/media/defer_background_media_browsertest.cc:58: #endif On 2015/08/14 02:21:18, Lei Zhang wrote: > // !defined(OS_ANDROID) Done. https://codereview.chromium.org/1292433002/diff/80001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/1292433002/diff/80001/content/public/renderer... content/public/renderer/content_renderer_client.h:156: bool render_frame_has_played_media_before, On 2015/08/18 17:30:03, Charlie Reis wrote: > Maybe |has_played_media_before|? The render frame part seems implicit from the > first parameter, and you can tie it together with a comment above. Done. https://codereview.chromium.org/1292433002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1292433002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:1017: bool has_played_media_; On 2015/08/18 17:30:03, Charlie Reis wrote: > Is this something that can live in one of the various media-related classes > above, or does it really need to be on RenderFrame? (If it does stay, we should > list it close to those members and not down here at the bottom.) None of those are any better of a fit unfortunately, so I've just moved this value up to the other media pieces. One day if we tease out media::WebMediaPlayerDelegate from RenderFrameImpl it could be added there.
Thanks. content/ LGTM.
mmenke: lgty for now? This isn't breaking anything that isn't already broken on Android.
On 2015/08/18 22:16:42, DaleCurtis wrote: > mmenke: lgty for now? This isn't breaking anything that isn't already broken on > Android. If it's already broken, no need to block this CL on it. prerender/ LGTM.
Thanks for review everyone!
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, gunsch@chromium.org, tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/1292433002/#ps100001 (title: "Comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292433002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292433002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0f9e7f4c43221c205982328021138a7ea54261ef Cr-Commit-Position: refs/heads/master@{#344092}
Message was sent while issue was closed.
soniahibberd337@gmail.com changed reviewers: + soniahibberd337@gmail.com
Message was sent while issue was closed.
soniahibberd337@gmail.com changed reviewers: + soniahibberd337@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
Message was sent while issue was closed.
Youtube Playlists and Google Play Music are broken... This needs a whitelist!!! I am forced to use Chrome if I want to use Google Play Music without staring at the player without using my computer for anything else... Come on
Message was sent while issue was closed.
On 2015/11/05 17:06:28, danituentimsn wrote: > Youtube Playlists and Google Play Music are broken... This needs a whitelist!!! > I am forced to use Chrome if I want to use Google Play Music without staring at > the player without using my computer for anything else... Come on Have you filed a bug on crbug.com ? Let's take the conversation there. |
