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

content/renderer/media/renderer_webmediaplayer_delegate.cc

Index: content/renderer/media/renderer_webmediaplayer_delegate.cc
diff --git a/content/renderer/media/renderer_webmediaplayer_delegate.cc b/content/renderer/media/renderer_webmediaplayer_delegate.cc
index 92d1140e8a6c52c2500f4daa21383717f482f37d..ef851b37229cdcf33678719062870473785025e5 100644
--- a/content/renderer/media/renderer_webmediaplayer_delegate.cc
+++ b/content/renderer/media/renderer_webmediaplayer_delegate.cc
@@ -98,6 +98,8 @@ void RendererWebMediaPlayerDelegate::DidPlay(
       playing_videos_.insert(player_id);
       has_played_video_ = true;
     }
+    if (has_video && IsFrameHidden() && !IsFrameClosed())

[sandersd (OOO until July 31), 2017/02/13 22:56:20]

Duplicate |has_video| condition.

[whywhat, 2017/02/15 17:13:12]

Done.

+      background_video_allowed_ = true;
   } else {
     playing_videos_.erase(player_id);
   }
@@ -112,6 +114,10 @@ void RendererWebMediaPlayerDelegate::DidPlay(
 void RendererWebMediaPlayerDelegate::DidPause(int player_id) {
   DVLOG(2) << __func__ << "(" << player_id << ")";
   DCHECK(id_map_.Lookup(player_id));
+
+  if (playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed())

[DaleCurtis, 2017/02/13 22:44:54]

Hmm, is this right? Can you add some comments about why you'd want to prevent
this here? It looks like it would stop playlist functionality from working in
the background properly.

[whywhat, 2017/02/14 01:36:28]

This is consistent with the user pressing on the pause button in the
notification (handled in OnMediaDelegatePause below) but covers the MediaSession
path (where it's the page that calls pause()) so we don't allow the page to
resume despite the user wishes. Perhaps we should just check for user gesture in
play() for that (and perhaps we do already).

But you're right, if the user presses on the next track, it's possible the page
will do something like:

v.pause();  // this will prevent play() from working.
v.src = nextTrackSrc;
v.play();

Which will be broken. If it doesn't call pause(), it will still work though.

[whywhat, 2017/02/15 17:13:12]

Added a user gesture check to that so we won't block queuing unless it was the
user who paused the playback. If the page will resume the next track in the same
event handler it will still work as there will still be a user gesture.

+    background_video_allowed_ = false;

[sandersd (OOO until July 31), 2017/02/13 22:56:20]

This can leave us in a strange state where there is background video playing,
but |background_video_allowed_| is false. This could result in surprising
behavior in WMPI if that flag is used in ComputePlayState().

[whywhat, 2017/02/14 01:36:28]

Hm, how? This is called when video is being paused and would prevent the video
to resume in the background, no?

[sandersd (OOO until July 31), 2017/02/14 01:39:39]

There can be more than one playing video on a page at a time.

[whywhat, 2017/02/15 17:13:12]

This is already the case with OnMediaDelegatePause.

[sandersd (OOO until July 31), 2017/02/15 21:54:08]

There is a difference, though; OnMediaDelegatePause() is an explicit user
gesture.

I imagine that the actual user intent would be to pause all playing content in
the page when they press pause there, but I suppose that doesn't happen either.

+
   playing_videos_.erase(player_id);
   Send(new MediaPlayerDelegateHostMsg_OnMediaPaused(routing_id(), player_id,
                                                     false));