Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(223)

Issue 1292433002: Defer media playback in background tabs. (Closed)

Created:
5 years, 4 months ago by DaleCurtis
Modified:
5 years, 1 month ago
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.

Description

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}

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+112 lines, -109 lines) Patch
A chrome/browser/media/defer_background_media_browsertest.cc View 1 2 3 4 5 1 chunk +60 lines, -0 lines 0 comments Download
M chrome/chrome_renderer.gypi View 1 2 3 4 5 2 chunks +0 lines, -9 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/BUILD.gn View 1 2 3 4 5 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.cc View 1 2 3 4 5 3 chunks +35 lines, -11 lines 0 comments Download
D chrome/renderer/prerender/prerender_media_load_deferrer.h View 1 2 1 chunk +0 lines, -37 lines 0 comments Download
D chrome/renderer/prerender/prerender_media_load_deferrer.cc View 1 2 1 chunk +0 lines, -44 lines 0 comments Download
M chromecast/renderer/cast_content_renderer_client.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chromecast/renderer/cast_content_renderer_client.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/public/renderer/content_renderer_client.h View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
M content/public/renderer/content_renderer_client.cc View 1 2 3 4 5 1 chunk +4 lines, -2 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 3 chunks +3 lines, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 47 (11 generated)
DaleCurtis
I thought about compentizing https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/renderer/cast_media_load_deferrer.h but since we'll want an idle handler in there and ...
5 years, 4 months ago (2015-08-12 22:26:26 UTC) #2
commit-bot: I haz the power
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
5 years, 4 months ago (2015-08-13 01:27:34 UTC) #4
DaleCurtis
On 2015/08/12 22:26:26, DaleCurtis wrote: > I thought about compentizing > https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/renderer/cast_media_load_deferrer.h > but since ...
5 years, 4 months ago (2015-08-13 01:40:10 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-08-13 02:38:08 UTC) #7
Lei Zhang
lgtm https://codereview.chromium.org/1292433002/diff/1/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/1/chrome/renderer/chrome_content_renderer_client.cc#newcode723 chrome/renderer/chrome_content_renderer_client.cc:723: if (!render_frame->IsHidden()) { nit, how about we structure ...
5 years, 4 months ago (2015-08-13 19:11:47 UTC) #8
DaleCurtis
+nick for content/ changes. +gunsch for chromecast/ changes. The change now includes a sticky-play bit ...
5 years, 4 months ago (2015-08-13 21:44:48 UTC) #11
DaleCurtis
Also if anyone knows of a good way to write a chrome/ test which can ...
5 years, 4 months ago (2015-08-13 21:45:42 UTC) #12
tommycli
Really cool approach! https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_content_renderer_client.cc#newcode712 chrome/renderer/chrome_content_renderer_client.cc:712: // Chromium for Android doesn't support ...
5 years, 4 months ago (2015-08-13 21:50:07 UTC) #13
Lei Zhang
On 2015/08/13 21:45:42, DaleCurtis wrote: > Also if anyone knows of a good way to ...
5 years, 4 months ago (2015-08-13 21:52:00 UTC) #14
DaleCurtis
https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_content_renderer_client.cc#newcode712 chrome/renderer/chrome_content_renderer_client.cc:712: // Chromium for Android doesn't support prerender yet. On ...
5 years, 4 months ago (2015-08-13 22:24:30 UTC) #15
DaleCurtis
+mmenke for real this time, see previous comment.
5 years, 4 months ago (2015-08-13 22:24:48 UTC) #17
tommycli
On 2015/08/13 21:52:00, Lei Zhang wrote: > On 2015/08/13 21:45:42, DaleCurtis wrote: > > Also ...
5 years, 4 months ago (2015-08-13 22:30:17 UTC) #18
mmenke
https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1292433002/diff/20001/chrome/renderer/chrome_content_renderer_client.cc#newcode716 chrome/renderer/chrome_content_renderer_client.cc:716: if (prerender::PrerenderHelper::IsPrerendering(render_frame)) { On 2015/08/13 22:24:30, DaleCurtis wrote: > ...
5 years, 4 months ago (2015-08-13 22:36:10 UTC) #19
DaleCurtis
Awesome, I'll go ahead and remove the code and work on some tests for this ...
5 years, 4 months ago (2015-08-13 22:48:39 UTC) #20
DaleCurtis
Actually after chatting with YouTube, they need playback to survive navigation for playlists since there's ...
5 years, 4 months ago (2015-08-13 23:11:49 UTC) #21
gunsch
rs lgtm for chromecast/
5 years, 4 months ago (2015-08-13 23:32:39 UTC) #22
DaleCurtis
Okay, everything should be good to go. I've added a browser test for the current ...
5 years, 4 months ago (2015-08-14 01:39:26 UTC) #23
Lei Zhang
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc#newcode5 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/defer_background_media_browsertest.cc#newcode15 chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does ...
5 years, 4 months ago (2015-08-14 02:21:18 UTC) #24
mmenke
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc#newcode15 chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. Does ...
5 years, 4 months ago (2015-08-14 14:57:35 UTC) #25
tommycli
lgtm awesome
5 years, 4 months ago (2015-08-14 15:53:07 UTC) #26
DaleCurtis
I'm OOO for a couple days, so I'll wait to address feedback until nick@ comments, ...
5 years, 4 months ago (2015-08-14 17:37:35 UTC) #27
DaleCurtis
Swapping the OOO ncarter1@ for creis@.
5 years, 4 months ago (2015-08-18 02:08:40 UTC) #29
Charlie Reis
Just a minor question and nit. I do think Nick would be more familiar with ...
5 years, 4 months ago (2015-08-18 17:30:03 UTC) #30
mmenke
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc#newcode15 chrome/browser/media/defer_background_media_browsertest.cc:15: // Android does not support deferred media loading. On ...
5 years, 4 months ago (2015-08-18 17:36:04 UTC) #31
DaleCurtis
https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc File chrome/browser/media/defer_background_media_browsertest.cc (right): https://codereview.chromium.org/1292433002/diff/60001/chrome/browser/media/defer_background_media_browsertest.cc#newcode5 chrome/browser/media/defer_background_media_browsertest.cc:5: #include "base/strings/stringprintf.h" On 2015/08/14 02:21:18, Lei Zhang wrote: > ...
5 years, 4 months ago (2015-08-18 18:19:12 UTC) #32
Charlie Reis
Thanks. content/ LGTM.
5 years, 4 months ago (2015-08-18 21:38:15 UTC) #33
DaleCurtis
mmenke: lgty for now? This isn't breaking anything that isn't already broken on Android.
5 years, 4 months ago (2015-08-18 22:16:42 UTC) #34
mmenke
On 2015/08/18 22:16:42, DaleCurtis wrote: > mmenke: lgty for now? This isn't breaking anything that ...
5 years, 4 months ago (2015-08-18 22:18:23 UTC) #35
DaleCurtis
Thanks for review everyone!
5 years, 4 months ago (2015-08-18 22:20:52 UTC) #36
commit-bot: I haz the power
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
5 years, 4 months ago (2015-08-18 22:21:19 UTC) #39
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 4 months ago (2015-08-19 00:13:04 UTC) #40
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/0f9e7f4c43221c205982328021138a7ea54261ef Cr-Commit-Position: refs/heads/master@{#344092}
5 years, 4 months ago (2015-08-19 00:13:41 UTC) #41
soniahibberd337
5 years, 2 months ago (2015-10-03 02:56:04 UTC) #44
soniahibberd337
5 years, 2 months ago (2015-10-03 02:56:04 UTC) #45
danituentimsn
Youtube Playlists and Google Play Music are broken... This needs a whitelist!!! I am forced ...
5 years, 1 month ago (2015-11-05 17:06:28 UTC) #46
Lei Zhang
5 years, 1 month ago (2015-11-05 17:09:18 UTC) #47
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.

Powered by Google App Engine
This is Rietveld 408576698