Implement media::mojom::MediaPlayer::AddMediaPlayerObserver()

As agreed on [1], media::mojom::MediaPlayer::SetMediaPlayerObserver()
is not the right method to expose in that interface since it should
be possible to register more than one observer for MediaPlayer related
events outside of the renderer process, so this CL migrates that old
method into a new AddMediaPlayerObserver() one.

[1] https://crrev.com/c/2526440/21#message-2189f6a0f6328b9cb01e5be9b4fae80ec7e1f080

Bug: 1039252
Change-Id: Ifb55f6c6eb896de02608df98654bc62ff7e7ee17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595049
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838690}

content/browser/media/media_web_contents_observer.cc

@@ -599,19 +599,6 @@ void MediaWebContentsObserver::BindMediaPlayerHost(
       std::move(player_receiver));
 }
 
-void MediaWebContentsObserver::SetMediaPlayerObserverForMediaPlayer(
-    const MediaPlayerId& player_id) {
-  if (!media_player_observer_hosts_.contains(player_id)) {
-    media_player_observer_hosts_[player_id] =
-        std::make_unique<MediaPlayerObserverHostImpl>(player_id, this);
-  }
-
-  DCHECK(media_player_remotes_[player_id]);
-  media_player_remotes_[player_id]->SetMediaPlayerObserver(
-      media_player_observer_hosts_[player_id]
-          ->BindMediaPlayerObserverReceiverAndPassRemote());
-}
-
 void MediaWebContentsObserver::OnMediaPlayerAdded(
     mojo::PendingRemote<media::mojom::MediaPlayer> player_remote,
     MediaPlayerId player_id) {
@@ -628,9 +615,16 @@ void MediaWebContentsObserver::OnMediaPlayerAdded(
       },
       base::Unretained(this), player_id));
 
-  // Create a new MediaPlayerObserverHostImpl to be able to receive messages
-  // from the renderer process via the MediaPlayerObserver mojo interface.
-  SetMediaPlayerObserverForMediaPlayer(player_id);
+  // Create a new MediaPlayerObserverHostImpl for |player_id|, implementing the
+  // media::mojom::MediaPlayerObserver mojo interface, to handle messages sent
+  // from the MediaPlayer element in the renderer process.
+  if (!media_player_observer_hosts_.contains(player_id)) {
+    media_player_observer_hosts_[player_id] =
+        std::make_unique<MediaPlayerObserverHostImpl>(player_id, this);
+  }
+  media_player_remotes_[player_id]->AddMediaPlayerObserver(
+      media_player_observer_hosts_[player_id]
+          ->BindMediaPlayerObserverReceiverAndPassRemote());
 }
 
 #if defined(OS_ANDROID)

content/browser/media/media_web_contents_observer.h

@@ -122,10 +122,6 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
       RenderFrameHost* host,
       mojo::PendingReceiver<media::mojom::MediaPlayerHost> player_receiver);
 
-  // Establishes a MediaPlayerObserver for |player_id|, allowing the MediaPlayer
-  // element in the renderer process to communicate back with the browser.
-  void SetMediaPlayerObserverForMediaPlayer(const MediaPlayerId& player_id);
-
   // Communicates with the MediaSessionControllerManager to find or create (if
   // needed) a MediaSessionController identified by |player_id|, in order to
   // bind its mojo remote for media::mojom::MediaPlayer.

content/browser/media/session/media_session_controller_unittest.cc

@@ -82,7 +82,7 @@ class MockMediaPlayerReceiverForTesting : public media::mojom::MediaPlayer {
   }
 
   // media::mojom::MediaPlayer implementation.
-  void SetMediaPlayerObserver(
+  void AddMediaPlayerObserver(
       mojo::PendingRemote<media::mojom::MediaPlayerObserver>) override {}
 
   void RequestPlay() override {

content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc

@@ -113,7 +113,7 @@ class PictureInPictureMediaPlayerReceiver : public media::mojom::MediaPlayer {
   }
 
   // media::mojom::MediaPlayer implementation.
-  void SetMediaPlayerObserver(
+  void AddMediaPlayerObserver(
       mojo::PendingRemote<media::mojom::MediaPlayerObserver>) override {}
   void RequestPlay() override {}
   void RequestPause(bool triggered_by_user) override {}

media/mojo/mojom/media_player.mojom

@@ -12,9 +12,7 @@ import "ui/gfx/geometry/mojom/geometry.mojom";
 interface MediaPlayer {
   // Sends |observer| to the renderer process so that it can be established a
   // communication channel with the implementor of MediaPlayerObserver.
-  // TODO(crbug.com/1039252): Make this an AddMediaPlayerObserver method as soon
-  // as there is a HeapMojoRemoteSet class available for its use in Blink.
-  SetMediaPlayerObserver(pending_remote<MediaPlayerObserver> observer);
+  AddMediaPlayerObserver(pending_remote<MediaPlayerObserver> observer);
 
   // Requests the media player to start or resume media playback.
   RequestPlay();

third_party/blink/renderer/core/html/media/html_media_element.cc

@@ -536,7 +536,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tag_name,
       controls_list_(MakeGarbageCollected<HTMLMediaElementControlsList>(this)),
       lazy_load_intersection_observer_(nullptr),
       media_player_host_remote_(GetExecutionContext()),
-      media_player_observer_remote_(GetExecutionContext()),
+      media_player_observer_remote_set_(GetExecutionContext()),
       media_player_receiver_set_(this, GetExecutionContext()) {
   DVLOG(1) << "HTMLMediaElement(" << *this << ")";
 
@@ -635,13 +635,6 @@ bool HTMLMediaElement::IsMouseFocusable() const {
   return !IsFullscreen() && SupportsFocus();
 }
 
-media::mojom::blink::MediaPlayerObserver*
-HTMLMediaElement::GetMediaPlayerObserverRemote() {
-  if (!media_player_observer_remote_.is_bound())
-    return nullptr;
-  return media_player_observer_remote_.get();
-}
-
 void HTMLMediaElement::ParseAttribute(
     const AttributeModificationParams& params) {
   const QualifiedName& name = params.name;
@@ -1469,9 +1462,9 @@ bool HTMLMediaElement::PausedWhenVisible() const {
          !GetWebMediaPlayer()->PausedWhenHidden();
 }
 
-void HTMLMediaElement::SetMediaPlayerObserverForTesting(
+void HTMLMediaElement::AddMediaPlayerObserverForTesting(
     mojo::PendingRemote<media::mojom::blink::MediaPlayerObserver> observer) {
-  SetMediaPlayerObserver(std::move(observer));
+  AddMediaPlayerObserver(std::move(observer));
 }
 
 bool HTMLMediaElement::TextTracksAreReady() const {
@@ -3674,7 +3667,7 @@ void HTMLMediaElement::
     // The lifetime of the mojo endpoints are tied to the WebMediaPlayer's, so
     // we need to reset those as well.
     media_player_receiver_set_.Clear();
-    media_player_observer_remote_.reset();
+    media_player_observer_remote_set_.Clear();
   }
   OnWebMediaPlayerCleared();
 }
@@ -4150,7 +4143,7 @@ void HTMLMediaElement::Trace(Visitor* visitor) const {
   visitor->Trace(controls_list_);
   visitor->Trace(lazy_load_intersection_observer_);
   visitor->Trace(media_player_host_remote_);
-  visitor->Trace(media_player_observer_remote_);
+  visitor->Trace(media_player_observer_remote_set_);
   visitor->Trace(media_player_receiver_set_);
   Supplementable<HTMLMediaElement>::Trace(visitor);
   HTMLElement::Trace(visitor);
@@ -4392,61 +4385,61 @@ void HTMLMediaElement::PausePlayback() {
 }
 
 void HTMLMediaElement::DidPlayerMutedStatusChange(bool muted) {
-  // The remote to the MediaPlayerObserver could be not set yet.
-  if (!media_player_observer_remote_.is_bound())
-    return;
-
-  media_player_observer_remote_->OnMutedStatusChanged(muted);
+  for (auto& observer : media_player_observer_remote_set_) {
+    if (!observer.is_bound())
+      continue;
+    observer->OnMutedStatusChanged(muted);
+  }
 }
 
 void HTMLMediaElement::DidPlayerMediaPositionStateChange(
     double playback_rate,
     base::TimeDelta duration,
     base::TimeDelta position) {
-  // The remote to the MediaPlayerObserver could be not set yet.
-  if (!media_player_observer_remote_.is_bound())
-    return;
-
-  media_player_observer_remote_->OnMediaPositionStateChanged(
-      media_session::mojom::blink::MediaPosition::New(
-          playback_rate, duration, position, base::TimeTicks::Now()));
+  for (auto& observer : media_player_observer_remote_set_) {
+    if (!observer.is_bound())
+      continue;
+    observer->OnMediaPositionStateChanged(
+        media_session::mojom::blink::MediaPosition::New(
+            playback_rate, duration, position, base::TimeTicks::Now()));
+  }
 }
 
 void HTMLMediaElement::DidDisableAudioOutputSinkChanges() {
-  // The remote to the MediaPlayerObserver could be not set yet.
-  if (!media_player_observer_remote_.is_bound())
-    return;
-
-  media_player_observer_remote_->OnAudioOutputSinkChangingDisabled();
+  for (auto& observer : media_player_observer_remote_set_) {
+    if (!observer.is_bound())
+      continue;
+    observer->OnAudioOutputSinkChangingDisabled();
+  }
 }
 
 void HTMLMediaElement::DidPlayerSizeChange(const gfx::Size& size) {
-  // The remote to the MediaPlayerObserver could be not set yet.
-  if (!media_player_observer_remote_.is_bound())
-    return;
-
-  media_player_observer_remote_->OnMediaSizeChanged(size);
+  for (auto& observer : media_player_observer_remote_set_) {
+    if (!observer.is_bound())
+      continue;
+    observer->OnMediaSizeChanged(size);
+  }
 }
 
 void HTMLMediaElement::DidBufferUnderflow() {
-  // The remote to the MediaPlayerObserver could be not set yet.
-  if (!media_player_observer_remote_.is_bound())
-    return;
-
-  media_player_observer_remote_->OnBufferUnderflow();
+  for (auto& observer : media_player_observer_remote_set_) {
+    if (!observer.is_bound())
+      continue;
+    observer->OnBufferUnderflow();
+  }
 }
 
 void HTMLMediaElement::DidSeek() {
-  // The remote to the MediaPlayerObserver could be not set yet.
-  if (!media_player_observer_remote_.is_bound())
-    return;
-
   // Send the seek updates to the browser process only once per second.
   if (last_seek_update_time_.is_null() ||
       (base::TimeTicks::Now() - last_seek_update_time_ >=
        base::TimeDelta::FromSeconds(1))) {
     last_seek_update_time_ = base::TimeTicks::Now();
-    media_player_observer_remote_->OnSeek();
+    for (auto& observer : media_player_observer_remote_set_) {
+      if (!observer.is_bound())
+        continue;
+      observer->OnSeek();
+    }
   }
 }
 
@@ -4462,12 +4455,18 @@ HTMLMediaElement::GetMediaPlayerHostRemote() {
   return *media_player_host_remote_.get();
 }
 
-void HTMLMediaElement::SetMediaPlayerObserver(
+void HTMLMediaElement::AddMediaPlayerObserver(
     mojo::PendingRemote<media::mojom::blink::MediaPlayerObserver> observer) {
-  DCHECK(!media_player_observer_remote_.is_bound());
-  media_player_observer_remote_.Bind(
+  media_player_observer_remote_set_.Add(
       std::move(observer),
       GetDocument().GetTaskRunner(TaskType::kInternalMedia));
+
+  media_player_observer_remote_set_.set_disconnect_handler(WTF::BindRepeating(
+      [](HTMLMediaElement* html_media_element,
+         mojo::RemoteSetElementId remote_id) {
+        html_media_element->media_player_observer_remote_set_.Remove(remote_id);
+      },
+      WrapWeakPersistent(this)));
 }
 
 void HTMLMediaElement::RequestPlay() {

third_party/blink/renderer/core/html/media/html_media_element.h

@@ -48,6 +48,7 @@
 #include "third_party/blink/renderer/platform/media/web_audio_source_provider_client.h"
 #include "third_party/blink/renderer/platform/mojo/heap_mojo_receiver_set.h"
 #include "third_party/blink/renderer/platform/mojo/heap_mojo_remote.h"
+#include "third_party/blink/renderer/platform/mojo/heap_mojo_remote_set.h"
 #include "third_party/blink/renderer/platform/network/mime/mime_type_registry.h"
 #include "third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h"
 #include "third_party/blink/renderer/platform/supplementable.h"
@@ -349,7 +350,7 @@ class CORE_EXPORT HTMLMediaElement
   void SetCcLayerForTesting(cc::Layer* layer) { SetCcLayer(layer); }
 
   // Required by tests set mock receivers to check that messages are delivered.
-  void SetMediaPlayerObserverForTesting(
+  void AddMediaPlayerObserverForTesting(
       mojo::PendingRemote<media::mojom::blink::MediaPlayerObserver> observer);
 
   bool IsShowPosterFlagSet() const { return show_poster_flag_; }
@@ -362,9 +363,13 @@ class CORE_EXPORT HTMLMediaElement
   ~HTMLMediaElement() override;
   void Dispose();
 
-  // Returns a pointer to the media::mojom::blink::MediaPlayerObserver remote if
-  // already bound, or nullptr otherwise. Used from subclasses as well.
-  media::mojom::blink::MediaPlayerObserver* GetMediaPlayerObserverRemote();
+  // Returns a constant reference to the HeapMojoRemoteSet holding all the bound
+  // remotes for the media::mojom::blink::MediaPlayerObserver interface. Needed
+  // to allow sending messages directly from HTMLMediaElement's subclasses.
+  const HeapMojoRemoteSet<media::mojom::blink::MediaPlayerObserver>&
+  GetMediaPlayerObserverRemoteSet() {
+    return media_player_observer_remote_set_;
+  }
 
   void ParseAttribute(const AttributeModificationParams&) override;
   void FinishParsingChildren() final;
@@ -486,7 +491,7 @@ class CORE_EXPORT HTMLMediaElement
   media::mojom::blink::MediaPlayerHost& GetMediaPlayerHostRemote();
 
   // media::mojom::MediaPlayer  implementation.
-  void SetMediaPlayerObserver(
+  void AddMediaPlayerObserver(
       mojo::PendingRemote<media::mojom::blink::MediaPlayerObserver> observer)
       override;
   void RequestPlay() override;
@@ -817,8 +822,11 @@ class CORE_EXPORT HTMLMediaElement
 
   HeapMojoRemote<media::mojom::blink::MediaPlayerHost>
       media_player_host_remote_;
-  HeapMojoRemote<media::mojom::blink::MediaPlayerObserver>
-      media_player_observer_remote_;
+
+  // Multiple objects outside of the renderer process can register as observers,
+  // so we need to store the remotes in a set here.
+  HeapMojoRemoteSet<media::mojom::blink::MediaPlayerObserver>
+      media_player_observer_remote_set_;
 
   // A receiver set is needed here as there will be different objects in the
   // browser communicating with this object. This is done this way to avoid

third_party/blink/renderer/core/html/media/html_media_element_test.cc

@@ -85,7 +85,7 @@ class WebMediaStubLocalFrameClient : public EmptyLocalFrameClient {
 // interface to allow checking that messages sent over mojo are received with
 // the right values in the other end.
 //
-// Note this relies on HTMLMediaElement::SetMediaPlayerObserverForTesting() to
+// Note this relies on HTMLMediaElement::AddMediaPlayerObserverForTesting() to
 // provide the HTMLMediaElement instance owned by the test with a valid mojo
 // remote, that will be bound to the mojo receiver provided by this class
 // instead of the real one used in production that would be owned by
@@ -97,7 +97,7 @@ class MockMediaPlayerObserverReceiverForTesting
       HTMLMediaElement* html_media_element) {
     // Bind the remote to the receiver, so that we can intercept incoming
     // messages sent via the different methods that use the remote.
-    html_media_element->SetMediaPlayerObserverForTesting(
+    html_media_element->AddMediaPlayerObserverForTesting(
         receiver_.BindNewPipeAndPassRemote());
   }
 

third_party/blink/renderer/core/html/media/html_video_element.cc

@@ -259,10 +259,10 @@ void HTMLVideoElement::UpdatePictureInPictureAvailability() {
   if (!web_media_player_)
     return;
 
-  auto* media_player_observer_remote = GetMediaPlayerObserverRemote();
-  if (media_player_observer_remote) {
-    media_player_observer_remote->OnPictureInPictureAvailabilityChanged(
-        SupportsPictureInPicture());
+  for (auto& observer : GetMediaPlayerObserverRemoteSet()) {
+    if (!observer.is_bound())
+      continue;
+    observer->OnPictureInPictureAvailabilityChanged(SupportsPictureInPicture());
   }
 }