Convert WMPI state management to level-triggered.

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 177 matching lines @@
   void DisplayCastFrameAfterSuspend(const scoped_refptr<VideoFrame>& new_frame,
                                     PipelineStatus status);
   gfx::Size GetCanvasSize() const;
   void SetDeviceScaleFactor(float scale_factor);
 #endif
 
   // Called from WebMediaPlayerCast.
   // TODO(hubbe): WMPI_CAST make private.
   void OnPipelineSeeked(bool time_updated);
 
+  // Distinct states that |delegate_| can be in.
+  // TODO(sandersd): This should move into WebMediaPlayerDelegate.
+  // (Public for testing.)
+  enum class DelegateState {
+    GONE,
+    PLAYING,
+    PAUSED,
+    ENDED,
+  };
+
+  // Playback state variables computed together in UpdatePlayState().
+  // (Public for testing.)
+  struct PlayState {
+    DelegateState delegate_state;
+    bool is_memory_reporting_enabled;
+    bool is_suspended;
+  };
+
  private:
+  friend class WebMediaPlayerImplTest;
+
   void OnPipelineSuspended();
-  void OnPipelineResumed();
   void OnPipelineEnded();
   void OnPipelineError(PipelineStatus error);
   void OnPipelineMetadata(PipelineMetadata metadata);
   void OnPipelineBufferingStateChanged(BufferingState buffering_state);
   void OnDemuxerOpened();
   void OnAddTextTrack(const TextTrackConfig& config,
                       const AddTextTrackDoneCB& done_cb);
 
   // Actually seek. Avoids causing |should_notify_time_changed_| to be set when
   // |time_updated| is false.
@@ skipping 56 matching lines @@
   // is not available.
   void OnWaitingForDecryptionKey();
 
   // Sets |cdm_context| on the pipeline and fires |cdm_attached_cb| when done.
   // Parameter order is reversed for easy binding.
   void SetCdm(const CdmAttachedCB& cdm_attached_cb, CdmContext* cdm_context);
 
   // Called when a CDM has been attached to the |pipeline_|.
   void OnCdmAttached(bool success);
 
-  // Notifies |delegate_| that playback has started or was paused; also starts
-  // or stops the memory usage reporting timer respectively.
-  void NotifyPlaybackStarted();
-  void NotifyPlaybackPaused();
+  // Inspects the current playback state and:
+  //   - notifies |delegate_|,
+  //   - toggles the memory usage reporting timer, and
+  //   - toggles suspend/resume as necessary.
+  //
+  // This method should be called any time its dependend values change. These

[DaleCurtis, 2016/04/01 18:25:48]

Dependent. Instead of trying to enforce this via a comment, should we stick
those values in a private inner class w/ setters so we can guarantee this
occurs? Update: Discussed offline, we need to change multiple values at once and
don't want to call UpdatePlayState for each one.

We should still come up with a stronger protection here, but can be done in a
followup.

[sandersd (OOO until July 31), 2016/04/01 21:22:11]

Acknowledged.

+  // are:
+  //   - isRemote(),
+  //   - hasVideo(),
+  //   - delegate_->IsHidden(),
+  //   - network_state_, ready_state_,
+  //   - is_idle_, must_suspend_,
+  //   - paused_, ended_,
+  //   - pending_suspend_resume_cycle_.
+  void UpdatePlayState();
+
+  // Methods internal to UpdatePlayState().
+  PlayState UpdatePlayState_ComputePlayState(bool is_remote,
+                                             bool is_backgrounded);
+  void SetDelegateState(DelegateState new_state);
+  void SetMemoryReportingState(bool is_memory_reporting_enabled);
+  void SetSuspendState(bool is_suspended);
 
   // 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 automatic resumption of a suspended playback is allowed.
-  bool IsAutomaticResumeAllowed();
+  blink::WebLocalFrame* frame_;
 
-  blink::WebLocalFrame* frame_;
+  // The playback state last reported to |delegate_|, to avoid setting duplicate
+  // states. (Which can have undesired effects like resetting the idle timer.)
+  DelegateState delegate_state_;
+
+  // Set when OnSuspendRequested() is called with |must_suspend| unset.
+  bool is_idle_;
+
+  // Set when OnSuspendRequested() is called with |must_suspend| set.
+  bool must_suspend_;
 
   // TODO(hclam): get rid of these members and read from the pipeline directly.

[DaleCurtis, 2016/04/01 18:25:48]

Delete? :)

[sandersd (OOO until July 31), 2016/04/01 21:22:11]

Done.

   blink::WebMediaPlayer::NetworkState network_state_;
   blink::WebMediaPlayer::ReadyState ready_state_;
+  blink::WebMediaPlayer::ReadyState highest_ready_state_;
 
   // Preload state for when |data_source_| is created after setPreload().
   BufferedDataSource::Preload preload_;
 
   // Buffering strategy for when |data_source_| is created after
   // setBufferingStrategy().
   BufferedDataSource::BufferingStrategy buffering_strategy_;
 
   // Task runner for posting tasks on Chrome's main thread. Also used
   // for DCHECKs so methods calls won't execute in the wrong thread.
@@ skipping 132 matching lines @@
   // 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_