media: Add AudioStreamMonitor::AudibleClientRegistration

As suggested in previous code review, adding a class to automatically
remove audible client on destruction.

Bug: 1017943
Change-Id: I3007f4f1300c12984a7d8e92ab26d3006c401d92
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2998306
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#897890}

Author: xhwang-chromium

content/browser/media/audio_stream_monitor.cc

@@ -28,6 +28,16 @@ AudioStreamMonitor* GetMonitorForRenderFrame(int render_process_id,
 
 }  // namespace
 
+AudioStreamMonitor::AudibleClientRegistration::AudibleClientRegistration(
+    AudioStreamMonitor* audio_stream_monitor)
+    : audio_stream_monitor_(audio_stream_monitor) {
+  audio_stream_monitor_->AddAudibleClient();
+}
+
+AudioStreamMonitor::AudibleClientRegistration::~AudibleClientRegistration() {
+  audio_stream_monitor_->RemoveAudibleClient();
+}
+
 bool AudioStreamMonitor::StreamID::operator<(const StreamID& other) const {
   return std::tie(render_process_id, render_frame_id, stream_id) <
          std::tie(other.render_process_id, other.render_frame_id,
@@ -47,7 +57,9 @@ AudioStreamMonitor::AudioStreamMonitor(WebContents* contents)
   DCHECK(web_contents_);
 }
 
-AudioStreamMonitor::~AudioStreamMonitor() {}
+AudioStreamMonitor::~AudioStreamMonitor() {
+  DCHECK_EQ(audible_clients_, 0);
+}
 
 bool AudioStreamMonitor::WasRecentlyAudible() const {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -77,6 +89,12 @@ void AudioStreamMonitor::RenderProcessGone(int render_process_id) {
   UpdateStreams();
 }
 
+std::unique_ptr<AudioStreamMonitor::AudibleClientRegistration>
+AudioStreamMonitor::RegisterAudibleClient() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return std::make_unique<AudibleClientRegistration>(this);
+}
+
 void AudioStreamMonitor::AddAudibleClient() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_GE(audible_clients_, 0);

content/browser/media/audio_stream_monitor.h

@@ -55,10 +55,6 @@ class CONTENT_EXPORT AudioStreamMonitor : public WebContentsObserver {
   // any outstanding poll callbacks.
   void RenderProcessGone(int render_process_id);
 
-  // Adds/Removes Audible clients.
-  void AddAudibleClient();
-  void RemoveAudibleClient();
-
   // Starts or stops monitoring respectively for the stream owned by the
   // specified renderer.  Safe to call from any thread.
   static void StartMonitoringStream(int render_process_id,
@@ -85,8 +81,24 @@ class CONTENT_EXPORT AudioStreamMonitor : public WebContentsObserver {
 
   void set_is_currently_audible_for_testing(bool value) { is_audible_ = value; }
 
+  // Class to help automatically remove audible client.
+  class CONTENT_EXPORT AudibleClientRegistration {
+   public:
+    explicit AudibleClientRegistration(
+        AudioStreamMonitor* audio_stream_monitor);
+    ~AudibleClientRegistration();
+
+   private:
+    AudioStreamMonitor* audio_stream_monitor_;
+  };
+
+  // Registers an audible client, which will be unregistered when the returned
+  // AudibleClientRegistration is released.
+  std::unique_ptr<AudibleClientRegistration> RegisterAudibleClient();
+
  private:
   friend class AudioStreamMonitorTest;
+  friend class AudibleClientRegistration;
 
   enum {
     // Minimum amount of time to hold a tab indicator on after it becomes
@@ -117,7 +129,9 @@ class CONTENT_EXPORT AudioStreamMonitor : public WebContentsObserver {
   void MaybeToggle();
   void UpdateStreams();
 
-  // void OnStreamRemoved();
+  // Adds/Removes Audible clients.
+  void AddAudibleClient();
+  void RemoveAudibleClient();
 
   // The WebContents instance to receive indicator toggle notifications.  This
   // pointer should be valid for the lifetime of AudioStreamMonitor.

content/browser/media/audio_stream_monitor_unittest.cc

@@ -403,11 +403,11 @@ TEST_F(AudioStreamMonitorTest, OneAudibleClient) {
 
   ExpectRecentlyAudibleChangeNotification(true);
   ExpectCurrentlyAudibleChangeNotification(true);
-  monitor_->AddAudibleClient();
+  auto registration = monitor_->RegisterAudibleClient();
   ExpectIsCurrentlyAudible();
 
   ExpectCurrentlyAudibleChangeNotification(false);
-  monitor_->RemoveAudibleClient();
+  registration.reset();
   ExpectNotCurrentlyAudible();
 }
 
@@ -417,20 +417,20 @@ TEST_F(AudioStreamMonitorTest, MultipleAudibleClients) {
   // Add one client and the tab becomes audible.
   ExpectRecentlyAudibleChangeNotification(true);
   ExpectCurrentlyAudibleChangeNotification(true);
-  monitor_->AddAudibleClient();
+  auto registration1 = monitor_->RegisterAudibleClient();
   ExpectIsCurrentlyAudible();
 
   // Add another client and the tab remains audible.
-  monitor_->AddAudibleClient();
+  auto registration2 = monitor_->RegisterAudibleClient();
   ExpectIsCurrentlyAudible();
 
   // Removes one client and the tab remains audible.
-  monitor_->RemoveAudibleClient();
+  registration1.reset();
   ExpectIsCurrentlyAudible();
 
   // Removes another client and the tab is not audible.
   ExpectCurrentlyAudibleChangeNotification(false);
-  monitor_->RemoveAudibleClient();
+  registration2.reset();
   ExpectNotCurrentlyAudible();
 }
 
@@ -441,15 +441,15 @@ TEST_F(AudioStreamMonitorTest, AudibleClientAndStream) {
   // Add one client and the tab becomes audible.
   ExpectRecentlyAudibleChangeNotification(true);
   ExpectCurrentlyAudibleChangeNotification(true);
-  monitor_->AddAudibleClient();
+  auto registration = monitor_->RegisterAudibleClient();
   ExpectIsCurrentlyAudible();
 
   // The stream becomes audible and the tab remains audible.
   UpdateAudibleState(kRenderProcessId, kRenderFrameId, kStreamId, true);
   ExpectIsCurrentlyAudible();
 
   // Remove the client and the tab remains audible.
-  monitor_->RemoveAudibleClient();
+  registration.reset();
   ExpectIsCurrentlyAudible();
 
   // The stream becomes not audible and the tab is not audible.

content/browser/media/media_web_contents_observer.cc

@@ -14,7 +14,6 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "content/browser/media/audible_metrics.h"
-#include "content/browser/media/audio_stream_monitor.h"
 #include "content/browser/media/media_devices_util.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
@@ -318,13 +317,7 @@ MediaWebContentsObserver::MediaPlayerObserverHostImpl::
       media_web_contents_observer_(media_web_contents_observer) {}
 
 MediaWebContentsObserver::MediaPlayerObserverHostImpl::
-    ~MediaPlayerObserverHostImpl() {
-  if (audible_client_added_) {
-    media_web_contents_observer_->web_contents_impl()
-        ->audio_stream_monitor()
-        ->RemoveAudibleClient();
-  }
-}
+    ~MediaPlayerObserverHostImpl() = default;
 
 void MediaWebContentsObserver::MediaPlayerObserverHostImpl::
     BindMediaPlayerObserverReceiver(
@@ -460,12 +453,10 @@ void MediaWebContentsObserver::MediaPlayerObserverHostImpl::
   auto* audio_stream_monitor =
       media_web_contents_observer_->web_contents_impl()->audio_stream_monitor();
 
-  if (should_add_client && !audible_client_added_) {
-    audio_stream_monitor->AddAudibleClient();
-    audible_client_added_ = true;
-  } else if (!should_add_client && audible_client_added_) {
-    audio_stream_monitor->RemoveAudibleClient();
-    audible_client_added_ = false;
+  if (should_add_client && !audio_client_registration_) {
+    audio_client_registration_ = audio_stream_monitor->RegisterAudibleClient();
+  } else if (!should_add_client && audio_client_registration_) {
+    audio_client_registration_.reset();
   }
 }
 

content/browser/media/media_web_contents_observer.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
+#include "content/browser/media/audio_stream_monitor.h"
 #include "content/browser/media/media_power_experiment_manager.h"
 #include "content/browser/media/session/media_session_controllers_manager.h"
 #include "content/common/content_export.h"
@@ -215,7 +216,8 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
 
     // Helps monitor audio stream when not using AudioService.
     bool uses_audio_service_ = true;
-    bool audible_client_added_ = false;
+    std::unique_ptr<AudioStreamMonitor::AudibleClientRegistration>
+        audio_client_registration_;
   };
 
   using MediaPlayerHostImplMap =