Expand suspension of idle media players to all platforms.

media/blink/webmediaplayer_impl.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_BLINK_WEBMEDIAPLAYER_IMPL_H_
 #define MEDIA_BLINK_WEBMEDIAPLAYER_IMPL_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 149 matching lines @@
   blink::WebAudioSourceProvider* audioSourceProvider() override;
 
   void setContentDecryptionModule(
       blink::WebContentDecryptionModule* cdm,
       blink::WebContentDecryptionModuleResult result) override;
 
   void enteredFullscreen() override;
   void exitedFullscreen() override;
 
   // WebMediaPlayerDelegate::Observer implementation.
-  void OnHidden(bool must_suspend) override;
+  void OnHidden() override;
   void OnShown() override;
+  void OnSuspend(bool must_suspend) override;

[sandersd (OOO until July 31), 2016/03/07 23:27:51]

I keep getting tripped up on this bool, so +1 for an enum here.

[DaleCurtis, 2016/03/09 02:56:14]

I kept going around on this and can't find names I liked. So instead I've kept
the bool but renamed this to OnSuspendRequested().

   void OnPlay() override;
   void OnPause() override;
   void OnVolumeMultiplierUpdate(double multiplier) override;
 
 #if defined(OS_ANDROID)  // WMPI_CAST
   bool isRemote() const override;
   void requestRemotePlayback() override;
   void requestRemotePlaybackControl() override;
 
   void SetMediaPlayerManager(
@@ skipping 90 matching lines @@
   void NotifyPlaybackStarted();
   void NotifyPlaybackPaused();
 
   // Called at low frequency to tell external observers how much memory we're
   // using for video playback.  Called by |memory_usage_reporting_timer_|.
   // Memory usage reporting is done in two steps, because |demuxer_| must be
   // accessed on the media thread.
   void ReportMemoryUsage();
   void FinishMemoryUsageReport(int64_t demuxer_memory_usage);
 
+  // Indicates if Resume() or Suspend() are allowed to be called respectively.

[sandersd (OOO until July 31), 2016/03/07 23:27:51]

I'm not clear on this comment given that there is only one method.

Perhaps better would be "Indicates if automatic resumption is allowed in the
current state", or something similar that indicates when this method is called.
(Without that the name sounds like it should be const.)

[DaleCurtis, 2016/03/09 02:56:14]

Done.

+  bool IsResumeAllowed();
+
   blink::WebLocalFrame* frame_;
 
   // TODO(hclam): get rid of these members and read from the pipeline directly.
   blink::WebMediaPlayer::NetworkState network_state_;
   blink::WebMediaPlayer::ReadyState ready_state_;
 
   // Preload state for when |data_source_| is created after setPreload().
   BufferedDataSource::Preload preload_;
 
   // Buffering strategy for when |data_source_| is created after
@@ skipping 122 matching lines @@
 
   // The last volume received by setVolume() and the last volume multiplier from
   // OnVolumeMultiplierUpdate().  The multiplier is typical 1.0, but may be less
   // if the WebMediaPlayerDelegate has requested a volume reduction (ducking)
   // for a transient sound.  Playout volume is derived by volume * multiplier.
   double volume_;
   double volume_multiplier_;
 
   scoped_ptr<RendererFactory> renderer_factory_;
 
+  // Indicates if playback may be started from the background via a to play from

[mlamouri (slow - plz ping), 2016/03/05 22:37:36]

nit: "via a to play from JavaScript" might be missing "call"? (and maybe "play"
should be "play()")

+  // JavaScript or the media session.
+  enum class BackgroundPlaybackState {
+    PLAY_FORBIDDEN_NEVER_CALLED_IN_FOREGROUND,

[xhwang, 2016/03/07 19:13:58]

Can you provide more comments around these states? It's not super obvious to me,
especially the "FORBIDDEN" part here.

[DaleCurtis, 2016/03/07 19:23:07]

Yeah my names are bad :( I was trying to capture "playback has never been
started in the foreground before, so don't let autoplay / js actually start
playback in the background."  This is similar to what we do where we never load
the media until the page is in the foreground unless playback has ever started
in that tab before.

The others are essentially "don't play video in the background" and "play
whatever you want in response to autoplay / js starting playback."

[DaleCurtis, 2016/03/09 02:56:14]

After some more thought, I don't need these values. Removed.

+    PLAY_FORBIDDEN_WHEN_SUSPENDED,
+    PLAY_ALLOWED_WHEN_SUSPENDED,
+  };
+  BackgroundPlaybackState background_playback_state_;
+
   // For requesting surfaces on behalf of the Android H/W decoder in fullscreen.
   // This will be null everywhere but Android.
   SurfaceManager* surface_manager_;
 
   // Suppresses calls to OnPipelineError() after destruction / shutdown has been
   // started; prevents us from spuriously logging errors that are transient or
   // unimportant.
   bool suppress_destruction_errors_;
 
   DISALLOW_COPY_AND_ASSIGN(WebMediaPlayerImpl);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BLINK_WEBMEDIAPLAYER_IMPL_H_