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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 2 weeks ago by whywhat
Modified:
3 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 (Inactive)
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
Commit queue not available (can’t edit this change).

Messages

Total messages: 117 (59 generated)
whywhat
PTaL (+Dale if Dan is OOO)
3 months, 2 weeks ago (2017-02-08 22:50:52 UTC) #2
whywhat
Fixed the unittests
3 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks ago (2017-02-09 22:43:56 UTC) #10
mlamouri (OOO up to 23-05)
3 months, 1 week ago (2017-02-13 12:16:10 UTC) #12
mlamouri (OOO up to 23-05)
On 2017/02/09 at 22:43:56, dalecurtis wrote: > On 2017/02/09 at 21:54:34, avayvod wrote: > > ...
3 months, 1 week 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 months, 1 week ago (2017-02-13 14:54:19 UTC) #14
whywhat
Fixed browser tests and removed some #if ANDROID
3 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week ago (2017-02-13 19:42:22 UTC) #23
whywhat
Restore permanent 5s pause.
3 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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. ...
3 months, 1 week 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 months, 1 week 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: > ...
3 months, 1 week ago (2017-02-14 01:39:39 UTC) #30
whywhat
Less state in WMPI?
3 months, 1 week ago (2017-02-14 23:39:33 UTC) #31
whywhat
Restore locking background playback in DidPause
3 months, 1 week 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 months, 1 week ago (2017-02-14 23:44:52 UTC) #36
whywhat
Added UserGesture check to DidPause
3 months, 1 week 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 months, 1 week 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: > ...
3 months, 1 week 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 months, 1 week ago (2017-02-15 22:20:01 UTC) #50
whywhat
has_session is true on desktop
3 months, 1 week 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 months, 1 week ago (2017-02-16 02:47:38 UTC) #56
DaleCurtis
3 months, 1 week 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 months, 1 week ago (2017-02-16 13:19:31 UTC) #59
whywhat
3 months, 1 week ago (2017-02-16 13:19:56 UTC) #61
Zhiqiang Zhang (Inactive)
I verified on the pages which had problems before and they are all working \o/
3 months, 1 week ago (2017-02-16 15:26:34 UTC) #63
whywhat
Addressed Dale's comments
3 months, 1 week 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 months, 1 week 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 months, 1 week ago (2017-02-16 22:02:16 UTC) #71
whywhat
Move UserGesture logic to HTMLMediaElement
3 months, 1 week ago (2017-02-17 01:32:06 UTC) #72
whywhat
Remove the flag from RendererWMPDelegate
3 months, 1 week ago (2017-02-17 03:48:00 UTC) #73
whywhat
Whitespace polishing
3 months, 1 week ago (2017-02-17 03:55:49 UTC) #74
whywhat
PTAL Need Mounir to take a look now too since I change HTMLMediaElement.
3 months, 1 week ago (2017-02-17 04:01:21 UTC) #77
mlamouri (OOO up to 23-05)
avayvod@, I think it would be good if you could walk me through the CL. ...
3 months, 1 week 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 months, 1 week ago (2017-02-17 16:30:25 UTC) #83
mlamouri (OOO up to 23-05)
On 2017/02/17 at 16:30:25, avayvod wrote: > The current CL indicates that if the user ...
3 months, 1 week 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 months, 1 week ago (2017-02-17 21:20:51 UTC) #85
whywhat
Tweaked the logic in HTMLME a bit
3 months, 1 week ago (2017-02-17 22:39:45 UTC) #86
mlamouri (OOO up to 23-05)
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 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 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 months ago (2017-02-21 18:28:21 UTC) #93
whywhat
Moved the lock logic into WMPI
3 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 months ago (2017-02-22 02:48:35 UTC) #95
whywhat
Added tests
3 months ago (2017-02-22 16:26:42 UTC) #101
sandersd
This change is actually looking reasonable now. lgtm % still reviewing tests; and I want ...
3 months ago (2017-02-23 00:38:32 UTC) #102
whywhat
Updated the comment in ComputePlayState
3 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 months ago (2017-02-23 02:50:27 UTC) #104
whywhat
3 months ago (2017-02-23 02:50:47 UTC) #107
sandersd
lgtm!
3 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 months ago (2017-02-24 00:53:45 UTC) #114
commit-bot: I haz the power
3 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...
Sign in to reply to this message.

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