Bug 1663128 - part2 : let MediaControlService determine when we should open/close the event source. r=chunmin

The old way to open/close the event source, which is triggered by controller amount change event, is less intuitive, and we do the extra clean up when close the event source by assigning some parameters [1] that causes an issue on Windows where the control interface can't be clear up completely.

Each platform has its own way to clean the interface. For example, on Windows, we can simply call `ISystemMediaTransportControlsDisplayUpdater::ClearAll()`. So calling those functions actually helps nothing. The best way to do that is to ask the event source to do the clean up, rathering than setting those unnecessary parameters.

Therefore, we make it happen closer to when we determine or clear main controller and ask the event source to take a responsible to clean up when it closes.

[1] https://searchfox.org/mozilla-central/rev/35245411b9e8a911fe3f5adb0632c3394f8b4ccb/dom/media/mediacontrol/MediaControlService.cpp#410-413

Differential Revision: https://phabricator.services.mozilla.com/D92115

dom/media/mediacontrol/MediaControlKeyManager.cpp

@@ -29,21 +29,21 @@ namespace mozilla {
 namespace dom {
 
 bool MediaControlKeyManager::IsOpened() const {
-  // As MediaControlKeyManager represents a platform-indenpendent event source,
-  // which we can use to add other listeners to moniter media key events, we
-  // would always return true even if we fail to open the real media key event
-  // source, because we still have chances to open the source again when there
-  // are other new controllers being added.
-  return true;
+  return mEventSource && mEventSource->IsOpened();
 }
 
 bool MediaControlKeyManager::Open() {
-  mControllerAmountChangedListener =
-      MediaControlService::GetService()
-          ->MediaControllerAmountChangedEvent()
-          .Connect(AbstractThread::MainThread(), this,
-                   &MediaControlKeyManager::ControllerAmountChanged);
-  return true;
+  if (IsOpened()) {
+    return true;
+  }
+  return StartMonitoringControlKeys();
+}
+
+void MediaControlKeyManager::Close() {
+  // We don't call parent's `Close()` because we want to keep the listener
+  // (MediaControlKeyHandler) all the time. It would be manually removed by
+  // `MediaControlService` when shutdown.
+  StopMonitoringControlKeys();
 }
 
 MediaControlKeyManager::MediaControlKeyManager()
@@ -57,7 +57,6 @@ MediaControlKeyManager::~MediaControlKeyManager() { Shutdown(); }
 void MediaControlKeyManager::Shutdown() {
   StopMonitoringControlKeys();
   mEventSource = nullptr;
-  mControllerAmountChangedListener.DisconnectIfExists();
   if (mObserver) {
     nsContentUtils::UnregisterShutdownObserver(mObserver);
     Preferences::RemoveObserver(mObserver, MEDIA_CONTROL_PREF);
@@ -65,28 +64,25 @@ void MediaControlKeyManager::Shutdown() {
   }
 }
 
-void MediaControlKeyManager::StartMonitoringControlKeys() {
+bool MediaControlKeyManager::StartMonitoringControlKeys() {
   if (!StaticPrefs::media_hardwaremediakeys_enabled()) {
-    return;
+    return false;
   }
 
   if (!mEventSource) {
     mEventSource = widget::CreateMediaControlKeySource();
   }
-
-  // When cross-compiling with MinGW, we cannot use the related WinAPI, thus
-  // mEventSource might be null there.
-  if (!mEventSource) {
-    return;
-  }
-
-  if (!mEventSource->IsOpened() && mEventSource->Open()) {
+  if (mEventSource && mEventSource->Open()) {
     LOG_INFO("StartMonitoringControlKeys");
     mEventSource->SetPlaybackState(mPlaybackState);
     mEventSource->SetMediaMetadata(mMetadata);
     mEventSource->SetSupportedMediaKeys(mSupportedKeys);
     mEventSource->AddListener(this);
+    return true;
   }
+  // Fail to open or create event source (eg. when cross-compiling with MinGW,
+  // we cannot use the related WinAPI)
+  return false;
 }
 
 void MediaControlKeyManager::StopMonitoringControlKeys() {
@@ -96,16 +92,6 @@ void MediaControlKeyManager::StopMonitoringControlKeys() {
   }
 }
 
-void MediaControlKeyManager::ControllerAmountChanged(
-    uint64_t aControllerAmount) {
-  LOG("Controller amount changed=%" PRId64, aControllerAmount);
-  if (aControllerAmount > 0) {
-    StartMonitoringControlKeys();
-  } else if (aControllerAmount == 0) {
-    StopMonitoringControlKeys();
-  }
-}
-
 void MediaControlKeyManager::OnActionPerformed(
     const MediaControlAction& aAction) {
   for (auto listener : mListeners) {
@@ -197,7 +183,7 @@ void MediaControlKeyManager::OnPreferenceChange() {
   LOG_INFO("Preference change : %s media control",
            isPrefEnabled ? "enable" : "disable");
   if (shouldMonitorKeys) {
-    StartMonitoringControlKeys();
+    Unused << StartMonitoringControlKeys();
   } else {
     StopMonitoringControlKeys();
   }

dom/media/mediacontrol/MediaControlKeyManager.h

@@ -29,6 +29,7 @@ class MediaControlKeyManager final : public MediaControlKeySource,
 
   // MediaControlKeySource methods
   bool Open() override;
+  void Close() override;
   bool IsOpened() const override;
 
   void SetPlaybackState(MediaSessionPlaybackState aState) override;
@@ -37,10 +38,6 @@ class MediaControlKeyManager final : public MediaControlKeySource,
   // MediaControlKeyListener methods
   void OnActionPerformed(const MediaControlAction& aAction) override;
 
-  // The callback function for monitoring the media controller amount changed
-  // event.
-  void ControllerAmountChanged(uint64_t aControllerAmount);
-
   void SetMediaMetadata(const MediaMetadataBase& aMetadata) override;
   void SetSupportedMediaKeys(const MediaKeysArray& aSupportedKeys) override;
   void SetEnableFullScreen(bool aIsEnabled) override;
@@ -65,10 +62,9 @@ class MediaControlKeyManager final : public MediaControlKeySource,
   RefPtr<Observer> mObserver;
   void OnPreferenceChange();
 
-  void StartMonitoringControlKeys();
+  bool StartMonitoringControlKeys();
   void StopMonitoringControlKeys();
   RefPtr<MediaControlKeySource> mEventSource;
-  MediaEventListener mControllerAmountChangedListener;
   MediaMetadataBase mMetadata;
   nsTArray<MediaControlKey> mSupportedKeys;
 };

dom/media/mediacontrol/MediaControlService.cpp

@@ -109,8 +109,6 @@ MediaControlService::MediaControlService() {
 void MediaControlService::Init() {
   mMediaKeysHandler = new MediaControlKeyHandler();
   mMediaControlKeyManager = new MediaControlKeyManager();
-  mMediaControlKeyManager->Open();
-  MOZ_ASSERT(mMediaControlKeyManager->IsOpened());
   mMediaControlKeyManager->AddListener(mMediaKeysHandler.get());
   mControllerManager = MakeUnique<ControllerManager>(this);
 
@@ -170,7 +168,6 @@ bool MediaControlService::RegisterActiveMediaController(
   }
   LOG("Register media controller %" PRId64 ", currentNum=%" PRId64,
       aController->Id(), GetActiveControllersNum());
-  mMediaControllerAmountChangedEvent.Notify(GetActiveControllersNum());
   if (StaticPrefs::media_mediacontrol_testingevents_enabled()) {
     if (nsCOMPtr<nsIObserverService> obs = services::GetObserverService()) {
       obs->NotifyObservers(nullptr, "media-controller-amount-changed", nullptr);
@@ -189,7 +186,6 @@ bool MediaControlService::UnregisterActiveMediaController(
   }
   LOG("Unregister media controller %" PRId64 ", currentNum=%" PRId64,
       aController->Id(), GetActiveControllersNum());
-  mMediaControllerAmountChangedEvent.Notify(GetActiveControllersNum());
   if (StaticPrefs::media_mediacontrol_testingevents_enabled()) {
     if (nsCOMPtr<nsIObserverService> obs = services::GetObserverService()) {
       obs->NotifyObservers(nullptr, "media-controller-amount-changed", nullptr);
@@ -407,6 +403,7 @@ void MediaControlService::ControllerManager::UpdateMainControllerInternal(
 
   if (!mMainController) {
     LOG_MAINCONTROLLER_INFO("Clear main controller");
+    mSource->Close();
     mSource->SetPlaybackState(MediaSessionPlaybackState::None);
     mSource->SetMediaMetadata(MediaMetadataBase::EmptyData());
     mSource->SetSupportedMediaKeys(MediaKeysArray());
@@ -414,10 +411,14 @@ void MediaControlService::ControllerManager::UpdateMainControllerInternal(
   } else {
     LOG_MAINCONTROLLER_INFO("Set controller %" PRId64 " as main controller",
                             mMainController->Id());
-    mSource->SetPlaybackState(mMainController->PlaybackState());
-    mSource->SetMediaMetadata(mMainController->GetCurrentMediaMetadata());
-    mSource->SetSupportedMediaKeys(mMainController->GetSupportedMediaKeys());
-    ConnectMainControllerEvents();
+    if (mSource->Open()) {
+      mSource->SetPlaybackState(mMainController->PlaybackState());
+      mSource->SetMediaMetadata(mMainController->GetCurrentMediaMetadata());
+      mSource->SetSupportedMediaKeys(mMainController->GetSupportedMediaKeys());
+      ConnectMainControllerEvents();
+    } else {
+      LOG("Failed to open source for monitoring media keys");
+    }
   }
 
   if (StaticPrefs::media_mediacontrol_testingevents_enabled()) {

dom/media/mediacontrol/MediaControlService.h

@@ -67,12 +67,6 @@ class MediaControlService final : public nsIObserver {
   // key events and would show its metadata to virtual controller interface.
   MediaController* GetMainController() const;
 
-  // This event is used to generate a media event indicating media controller
-  // amount changed.
-  MediaEventSource<uint64_t>& MediaControllerAmountChangedEvent() {
-    return mMediaControllerAmountChangedEvent;
-  }
-
   /**
    * These following functions are used for testing only. We use them to
    * generate fake media control key events, get the media metadata and playback