Bug 1632301 - part1 : decouple ContentMediaController from MediaControlKeysEventListener/MediaControlKeysEventSource. r=bryce
authoralwu <alwu@mozilla.com>
Tue, 28 Apr 2020 05:10:03 +0000
changeset 526380 48ef712d80172a1cc578bc1a4ba529490ab70b5c
parent 526379 064453fc2dac1435319bf0441d442a614137e0f3
child 526381 636b863fce4b5370f9b306b56b7ffee0257d68e4
push id37356
push useraiakab@mozilla.com
push dateTue, 28 Apr 2020 16:30:11 +0000
treeherdermozilla-central@5797e768d878 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbryce
bugs1632301
milestone77.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1632301 - part1 : decouple ContentMediaController from MediaControlKeysEventListener/MediaControlKeysEventSource. r=bryce This patch will do : - remove the inheritance relationship for `ContentControlKeyEventReceiver` and `ContentMediaAgent` and manually implement the methods we need - `MediaControlEventListener` will inherit from `ContentControlKeyEventReceiver` directly The advantage of doing so : - increase the flexibilty of modification of `ContentMediaAgent` and those modification won't affect other classes inherited from `MediaControlKeysEventListener` --- More details about this change : As we would like to extend the class `ContentMediaAgent` and allow the `ContentMediaController` to call its extended method, but if we want to do so in current implementation, the extended method would also affect other classes inherited from `MediaControlKeysEventListener` and that is something we don't want to see. Considering that, I decided to decouple the inheritance relationship and manually create the function I need (which will be implemented in the next patch) Differential Revision: https://phabricator.services.mozilla.com/D72054
dom/html/HTMLMediaElement.cpp
dom/media/mediacontrol/ContentMediaController.cpp
dom/media/mediacontrol/ContentMediaController.h
dom/media/mediacontrol/PlaybackController.cpp
--- a/dom/html/HTMLMediaElement.cpp
+++ b/dom/html/HTMLMediaElement.cpp
@@ -384,17 +384,17 @@ class nsSourceErrorEventRunner : public 
  * play and pause media element when user press media control keys and update
  * media's playback and audible state to the media controller.
  *
  * Use `Start()` to start listening event and use `Stop()` to stop listening
  * event. In addition, notifying any change to media controller MUST be done
  * after successfully calling `Start()`.
  */
 class HTMLMediaElement::MediaControlEventListener final
-    : public MediaControlKeysEventListener {
+    : public ContentControlKeyEventReceiver {
  public:
   NS_INLINE_DECL_REFCOUNTING(MediaControlEventListener, override)
 
   MOZ_INIT_OUTSIDE_CTOR explicit MediaControlEventListener(
       HTMLMediaElement* aElement)
       : mElement(aElement) {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(aElement);
@@ -423,17 +423,17 @@ class HTMLMediaElement::MediaControlEven
     MOZ_ASSERT(NS_IsMainThread());
     if (!IsStarted()) {
       // We have already been stopped, do not notify stop twice.
       return;
     }
     NotifyMediaStateChanged(ControlledMediaState::eStopped);
 
     // Remove ourselves from media agent, which would stop receiving event.
-    mControlAgent->RemoveListener(this);
+    mControlAgent->RemoveReceiver(this);
     mControlAgent = nullptr;
   }
 
   bool IsStarted() const { return mState != ControlledMediaState::eStopped; }
 
   /**
    * Following methods should only be used after starting listener.
    */
@@ -487,20 +487,20 @@ class HTMLMediaElement::MediaControlEven
     if (mIsPictureInPictureEnabled == aIsEnabled) {
       return;
     }
     mIsPictureInPictureEnabled = aIsEnabled;
     mControlAgent->NotifyPictureInPictureModeChanged(
         this, mIsPictureInPictureEnabled);
   }
 
-  void OnKeyPressed(MediaControlKeysEvent aEvent) override {
+  void HandleEvent(MediaControlKeysEvent aEvent) override {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(IsStarted());
-    MEDIACONTROL_LOG("OnKeyPressed '%s'", ToMediaControlKeysEventStr(aEvent));
+    MEDIACONTROL_LOG("HandleEvent '%s'", ToMediaControlKeysEventStr(aEvent));
     if (aEvent == MediaControlKeysEvent::ePlay && Owner()->Paused()) {
       Owner()->Play();
     } else if ((aEvent == MediaControlKeysEvent::ePause ||
                 aEvent == MediaControlKeysEvent::eStop) &&
                !Owner()->Paused()) {
       Owner()->Pause();
     }
   }
@@ -514,17 +514,17 @@ class HTMLMediaElement::MediaControlEven
     if (!window) {
       return false;
     }
 
     mControlAgent = ContentMediaAgent::Get(window->GetBrowsingContext());
     if (!mControlAgent) {
       return false;
     }
-    mControlAgent->AddListener(this);
+    mControlAgent->AddReceiver(this);
     return true;
   }
 
   HTMLMediaElement* Owner() const {
     MOZ_ASSERT(mElement);
     return mElement.get();
   }
 
--- a/dom/media/mediacontrol/ContentMediaController.cpp
+++ b/dom/media/mediacontrol/ContentMediaController.cpp
@@ -69,37 +69,38 @@ ContentMediaAgent* ContentMediaAgent::Ge
       GetContentMediaControllerFromBrowsingContext(aBC);
   return controller ? static_cast<ContentMediaAgent*>(controller.get())
                     : nullptr;
 }
 
 ContentMediaController::ContentMediaController(uint64_t aId)
     : mTopLevelBrowsingContextId(aId) {}
 
-void ContentMediaController::AddListener(
-    MediaControlKeysEventListener* aListener) {
+void ContentMediaController::AddReceiver(
+    ContentControlKeyEventReceiver* aListener) {
   MOZ_ASSERT(NS_IsMainThread());
-  ContentMediaAgent::AddListener(aListener);
+  mReceivers.AppendElement(aListener);
 }
 
-void ContentMediaController::RemoveListener(
-    MediaControlKeysEventListener* aListener) {
+void ContentMediaController::RemoveReceiver(
+    ContentControlKeyEventReceiver* aListener) {
   MOZ_ASSERT(NS_IsMainThread());
-  ContentMediaAgent::RemoveListener(aListener);
+  mReceivers.RemoveElement(aListener);
   // No more media needs to be controlled, so we can release this and recreate
-  // it when someone needs it.
-  if (mListeners.IsEmpty()) {
-    Close();
+  // it when someone needs it. We have to check `sControllers` because this can
+  // be called via CC after we clear `sControllers`.
+  if (mReceivers.IsEmpty() && sControllers) {
+    sControllers->Remove(mTopLevelBrowsingContextId);
   }
 }
 
 void ContentMediaController::NotifyMediaStateChanged(
-    const MediaControlKeysEventListener* aMedia, ControlledMediaState aState) {
+    const ContentControlKeyEventReceiver* aMedia, ControlledMediaState aState) {
   MOZ_ASSERT(NS_IsMainThread());
-  if (!mListeners.Contains(aMedia)) {
+  if (!mReceivers.Contains(aMedia)) {
     return;
   }
 
   RefPtr<BrowsingContext> bc = GetTopLevelBrowsingContext();
   if (!bc || bc->IsDiscarded()) {
     return;
   }
 
@@ -114,19 +115,19 @@ void ContentMediaController::NotifyMedia
     if (RefPtr<MediaController> controller =
             bc->Canonical()->GetMediaController()) {
       controller->NotifyMediaStateChanged(aState);
     }
   }
 }
 
 void ContentMediaController::NotifyAudibleStateChanged(
-    const MediaControlKeysEventListener* aMedia, bool aAudible) {
+    const ContentControlKeyEventReceiver* aMedia, bool aAudible) {
   MOZ_ASSERT(NS_IsMainThread());
-  if (!mListeners.Contains(aMedia)) {
+  if (!mReceivers.Contains(aMedia)) {
     return;
   }
 
   RefPtr<BrowsingContext> bc = GetTopLevelBrowsingContext();
   if (!bc || bc->IsDiscarded()) {
     return;
   }
 
@@ -141,19 +142,19 @@ void ContentMediaController::NotifyAudib
     if (RefPtr<MediaController> controller =
             bc->Canonical()->GetMediaController()) {
       controller->NotifyMediaAudibleChanged(aAudible);
     }
   }
 }
 
 void ContentMediaController::NotifyPictureInPictureModeChanged(
-    const MediaControlKeysEventListener* aMedia, bool aEnabled) {
+    const ContentControlKeyEventReceiver* aMedia, bool aEnabled) {
   MOZ_ASSERT(NS_IsMainThread());
-  if (!mListeners.Contains(aMedia)) {
+  if (!mReceivers.Contains(aMedia)) {
     return;
   }
 
   RefPtr<BrowsingContext> bc = GetTopLevelBrowsingContext();
   if (!bc || bc->IsDiscarded()) {
     return;
   }
 
@@ -167,32 +168,22 @@ void ContentMediaController::NotifyPictu
     // media would be run in the content process.
     if (RefPtr<MediaController> controller =
             bc->Canonical()->GetMediaController()) {
       controller->SetIsInPictureInPictureMode(aEnabled);
     }
   }
 }
 
-void ContentMediaController::OnKeyPressed(MediaControlKeysEvent aEvent) {
+void ContentMediaController::HandleEvent(MediaControlKeysEvent aEvent) {
   MOZ_ASSERT(NS_IsMainThread());
-  LOG("Handle '%s' event, listener num=%zu", ToMediaControlKeysEventStr(aEvent),
-      mListeners.Length());
-  for (auto& listener : mListeners) {
-    listener->OnKeyPressed(aEvent);
-  }
-}
-
-void ContentMediaController::Close() {
-  MOZ_ASSERT(NS_IsMainThread());
-  MediaControlKeysEventSource::Close();
-  // `sControllers` might be null if ContentMediaController is detroyed after
-  // freeing `sControllers`.
-  if (sControllers) {
-    sControllers->Remove(mTopLevelBrowsingContextId);
+  LOG("Handle '%s' event, receiver num=%zu", ToMediaControlKeysEventStr(aEvent),
+      mReceivers.Length());
+  for (auto& receiver : mReceivers) {
+    receiver->HandleEvent(aEvent);
   }
 }
 
 already_AddRefed<BrowsingContext>
 ContentMediaController::GetTopLevelBrowsingContext() const {
   if (!sControllers) {
     // `sControllers` would be destroyed when XPCOM is shutdown, which means
     // we are not able to access browsing context anymore.
--- a/dom/media/mediacontrol/ContentMediaController.h
+++ b/dom/media/mediacontrol/ContentMediaController.h
@@ -29,78 +29,83 @@ class BrowsingContext;
 enum class ControlledMediaState : uint32_t {
   eStarted,
   ePlayed,
   ePaused,
   eStopped,
 };
 
 /**
+ * ContentControlKeyEventReceiver is an interface which is used to receive media
+ * control key events sent from the chrome process, this class MUST only be used
+ * in PlaybackController.
+ *
+ * Each browsing context tree would only have one ContentControlKeyEventReceiver
+ * that is used to handle media control key events for that browsing context
+ * tree.
+ */
+class ContentControlKeyEventReceiver {
+ public:
+  NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING
+
+  // Return nullptr if the top level browsing context is no longer alive.
+  static ContentControlKeyEventReceiver* Get(BrowsingContext* aBC);
+
+  // Use this method to handle the event from `ContentMediaAgent`.
+  virtual void HandleEvent(MediaControlKeysEvent aKeyEvent) = 0;
+};
+
+/**
  * ContentMediaAgent is an interface which we use to (1) update controlled media
  * status to the media controller in the chrome process and (2) act an event
  * source to dispatch control key events to its listeners.
  *
  * If the media would like to know the media control key events, then media
- * MUST inherit from MediaControlKeysEventListener, and register themselves to
+ * MUST inherit from ContentControlKeyEventReceiver, and register themselves to
  * ContentMediaAgent. Whenever media key events occur, ContentMediaAgent would
- * notify all its listeners. In addition, whenever controlled media changes its
+ * notify all its receivers. In addition, whenever controlled media changes its
  * playback status or audible state, they should update their status update via
  * ContentMediaAgent.
  *
  * Each browsing context tree would only have one ContentMediaAgent that is used
  * to update controlled media status existing in that browsing context tree.
  */
-class ContentMediaAgent : public MediaControlKeysEventSource {
+class ContentMediaAgent {
  public:
+  NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING
+
   // Return nullptr if the top level browsing context is no longer alive.
   static ContentMediaAgent* Get(BrowsingContext* aBC);
 
   // Use this method to update the media playback state of controlled media, and
   // MUST follow the rule of ControlledMediaState.
   virtual void NotifyMediaStateChanged(
-      const MediaControlKeysEventListener* aMedia,
+      const ContentControlKeyEventReceiver* aMedia,
       ControlledMediaState aState) = 0;
 
   // Use this method to update the audible state of controlled media, and MUST
   // follow the following rules in which `audible` and `inaudible` should be a
   // pair. `inaudible` should always be notified after `audible`. When audible
   // media paused, `inaudible` should be notified
   // Eg. (O) `audible` -> `inaudible` -> `audible` -> `inaudible`
   //     (X) `inaudible` -> `audible`    [notify `inaudible` before `audible`]
   //     (X) `audible` -> `audible`      [notify `audible` twice]
   //     (X) `audible` -> (media pauses) [forgot to notify `inaudible`]
   virtual void NotifyAudibleStateChanged(
-      const MediaControlKeysEventListener* aMedia, bool aAudible) = 0;
+      const ContentControlKeyEventReceiver* aMedia, bool aAudible) = 0;
 
   // Use this method to update the picture in picture mode state of controlled
   // media, and it's safe to notify same state again.
   virtual void NotifyPictureInPictureModeChanged(
-      const MediaControlKeysEventListener* aMedia, bool aEnabled) = 0;
-
- private:
-  // We don't need them in our case, so make them private to prevent usage.
-  bool Open() override { return true; }
-  bool IsOpened() const override { return true; }
-};
+      const ContentControlKeyEventReceiver* aMedia, bool aEnabled) = 0;
 
-/**
- * ContentControlKeyEventReceiver is an interface which is used to receive media
- * control key events sent from the chrome process, this class MUST only be used
- * in PlaybackController.
- *
- * Each browsing context tree would only have one ContentControlKeyEventReceiver
- * that is used to handle media control key events for that browsing context
- * tree.
- */
-class ContentControlKeyEventReceiver : public MediaControlKeysEventListener {
- public:
-  // Return nullptr if the top level browsing context is no longer alive.
-  static ContentControlKeyEventReceiver* Get(BrowsingContext* aBC);
-
-  void OnKeyPressed(MediaControlKeysEvent aKeyEvent) override = 0;
+  // Use these methods to register/unregister `ContentControlKeyEventReceiver`
+  // in order to listen to media control key events.
+  virtual void AddReceiver(ContentControlKeyEventReceiver* aReceiver) = 0;
+  virtual void RemoveReceiver(ContentControlKeyEventReceiver* aReceiver) = 0;
 };
 
 /**
  * ContentMediaController has a responsibility to update the content media state
  * to MediaController that exists in the chrome process and control all media
  * within a tab. It also delivers control commands from MediaController in order
  * to control media in the content page.
  *
@@ -117,32 +122,32 @@ class ContentControlKeyEventReceiver : p
  */
 class ContentMediaController final : public ContentMediaAgent,
                                      public ContentControlKeyEventReceiver {
  public:
   NS_INLINE_DECL_REFCOUNTING(ContentMediaController, override)
 
   explicit ContentMediaController(uint64_t aId);
   // ContentMediaAgent methods
-  void AddListener(MediaControlKeysEventListener* aListener) override;
-  void RemoveListener(MediaControlKeysEventListener* aListener) override;
-  void NotifyMediaStateChanged(const MediaControlKeysEventListener* aMedia,
+  void AddReceiver(ContentControlKeyEventReceiver* aListener) override;
+  void RemoveReceiver(ContentControlKeyEventReceiver* aListener) override;
+  void NotifyMediaStateChanged(const ContentControlKeyEventReceiver* aMedia,
                                ControlledMediaState aState) override;
-  void NotifyAudibleStateChanged(const MediaControlKeysEventListener* aMedia,
+  void NotifyAudibleStateChanged(const ContentControlKeyEventReceiver* aMedia,
                                  bool aAudible) override;
   void NotifyPictureInPictureModeChanged(
-      const MediaControlKeysEventListener* aMedia, bool aEnabled) override;
+      const ContentControlKeyEventReceiver* aMedia, bool aEnabled) override;
 
   // ContentControlKeyEventReceiver method
-  void OnKeyPressed(MediaControlKeysEvent aEvent) override;
+  void HandleEvent(MediaControlKeysEvent aEvent) override;
 
  private:
   ~ContentMediaController() = default;
-  void Close() override;
   already_AddRefed<BrowsingContext> GetTopLevelBrowsingContext() const;
 
+  nsTArray<RefPtr<ContentControlKeyEventReceiver>> mReceivers;
   uint64_t mTopLevelBrowsingContextId;
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif  // DOM_MEDIA_MEDIACONTROL_CONTENTMEDIACONTROLLER_H_
--- a/dom/media/mediacontrol/PlaybackController.cpp
+++ b/dom/media/mediacontrol/PlaybackController.cpp
@@ -46,33 +46,33 @@ void PlaybackController::Play() {
   const MediaSessionAction action = MediaSessionAction::Play;
   RefPtr<MediaSession> session = GetMediaSession();
   if (!session || !session->IsSupportedAction(action)) {
     // Our default behavior is to play all media elements within same browsing
     // context tree.
     LOG("Handle 'play' in default behavior");
     if (RefPtr<ContentControlKeyEventReceiver> receiver =
             ContentControlKeyEventReceiver::Get(mBC)) {
-      receiver->OnKeyPressed(MediaControlKeysEvent::ePlay);
+      receiver->HandleEvent(MediaControlKeysEvent::ePlay);
     }
   } else {
     session->NotifyHandler(action);
   }
 };
 
 void PlaybackController::Pause() {
   const MediaSessionAction action = MediaSessionAction::Pause;
   RefPtr<MediaSession> session = GetMediaSession();
   if (!session || !session->IsSupportedAction(action)) {
     // Our default behavior is to pause all media elements within same browsing
     // context tree.
     LOG("Handle 'pause' in default behavior");
     if (RefPtr<ContentControlKeyEventReceiver> receiver =
             ContentControlKeyEventReceiver::Get(mBC)) {
-      receiver->OnKeyPressed(MediaControlKeysEvent::ePause);
+      receiver->HandleEvent(MediaControlKeysEvent::ePause);
     }
   } else {
     session->NotifyHandler(action);
   }
 }
 
 void PlaybackController::SeekBackward() {
   const MediaSessionAction action = MediaSessionAction::Seekbackward;
@@ -113,17 +113,17 @@ void PlaybackController::Stop() {
   RefPtr<MediaSession> session = GetMediaSession();
   if (!session || !session->IsSupportedAction(action)) {
     // Our default behavior is to stop all media elements within same browsing
     // context tree.
     LOG("Handle 'stop' in default behavior");
     RefPtr<ContentControlKeyEventReceiver> receiver =
         ContentControlKeyEventReceiver::Get(mBC);
     if (receiver) {
-      receiver->OnKeyPressed(MediaControlKeysEvent::eStop);
+      receiver->HandleEvent(MediaControlKeysEvent::eStop);
     }
   } else {
     session->NotifyHandler(action);
   }
 }
 
 void PlaybackController::SeekTo() {
   // TODO : use media session's action handler if it exists. MediaSessionAction