Refactor WebMediaPlayerDelegate interface.

content/renderer/media/renderer_webmediaplayer_delegate.h

Index: content/renderer/media/renderer_webmediaplayer_delegate.h
diff --git a/content/renderer/media/renderer_webmediaplayer_delegate.h b/content/renderer/media/renderer_webmediaplayer_delegate.h
index a8aa0e144de5ee5e8622d8e9524fcb309e6265ba..69e02c424ff5ff1f11b953bfc196f7b60d84c314 100644
--- a/content/renderer/media/renderer_webmediaplayer_delegate.h
+++ b/content/renderer/media/renderer_webmediaplayer_delegate.h
@@ -11,7 +11,9 @@
 
 #include "base/id_map.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
 #include "base/time/default_tick_clock.h"
 #include "base/timer/timer.h"
 #include "content/common/content_export.h"
@@ -26,8 +28,8 @@ namespace media {
 
 enum class MediaContentType;
 
-// An interface to allow a WebMediaPlayerImpl to communicate changes of state
-// to objects that need to know.
+// Standard implementation of WebMediaPlayerDelegate; communicates state to
+// the MediaPlayerDelegateHost.
 class CONTENT_EXPORT RendererWebMediaPlayerDelegate
     : public content::RenderFrameObserver,
       public NON_EXPORTED_BASE(WebMediaPlayerDelegate),
@@ -41,17 +43,21 @@ class CONTENT_EXPORT RendererWebMediaPlayerDelegate
   bool has_played_media() const { return has_played_media_; }
 
   // WebMediaPlayerDelegate implementation.
+  bool IsFrameHidden() override;
+  bool IsFrameClosed() override;
+  bool IsBackgroundVideoPlaybackUnlocked() override;
   int AddObserver(Observer* observer) override;
-  void RemoveObserver(int delegate_id) override;
-  void DidPlay(int delegate_id,
-               bool has_video,
+  void RemoveObserver(int player_id) override;
+  void DidPlay(int player_id,
                bool has_audio,

[DaleCurtis, 2017/01/10 21:28:23]

Why did you reorder these? The order matches that in the delegate message and
elsewhere. If anything we should just replace this with a enum instead of
reordering.

[sandersd (OOO until July 31), 2017/01/11 01:22:50]

Done.

-               bool is_remote,
+               bool has_video,
                MediaContentType media_content_type) override;
-  void DidPause(int delegate_id, bool reached_end_of_stream) override;
-  void PlayerGone(int delegate_id) override;
-  bool IsHidden() override;
-  bool IsPlayingBackgroundVideo() override;
+  void DidPause(int player_id) override;
+  void PlayerGone(int player_id) override;
+  void SetIdle(int player_id, bool is_idle) override;
+  bool IsIdle(int player_id) override;
+  void ClearStaleFlag(int player_id) override;
+  bool IsStale(int player_id) override;
 
   // content::RenderFrameObserver overrides.
   void WasHidden() override;
@@ -65,61 +71,67 @@ class CONTENT_EXPORT RendererWebMediaPlayerDelegate
   void SetIdleCleanupParamsForTesting(base::TimeDelta idle_timeout,
                                       base::TickClock* tick_clock,
                                       bool is_low_end_device);
-  bool IsIdleCleanupTimerRunningForTesting() const {
-    return idle_cleanup_timer_.IsRunning();
-  }
+  bool IsIdleCleanupTimerRunningForTesting() const;
+
+  // Note: Does not call OnFrameHidden()/OnFrameShown().
+  void SetFrameHiddenForTesting(bool is_hidden);
 
   friend class RendererWebMediaPlayerDelegateTest;

[DaleCurtis, 2017/01/10 21:28:23]

Do we need all these ForTesting() methods if the test class is a friend?

[sandersd (OOO until July 31), 2017/01/11 01:22:50]

I suppose not; at the very least they can be made private. I do prefer this
style though, rather than making boilerplate wrapper functions in the test
class. Do you have a strong preference?

 
  private:
-  void OnMediaDelegatePause(int delegate_id);
-  void OnMediaDelegatePlay(int delegate_id);
+  void OnMediaDelegatePause(int player_id);
+  void OnMediaDelegatePlay(int player_id);
   void OnMediaDelegateSuspendAllMediaPlayers();
-  void OnMediaDelegateVolumeMultiplierUpdate(int delegate_id,
-                                             double multiplier);
+  void OnMediaDelegateVolumeMultiplierUpdate(int player_id, double multiplier);
 
-  // Adds or removes a delegate from |idle_delegate_map_|. The first insertion
-  // or last removal will start or stop |idle_cleanup_timer_| respectively.
-  void AddIdleDelegate(int delegate_id);
-  void RemoveIdleDelegate(int delegate_id);
+  // Schedules UpdateTask() to run soon.
+  void ScheduleUpdateTask();
 
-  // Runs periodically to suspend idle delegates in |idle_delegate_map_| which
-  // have been idle for longer than |timeout|.
-  void CleanupIdleDelegates(base::TimeDelta timeout);
+  // Processes state changes, dispatches CleanupIdlePlayers().
+  void UpdateTask();
+
+  // Records UMAs about background playback.
+  void RecordBackgroundVideoPlayback();
 
-  // Setter for |is_playing_background_video_| that updates the metrics.
-  void SetIsPlayingBackgroundVideo(bool is_playing);
+  // Runs periodically to notify stale players in |idle_player_map_| which
+  // have been idle for longer than |timeout|.
+  void CleanUpIdlePlayers(base::TimeDelta timeout);
 
   bool has_played_media_ = false;
+  bool background_video_allowed_ = false;
+  bool is_frame_closed_ = false;
+  bool is_frame_hidden_for_testing_ = false;
+
+  // State related to scheduling UpdateTask().
+  bool did_play_video_ = false;

[DaleCurtis, 2017/01/10 21:28:23]

has_played_video_ for consistency?

[sandersd (OOO until July 31), 2017/01/11 01:22:50]

Done.

+  bool pending_update_task_ = false;
+
   IDMap<Observer*> id_map_;
 
-  // Tracks which delegates have entered an idle state. After some period of
-  // inactivity these players will be suspended to release unused resources.
-  bool idle_cleanup_running_ = false;
-  std::map<int, base::TimeTicks> idle_delegate_map_;
-  base::Timer idle_cleanup_timer_;
+  // Tracks which players have entered an idle state. After some period of
+  // inactivity these players will be notified and become stale.
+  std::map<int, base::TimeTicks> idle_player_map_;
+  std::set<int> stale_players_;
+  base::OneShotTimer idle_cleanup_timer_;
 
-  // Amount of time allowed to elapse after a delegate enters the paused before
-  // the delegate is suspended.
+  // Amount of time allowed to elapse after a player becomes idle before
+  // it can transition to stale.
   base::TimeDelta idle_timeout_;
 
-  // The polling interval used for checking the delegates to see if any have
-  // exceeded |idle_timeout_| since their last pause state.
+  // The polling interval used for checking the players to see if any have
+  // exceeded |idle_timeout_| since becoming idle.
   base::TimeDelta idle_cleanup_interval_;
 
-  // Clock used for calculating when delegates have expired. May be overridden
-  // for testing.
+  // Clock used for calculating when players have become stale. May be
+  // overridden for testing.
   std::unique_ptr<base::DefaultTickClock> default_tick_clock_;
   base::TickClock* tick_clock_;
 
-  // If a video is playing in the background. Set when user resumes a video
-  // allowing it to play and reset when either user pauses it or it goes
-  // foreground.
-  bool is_playing_background_video_ = false;
-
 #if defined(OS_ANDROID)
+  bool was_playing_background_video_ = false;
+
   // Keeps track of when the background video playback started for metrics.
-  base::TimeTicks background_video_playing_start_time_;
+  base::TimeTicks background_video_start_time_;
 #endif  // OS_ANDROID
 
   // The currently playing local videos. Used to determine whether