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

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

Created:
3 years, 10 months ago by whywhat
Modified:
3 years, 9 months 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)
3 years, 10 months ago (2017-02-08 22:50:52 UTC) #2
whywhat
Fixed the unittests
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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: > > ...
3 years, 10 months ago (2017-02-09 22:43:56 UTC) #10
mlamouri (slow - plz ping)
3 years, 10 months 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: > > ...
3 years, 10 months 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: > > ...
3 years, 10 months ago (2017-02-13 14:54:19 UTC) #14
whywhat
Fixed browser tests and removed some #if ANDROID
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-13 19:42:22 UTC) #23
whywhat
Restore permanent 5s pause.
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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. ...
3 years, 10 months 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 ...
3 years, 10 months 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: > ...
3 years, 10 months ago (2017-02-14 01:39:39 UTC) #30
whywhat
Less state in WMPI?
3 years, 10 months ago (2017-02-14 23:39:33 UTC) #31
whywhat
Restore locking background playback in DidPause
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-14 23:44:52 UTC) #36
whywhat
Added UserGesture check to DidPause
3 years, 10 months 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 ...
3 years, 10 months 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: > ...
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-15 22:20:01 UTC) #50
whywhat
has_session is true on desktop
3 years, 10 months 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 years, 10 months ago (2017-02-16 02:47:38 UTC) #56
DaleCurtis
3 years, 10 months 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 years, 10 months ago (2017-02-16 13:19:31 UTC) #59
whywhat
3 years, 10 months 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/
3 years, 10 months ago (2017-02-16 15:26:34 UTC) #63
whywhat
Addressed Dale's comments
3 years, 10 months 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 years, 10 months 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 years, 10 months ago (2017-02-16 22:02:16 UTC) #71
whywhat
Move UserGesture logic to HTMLMediaElement
3 years, 10 months ago (2017-02-17 01:32:06 UTC) #72
whywhat
Remove the flag from RendererWMPDelegate
3 years, 10 months ago (2017-02-17 03:48:00 UTC) #73
whywhat
Whitespace polishing
3 years, 10 months ago (2017-02-17 03:55:49 UTC) #74
whywhat
PTAL Need Mounir to take a look now too since I change HTMLMediaElement.
3 years, 10 months 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. ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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: > > ...
3 years, 10 months ago (2017-02-17 21:20:51 UTC) #85
whywhat
Tweaked the logic in HTMLME a bit
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-21 18:28:21 UTC) #93
whywhat
Moved the lock logic into WMPI
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-22 02:48:35 UTC) #95
whywhat
Added tests
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-23 00:38:32 UTC) #102
whywhat
Updated the comment in ComputePlayState
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-23 02:50:27 UTC) #104
whywhat
3 years, 10 months ago (2017-02-23 02:50:47 UTC) #107
sandersd (OOO until July 31)
lgtm!
3 years, 9 months 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
3 years, 9 months ago (2017-02-24 00:53:45 UTC) #114
commit-bot: I haz the power
3 years, 9 months 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