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

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

Created:
1 year ago by whywhat
Modified:
1 year 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, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org, Zhiqiang Zhang (Slow)
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. WMPI now tracks whether background video playback has been unlocked by a user gesture. RendererWebMediaPlayerDelegate emulates user gesture in OnPlay/OnPause to behave the same way as MediaSession. BUG=689637 TEST=manual+updated tests Review-Url: https://codereview.chromium.org/2681863005 Cr-Commit-Position: refs/heads/master@{#452700} Committed: https://chromium.googlesource.com/chromium/src/+/65fad2795b571f868c559941187f785aaa0414a0

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 #

Total comments: 4

Patch Set 14 : Moved the lock logic into WMPI #

Patch Set 15 : Added tests #

Total comments: 4

Patch Set 16 : Updated the comment in ComputePlayState #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -255 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 12 13 6 chunks +14 lines, -17 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 13 1 chunk +0 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.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -0 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 12 chunks +66 lines, -55 lines 0 comments Download
M media/blink/webmediaplayer_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +242 lines, -136 lines 0 comments Download

Messages

Total messages: 117 (59 generated)
whywhat
PTaL (+Dale if Dan is OOO)
1 year ago (2017-02-08 22:50:52 UTC) #2
whywhat
Fixed the unittests
1 year 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 year 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 year 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 year ago (2017-02-09 22:43:56 UTC) #10
mlamouri (slow - plz ping)
1 year ago (2017-02-13 12:16:10 UTC) #12
mlamouri (slow - plz ping)
On 2017/02/09 at 22:43:56, dalecurtis wrote: > On 2017/02/09 at 21:54:34, avayvod wrote: > > ...
1 year 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: > > ...
1 year ago (2017-02-13 14:54:19 UTC) #14
whywhat
Fixed browser tests and removed some #if ANDROID
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year ago (2017-02-13 19:42:22 UTC) #23
whywhat
Restore permanent 5s pause.
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year ago (2017-02-13 22:44:54 UTC) #27
sandersd (OOO until July 31)
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. ...
1 year 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 ...
1 year ago (2017-02-14 01:36:29 UTC) #29
sandersd (OOO until July 31)
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: > ...
1 year ago (2017-02-14 01:39:39 UTC) #30
whywhat
Less state in WMPI?
1 year ago (2017-02-14 23:39:33 UTC) #31
whywhat
Restore locking background playback in DidPause
1 year 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 ...
1 year ago (2017-02-14 23:44:52 UTC) #36
whywhat
Added UserGesture check to DidPause
1 year 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 ...
1 year ago (2017-02-15 17:13:12 UTC) #44
sandersd (OOO until July 31)
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: > ...
1 year 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 ...
1 year ago (2017-02-15 22:20:01 UTC) #50
whywhat
has_session is true on desktop
1 year 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, ...
1 year ago (2017-02-16 02:47:38 UTC) #56
DaleCurtis
1 year 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 ...
1 year ago (2017-02-16 13:19:31 UTC) #59
whywhat
1 year ago (2017-02-16 13:19:56 UTC) #61
Zhiqiang Zhang (Slow)
I verified on the pages which had problems before and they are all working \o/
1 year ago (2017-02-16 15:26:34 UTC) #63
whywhat
Addressed Dale's comments
1 year 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 ...
1 year 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 ...
1 year ago (2017-02-16 22:02:16 UTC) #71
whywhat
Move UserGesture logic to HTMLMediaElement
1 year ago (2017-02-17 01:32:06 UTC) #72
whywhat
Remove the flag from RendererWMPDelegate
1 year ago (2017-02-17 03:48:00 UTC) #73
whywhat
Whitespace polishing
1 year ago (2017-02-17 03:55:49 UTC) #74
whywhat
PTAL Need Mounir to take a look now too since I change HTMLMediaElement.
1 year ago (2017-02-17 04:01:21 UTC) #77
mlamouri (slow - plz ping)
avayvod@, I think it would be good if you could walk me through the CL. ...
1 year 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 ...
1 year ago (2017-02-17 16:30:25 UTC) #83
mlamouri (slow - plz ping)
On 2017/02/17 at 16:30:25, avayvod wrote: > The current CL indicates that if the user ...
1 year 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: > > ...
1 year ago (2017-02-17 21:20:51 UTC) #85
whywhat
Tweaked the logic in HTMLME a bit
1 year ago (2017-02-17 22:39:45 UTC) #86
mlamouri (slow - plz ping)
https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3133 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3133: } I know you already replied to this comment ...
1 year ago (2017-02-20 12:00:17 UTC) #91
whywhat
Note that this changes the behavior (compared to the previous PS) in the case when ...
1 year ago (2017-02-20 17:16:04 UTC) #92
whywhat
After a discussion with Mounir we decided we don't want to change the behavior of ...
1 year ago (2017-02-21 18:28:21 UTC) #93
whywhat
Moved the lock logic into WMPI
1 year ago (2017-02-22 02:41:36 UTC) #94
whywhat
PTAL I moved the logic to keep track of whether the player can be resumed ...
1 year ago (2017-02-22 02:48:35 UTC) #95
whywhat
Added tests
1 year ago (2017-02-22 16:26:42 UTC) #101
sandersd (OOO until July 31)
This change is actually looking reasonable now. lgtm % still reviewing tests; and I want ...
1 year ago (2017-02-23 00:38:32 UTC) #102
whywhat
Updated the comment in ComputePlayState
1 year ago (2017-02-23 02:48:57 UTC) #103
whywhat
Thanks for a quick turnaround! https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediaplayer_impl.cc#newcode423 media/blink/webmediaplayer_impl.cc:423: if (blink::WebUserGestureIndicator::isProcessingUserGesture()) On 2017/02/23 ...
1 year ago (2017-02-23 02:50:27 UTC) #104
whywhat
1 year ago (2017-02-23 02:50:47 UTC) #107
sandersd (OOO until July 31)
lgtm!
1 year ago (2017-02-23 22:57:24 UTC) #111
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2681863005/300001
1 year ago (2017-02-24 00:53:45 UTC) #114
commit-bot: I haz the power
1 year ago (2017-02-24 01:01:31 UTC) #117
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/65fad2795b571f868c559941187f...

Powered by Google App Engine
This is Rietveld 408576698