Defer media playback in background tabs.

Defer 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}

[DaleCurtis, 2015-08-12 22:26:26]

dalecurtis@chromium.org changed reviewers:
+ thestig@chromium.org, tommycli@chromium.org

[DaleCurtis, 2015-08-12 22:26:26]

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.

[DaleCurtis, 2015-08-13 01:27:23]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 01:27:34]

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

[DaleCurtis, 2015-08-13 01:40:10]

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.

[commit-bot: I haz the power, 2015-08-13 02:38:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 02:38:08]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-08-13 19:11:47]

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, 2015-08-13 21:41:44]

dalecurtis@chromium.org changed reviewers:
+ ncarter@chromium.org

[DaleCurtis, 2015-08-13 21:44:47]

dalecurtis@chromium.org changed reviewers:
+ gunsch@chromium.org

[DaleCurtis, 2015-08-13 21:44:48]

+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.

[DaleCurtis, 2015-08-13 21:45:42]

Also if anyone knows of a good way to write a chrome/ test which can simulate
WasHidden/WasVisible, that'd be awesome...

[tommycli, 2015-08-13 21:50:07]

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?

[Lei Zhang, 2015-08-13 21:52:00]

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?

[DaleCurtis, 2015-08-13 22:24:30]

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, 2015-08-13 22:24:48]

dalecurtis@chromium.org changed reviewers:
+ mmenke@chromium.org

[DaleCurtis, 2015-08-13 22:24:48]

+mmenke for real this time, see previous comment.

[tommycli, 2015-08-13 22:30:17]

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...

[mmenke, 2015-08-13 22:36:10]

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)

[DaleCurtis, 2015-08-13 22:48:39]

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).

[DaleCurtis, 2015-08-13 23:11:49]

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.

[gunsch, 2015-08-13 23:32:39]

rs lgtm for chromecast/

[DaleCurtis, 2015-08-14 01:39:26]

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 :|

[Lei Zhang, 2015-08-14 02:21:18]

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)

[mmenke, 2015-08-14 14:57:35]

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?

[tommycli, 2015-08-14 15:53:07]

lgtm awesome

[DaleCurtis, 2015-08-14 17:37:35]

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, 2015-08-18 02:08:40]

dalecurtis@chromium.org changed reviewers:
+ creis@chromium.org
- ncarter@chromium.org

[DaleCurtis, 2015-08-18 02:08:40]

Swapping the OOO ncarter1@ for creis@.

[Charlie Reis, 2015-08-18 17:30:03]

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.)

[mmenke, 2015-08-18 17:36:04]

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.

[DaleCurtis, 2015-08-18 18:19:12]

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.

[Charlie Reis, 2015-08-18 21:38:15]

Thanks.  content/ LGTM.

[DaleCurtis, 2015-08-18 22:16:42]

mmenke: lgty for now? This isn't breaking anything that isn't already broken on
Android.

[mmenke, 2015-08-18 22:18:23]

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.

[DaleCurtis, 2015-08-18 22:20:52]

Thanks for review everyone!

[DaleCurtis, 2015-08-18 22:20:56]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-08-18 22:20:57]

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.")

[commit-bot: I haz the power, 2015-08-18 22:21:19]

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

[commit-bot: I haz the power, 2015-08-19 00:13:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-19 00:13:41]

Patchset 6 (id:??) landed as
https://crrev.com/0f9e7f4c43221c205982328021138a7ea54261ef
Cr-Commit-Position: refs/heads/master@{#344092}

[soniahibberd337, 2015-10-03 02:56:04]

soniahibberd337@gmail.com changed reviewers:
+ soniahibberd337@gmail.com

[soniahibberd337, 2015-10-03 02:56:04]

soniahibberd337@gmail.com changed reviewers:
+ soniahibberd337@gmail.com

[danituentimsn, 2015-11-05 17:06:28]

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

[Lei Zhang, 2015-11-05 17:09:18]

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.