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..da959544e668fc3e03ae5d74a73fa7c60be30ca1 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,32 +28,36 @@ namespace media {
 
 enum class MediaContentType;
 
-// An interface to allow a WebMediaPlayerImpl to communicate changes of state
-// to objects that need to know.
+// An interface to collect WebMediaPlayer state changes and to fan out commands

[watk, 2016/12/10 00:01:30]

Not an interface

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

+// from the browser.
 class CONTENT_EXPORT RendererWebMediaPlayerDelegate
     : public content::RenderFrameObserver,
       public NON_EXPORTED_BASE(WebMediaPlayerDelegate),
       public NON_EXPORTED_BASE(
           base::SupportsWeakPtr<RendererWebMediaPlayerDelegate>) {
  public:
-  explicit RendererWebMediaPlayerDelegate(content::RenderFrame* render_frame);
+  RendererWebMediaPlayerDelegate(content::RenderFrame* render_frame);

[watk, 2016/12/10 00:01:31]

Keep explicit?

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

   ~RendererWebMediaPlayerDelegate() override;
 
   // Returns true if this RenderFrame has ever seen media playback before.
   bool has_played_media() const { return has_played_media_; }
 
   // WebMediaPlayerDelegate implementation.
+  bool IsFrameHidden() override;
+  bool IsFrameClosed() override;
+  bool IsBackgroundVideoPlaybackAllowed() override;
   int AddObserver(Observer* observer) override;
-  void RemoveObserver(int delegate_id) override;
-  void DidPlay(int delegate_id,
+  void RemoveObserver(int player_id) override;
+  void DidPlay(int player_id,
                bool has_video,

[watk, 2016/12/10 00:01:30]

You swapped audio and video in the implementation

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

                bool has_audio,
-               bool is_remote,
                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;
 
  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();
 
-  // Setter for |is_playing_background_video_| that updates the metrics.
-  void SetIsPlayingBackgroundVideo(bool is_playing);
+  // Records UMAs about background playback.
+  void RecordBackgroundVideoPlayback();
+
+  // Runs periodically to notify stale players in |idle_player_map_| which
+  // have been idle for longer than |timeout|.
+  void CleanupIdlePlayers(base::TimeDelta timeout);

[watk, 2016/12/10 00:01:31]

While you're changing this (:]) s/Cleanup/CleanUp/ because cleanup is not a
verb.

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

 
   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_ = false;
+  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.

[watk, 2016/12/10 00:01:30]

I'm fine with stale, but I'm wondering if suspendable is better?

[watk, 2016/12/10 00:12:30]

After reading through the whole thing stale has grown on me.

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Acknowledged.

+  std::map<int, base::TimeTicks> idle_player_map_;
+  std::set<int> stale_players_;
+  base::RepeatingTimer 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 enters the paused before
+  // the player is suspended.

[watk, 2016/12/10 00:01:31]

Should probably reword this now. "paused" and "suspended" seem like they should
be "idle" and "stale".

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

   base::TimeDelta idle_timeout_;
 
-  // The polling interval used for checking the delegates to see if any have
+  // The polling interval used for checking the players to see if any have
   // exceeded |idle_timeout_| since their last pause state.

[watk, 2016/12/10 00:01:31]

since they became idle?

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

   base::TimeDelta idle_cleanup_interval_;
 
-  // Clock used for calculating when delegates have expired. May be overridden
+  // Clock used for calculating when players have expired. May be overridden

[watk, 2016/12/10 00:01:30]

expired? 

[sandersd (OOO until July 31), 2017/01/05 23:12:20]

Done.

   // 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