Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 2681863005: [Video] MediaSession API event handlers can resume background video.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 4 days ago by whywhat
Modified:
2 days, 1 hour ago
CC:
apacible+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, erickung+watch_chromium.org, feature-media-reviews_chromium.org, jam (OOO), mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org, Zhiqiang Zhang
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Video] MediaSession API event handlers can resume background video. ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused. WMPI::UpdatePlayState_ComputePlayState now only cares about the presence of the session and allows playback of anything as long as it got play() command. HTMLMediaElement now controls the user gesture aspects of the background video playback. It also uses play() and pause() to implement the default MediaSession behavior. BUG=689637 TEST=manual+updated tests

Patch Set 1 #

Patch Set 2 : Fixed the unittests #

Patch Set 3 : Fixed browser tests and removed some #if ANDROID #

Patch Set 4 : Restore permanent 5s pause. #

Total comments: 14

Patch Set 5 : Less state in WMPI? #

Patch Set 6 : Restore locking background playback in DidPause #

Patch Set 7 : Added UserGesture check to DidPause #

Patch Set 8 : has_session is true on desktop #

Total comments: 4

Patch Set 9 : Addressed Dale's comments #

Patch Set 10 : Move UserGesture logic to HTMLMediaElement #

Patch Set 11 : Remove the flag from RendererWMPDelegate #

Patch Set 12 : Whitespace polishing #

Total comments: 6

Patch Set 13 : Tweaked the logic in HTMLME a bit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+212 lines, -155 lines) Patch
M content/browser/media/session/media_session_impl_visibility_browsertest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/renderer_webmediaplayer_delegate.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +0 lines, -2 lines 0 comments Download
M content/renderer/media/renderer_webmediaplayer_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +3 lines, -21 lines 0 comments Download
M content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +0 lines, -39 lines 0 comments Download
M content/renderer/media/webmediaplayer_ms_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download
M media/blink/webmediaplayer_delegate.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -4 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 10 chunks +33 lines, -52 lines 0 comments Download
M media/blink/webmediaplayer_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +135 lines, -30 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +28 lines, -5 lines 0 comments Download
M third_party/WebKit/public/platform/WebMediaPlayerClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 90 (45 generated)
whywhat
PTaL (+Dale if Dan is OOO)
1 week, 4 days ago (2017-02-08 22:50:52 UTC) #2
whywhat
Fixed the unittests
1 week, 4 days ago (2017-02-08 23:55:53 UTC) #3
DaleCurtis
This doesn't seem to touch the speculative pause after 5 seconds code? If we're always ...
1 week, 3 days ago (2017-02-09 20:30:50 UTC) #8
whywhat
On 2017/02/09 at 20:30:50, dalecurtis wrote: > This doesn't seem to touch the speculative pause ...
1 week, 3 days ago (2017-02-09 21:54:34 UTC) #9
DaleCurtis
On 2017/02/09 at 21:54:34, avayvod wrote: > On 2017/02/09 at 20:30:50, dalecurtis wrote: > > ...
1 week, 3 days ago (2017-02-09 22:43:56 UTC) #10
mlamouri
6 days, 13 hours ago (2017-02-13 12:16:10 UTC) #12
mlamouri
On 2017/02/09 at 22:43:56, dalecurtis wrote: > On 2017/02/09 at 21:54:34, avayvod wrote: > > ...
6 days, 13 hours ago (2017-02-13 12:19:29 UTC) #13
whywhat
On 2017/02/13 at 12:19:29, mlamouri wrote: > On 2017/02/09 at 22:43:56, dalecurtis wrote: > > ...
6 days, 10 hours ago (2017-02-13 14:54:19 UTC) #14
whywhat
Fixed browser tests and removed some #if ANDROID
6 days, 9 hours ago (2017-02-13 16:31:31 UTC) #15
DaleCurtis
Sorry some of the comments here aren't making sense. I'll patch this in and take ...
6 days, 6 hours ago (2017-02-13 19:11:51 UTC) #20
DaleCurtis
Okay after testing, this breaks the auto-pause after 5 seconds; which isn't okay. It doesn't ...
6 days, 6 hours ago (2017-02-13 19:27:12 UTC) #21
DaleCurtis
I.e., if you put a playing video in the background, it should not resume into ...
6 days, 6 hours ago (2017-02-13 19:27:34 UTC) #22
whywhat
On 2017/02/13 at 19:27:34, dalecurtis wrote: > I.e., if you put a playing video in ...
6 days, 6 hours ago (2017-02-13 19:42:22 UTC) #23
whywhat
Restore permanent 5s pause.
6 days, 5 hours ago (2017-02-13 19:47:30 UTC) #24
DaleCurtis
Should never auto-resume after some timeout, currently at 5 seconds. This prevents user surprise when ...
6 days, 4 hours ago (2017-02-13 21:09:39 UTC) #25
whywhat
On 2017/02/13 at 21:09:39, dalecurtis wrote: > Should never auto-resume after some timeout, currently at ...
6 days, 4 hours ago (2017-02-13 21:21:53 UTC) #26
DaleCurtis
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode118 content/renderer/media/renderer_webmediaplayer_delegate.cc:118: if (playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) Hmm, is this ...
6 days, 3 hours ago (2017-02-13 22:44:54 UTC) #27
sandersd
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode101 content/renderer/media/renderer_webmediaplayer_delegate.cc:101: if (has_video && IsFrameHidden() && !IsFrameClosed()) Duplicate |has_video| condition. ...
6 days, 2 hours ago (2017-02-13 22:56:20 UTC) #28
whywhat
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode118 content/renderer/media/renderer_webmediaplayer_delegate.cc:118: if (playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) On 2017/02/13 at ...
6 days ago (2017-02-14 01:36:29 UTC) #29
sandersd
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode119 content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; On 2017/02/14 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > ...
6 days ago (2017-02-14 01:39:39 UTC) #30
whywhat
Less state in WMPI?
5 days, 2 hours ago (2017-02-14 23:39:33 UTC) #31
whywhat
Restore locking background playback in DidPause
5 days, 2 hours ago (2017-02-14 23:43:49 UTC) #35
whywhat
On 2017/02/14 at 01:39:39, sandersd wrote: > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc > File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): > > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode119 ...
5 days, 2 hours ago (2017-02-14 23:44:52 UTC) #36
whywhat
Added UserGesture check to DidPause
4 days, 8 hours ago (2017-02-15 17:02:42 UTC) #43
whywhat
PTAL https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode101 content/renderer/media/renderer_webmediaplayer_delegate.cc:101: if (has_video && IsFrameHidden() && !IsFrameClosed()) On 2017/02/13 ...
4 days, 8 hours ago (2017-02-15 17:13:12 UTC) #44
sandersd
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode119 content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; On 2017/02/15 17:13:12, whywhat_slow_PST_till_Feb_6 wrote: > ...
4 days, 3 hours ago (2017-02-15 21:54:09 UTC) #49
whywhat
On 2017/02/15 at 21:54:09, sandersd wrote: > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc > File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): > > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode119 ...
4 days, 3 hours ago (2017-02-15 22:20:01 UTC) #50
whywhat
has_session is true on desktop
4 days, 2 hours ago (2017-02-15 23:30:47 UTC) #51
DaleCurtis
UpdatePlayState changes are hard to vet, but tests look good, so just a few questions, ...
3 days, 22 hours ago (2017-02-16 02:47:38 UTC) #56
DaleCurtis
3 days, 22 hours ago (2017-02-16 02:47:49 UTC) #58
whywhat
https://codereview.chromium.org/2681863005/diff/140001/content/renderer/media/renderer_webmediaplayer_delegate.cc File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/140001/content/renderer/media/renderer_webmediaplayer_delegate.cc#newcode124 content/renderer/media/renderer_webmediaplayer_delegate.cc:124: playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) { On 2017/02/16 at ...
3 days, 12 hours ago (2017-02-16 13:19:31 UTC) #59
whywhat
3 days, 12 hours ago (2017-02-16 13:19:56 UTC) #61
Zhiqiang Zhang
I verified on the pages which had problems before and they are all working \o/
3 days, 10 hours ago (2017-02-16 15:26:34 UTC) #63
whywhat
Addressed Dale's comments
3 days, 5 hours ago (2017-02-16 19:59:24 UTC) #64
DaleCurtis
Dan explained some of his concerns for the user gesture processing and I agree. You ...
3 days, 4 hours ago (2017-02-16 21:32:43 UTC) #68
whywhat
On 2017/02/16 at 21:32:43, dalecurtis wrote: > Dan explained some of his concerns for the ...
3 days, 3 hours ago (2017-02-16 22:02:16 UTC) #71
whywhat
Move UserGesture logic to HTMLMediaElement
3 days ago (2017-02-17 01:32:06 UTC) #72
whywhat
Remove the flag from RendererWMPDelegate
2 days, 21 hours ago (2017-02-17 03:48:00 UTC) #73
whywhat
Whitespace polishing
2 days, 21 hours ago (2017-02-17 03:55:49 UTC) #74
whywhat
PTAL Need Mounir to take a look now too since I change HTMLMediaElement.
2 days, 21 hours ago (2017-02-17 04:01:21 UTC) #77
mlamouri
avayvod@, I think it would be good if you could walk me through the CL. ...
2 days, 13 hours ago (2017-02-17 12:31:31 UTC) #82
whywhat
https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3131 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3131: DocumentUserGestureToken::create(&document())); On 2017/02/17 at 12:31:31, mlamouri wrote: > You ...
2 days, 9 hours ago (2017-02-17 16:30:25 UTC) #83
mlamouri
On 2017/02/17 at 16:30:25, avayvod wrote: > The current CL indicates that if the user ...
2 days, 8 hours ago (2017-02-17 17:00:16 UTC) #84
whywhat
On 2017/02/17 at 17:00:16, mlamouri wrote: > On 2017/02/17 at 16:30:25, avayvod wrote: > > ...
2 days, 4 hours ago (2017-02-17 21:20:51 UTC) #85
whywhat
2 days, 3 hours ago (2017-02-17 22:39:45 UTC) #86
Tweaked the logic in HTMLME a bit
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd