CameraMicTrayItemView follows notifications instead of capture state

VmCameraMicManager displays camera and mic notifications based on VMs'
media capture state. And CameraMicTrayItemView is meant to follow
VmCameraMicManager and displays camera and mic indicators on the
systray.

CL 2576232 changes the notifications behavior such that when a VM is
accessing both camera and mic, only a camera icon notification is
displayed. In this case, we don't want CameraMicTrayItemView to display
the mic indicator unless a mic icon notification is also displayed.

To fix this issue, this CL lets CameraMicTrayItemView follows the
notification state (via VmCameraMicManager::IsNotificationActive())
instead of the device state (via VmCameraMicManager::IsDeviceActive()).

bug: b/167491603
Change-Id: I514f7be204d74cc43ac67d12bc2ce7215bf50cb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2581180
Commit-Queue: Jason Lin <lxj@google.com>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835526}

ash/media/media_controller_impl.cc

@@ -92,10 +92,10 @@ void MediaControllerImpl::NotifyCaptureState(
     observer.OnMediaCaptureChanged(capture_states);
 }
 
-void MediaControllerImpl::NotifyVmCaptureState(
-    MediaCaptureState capture_state) {
+void MediaControllerImpl::NotifyVmMediaNotificationState(bool camera,
+                                                         bool mic) {
   for (auto& observer : observers_)
-    observer.OnVmMediaCaptureChanged(capture_state);
+    observer.OnVmMediaNotificationChanged(camera, mic);
 }
 
 void MediaControllerImpl::HandleMediaPlayPause() {

ash/media/media_controller_impl.h

@@ -26,8 +26,8 @@ class MediaCaptureObserver {
   // Called when media capture state has changed.
   virtual void OnMediaCaptureChanged(
       const base::flat_map<AccountId, MediaCaptureState>& capture_states) {}
-  // Called when a VM's media capture state has changed.
-  virtual void OnVmMediaCaptureChanged(MediaCaptureState state) {}
+  // Called when a VM's media notifications have changed.
+  virtual void OnVmMediaNotificationChanged(bool camera, bool mic) {}
 
  protected:
   virtual ~MediaCaptureObserver() {}
@@ -57,7 +57,7 @@ class ASH_EXPORT MediaControllerImpl
   void SetForceMediaClientKeyHandling(bool enabled) override;
   void NotifyCaptureState(const base::flat_map<AccountId, MediaCaptureState>&
                               capture_states) override;
-  void NotifyVmCaptureState(MediaCaptureState capture_state) override;
+  void NotifyVmMediaNotificationState(bool camera, bool mic) override;
 
   // If media session accelerators are enabled then these methods will use the
   // media session service to control playback. Otherwise it will forward to

ash/public/cpp/media_controller.h

@@ -41,13 +41,9 @@ class ASH_PUBLIC_EXPORT MediaController {
   // MediaCaptureState representing every user's state.
   virtual void NotifyCaptureState(
       const base::flat_map<AccountId, MediaCaptureState>& capture_states) = 0;
-  // Called when a VM's media capture state changes. There is no `AccountId` in
+  // Called when a VM's media notifications change. There is no `AccountId` in
   // the argument because only the primary account/profile can launch a VM.
-  //
-  // TODO(b/167491603): We should consider merging this with
-  // `NotifyCaptureState()` if the browser also uses the same systray capturing
-  // indicators as VMs'.
-  virtual void NotifyVmCaptureState(MediaCaptureState capture_state) = 0;
+  virtual void NotifyVmMediaNotificationState(bool camera, bool mic) = 0;
 
  protected:
   MediaController();

ash/system/unified/camera_mic_tray_item_view.cc

@@ -58,16 +58,14 @@ CameraMicTrayItemView::~CameraMicTrayItemView() {
   shell->session_controller()->RemoveObserver(this);
 }
 
-void CameraMicTrayItemView::OnVmMediaCaptureChanged(
-    MediaCaptureState capture_state) {
+void CameraMicTrayItemView::OnVmMediaNotificationChanged(bool camera,
+                                                         bool mic) {
   switch (type_) {
     case Type::kCamera:
-      active_ = (capture_state == MediaCaptureState::kVideo ||
-                 capture_state == MediaCaptureState::kAudioVideo);
+      active_ = camera;
       break;
     case Type::kMic:
-      active_ = (capture_state == MediaCaptureState::kAudio ||
-                 capture_state == MediaCaptureState::kAudioVideo);
+      active_ = mic;
       break;
   }
   Update();

ash/system/unified/camera_mic_tray_item_view.h

@@ -43,7 +43,7 @@ class ASH_EXPORT CameraMicTrayItemView : public TrayItemView,
   void HandleLocaleChange() override;
 
   // MediaCaptureObserver:
-  void OnVmMediaCaptureChanged(MediaCaptureState capture_state) override;
+  void OnVmMediaNotificationChanged(bool camera, bool mic) override;
 
  private:
   void Update();

ash/system/unified/camera_mic_tray_item_view_unittest.cc

@@ -17,25 +17,6 @@ namespace ash {
 
 namespace {
 using Type = CameraMicTrayItemView::Type;
-
-MediaCaptureState GetRelevantCaptureState(Type type) {
-  switch (type) {
-    case Type::kCamera:
-      return MediaCaptureState::kVideo;
-    case Type::kMic:
-      return MediaCaptureState::kAudio;
-  }
-}
-
-MediaCaptureState GetIrrelevantCaptureState(Type type) {
-  switch (type) {
-    case Type::kCamera:
-      return MediaCaptureState::kAudio;
-    case Type::kMic:
-      return MediaCaptureState::kVideo;
-  }
-}
-
 }  // namespace
 
 class CameraMicTrayItemViewTest : public AshTestBase,
@@ -65,31 +46,30 @@ class CameraMicTrayItemViewTest : public AshTestBase,
   std::unique_ptr<CameraMicTrayItemView> camera_mic_tray_item_view_;
 };
 
-TEST_P(CameraMicTrayItemViewTest, OnVmMediaCaptureChanged) {
+TEST_P(CameraMicTrayItemViewTest, OnVmMediaNotificationChanged) {
   Type type = GetParam();
-  MediaCaptureState relevant = GetRelevantCaptureState(type);
-  MediaCaptureState irrelevant = GetIrrelevantCaptureState(type);
-
   EXPECT_FALSE(camera_mic_tray_item_view_->GetVisible());
 
-  camera_mic_tray_item_view_->OnVmMediaCaptureChanged(relevant);
-  EXPECT_TRUE(camera_mic_tray_item_view_->GetVisible());
+  camera_mic_tray_item_view_->OnVmMediaNotificationChanged(/*camera=*/true,
+                                                           /*mic=*/false);
+  EXPECT_EQ(camera_mic_tray_item_view_->GetVisible(), type == Type::kCamera);
 
-  camera_mic_tray_item_view_->OnVmMediaCaptureChanged(irrelevant);
-  EXPECT_FALSE(camera_mic_tray_item_view_->GetVisible());
+  camera_mic_tray_item_view_->OnVmMediaNotificationChanged(/*camera=*/false,
+                                                           /*mic=*/true);
+  EXPECT_EQ(camera_mic_tray_item_view_->GetVisible(), type == Type::kMic);
 
-  camera_mic_tray_item_view_->OnVmMediaCaptureChanged(
-      static_cast<MediaCaptureState>(static_cast<int>(relevant) |
-                                     static_cast<int>(irrelevant)));
+  camera_mic_tray_item_view_->OnVmMediaNotificationChanged(/*camera=*/true,
+                                                           /*mic=*/true);
   EXPECT_TRUE(camera_mic_tray_item_view_->GetVisible());
 
-  camera_mic_tray_item_view_->OnVmMediaCaptureChanged(MediaCaptureState::kNone);
+  camera_mic_tray_item_view_->OnVmMediaNotificationChanged(/*camera=*/false,
+                                                           /*mic=*/false);
   EXPECT_FALSE(camera_mic_tray_item_view_->GetVisible());
 }
 
 TEST_P(CameraMicTrayItemViewTest, HideForNonPrimaryUser) {
-  camera_mic_tray_item_view_->OnVmMediaCaptureChanged(
-      GetRelevantCaptureState(GetParam()));
+  camera_mic_tray_item_view_->OnVmMediaNotificationChanged(/*camera=*/true,
+                                                           /*mic=*/true);
   EXPECT_TRUE(camera_mic_tray_item_view_->GetVisible());
 
   SimulateUserLogin("user2@test.com");

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager.cc

@@ -111,7 +111,7 @@ void VmCameraMicManager::SetActive(VmType vm, DeviceType device, bool active) {
   }
 }
 
-bool VmCameraMicManager::GetDeviceActive(DeviceType device) const {
+bool VmCameraMicManager::IsDeviceActive(DeviceType device) const {
   for (const auto& vm_notification : notification_map_) {
     const NotificationType& notification_type = vm_notification.second;
     if (notification_type[static_cast<size_t>(device)]) {

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager.h

@@ -55,9 +55,7 @@ class VmCameraMicManager : public KeyedService {
   void RemoveObserver(Observer* observer);
 
   // Return true if any of the VMs is using the device.
-  //
-  // TODO(b/167491603): Rename to `IsDeviceActive()` for consistency.
-  bool GetDeviceActive(DeviceType device) const;
+  bool IsDeviceActive(DeviceType device) const;
 
   // When a VM is using both camera and mic, we only show a single "camera and
   // mic" notification, which is considered a camera notification but not a mic
@@ -65,9 +63,6 @@ class VmCameraMicManager : public KeyedService {
   // "camera and mic" notifications are shown, this function returns true for
   // `kCamera` but false for `kMic`. If a "mic only" notification is shown, this
   // function returns true for `kMic`.
-  //
-  // TODO(b/167491603): We need to switch the indicator displaying logic to use
-  // this function instead.
   bool IsNotificationActive(DeviceType device) const;
 
  private:

chrome/browser/chromeos/camera_mic/vm_camera_mic_manager_unittest.cc

@@ -150,7 +150,7 @@ class VmCameraMicManagerTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(VmCameraMicManagerTest);
 };
 
-// Test `GetDeviceActive()` and `IsNotificationActive()`.
+// Test `IsDeviceActive()` and `IsNotificationActive()`.
 class VmCameraMicManagerIsActiveTest
     : public VmCameraMicManagerTest,
       public testing::WithParamInterface<IsActiveTestParam> {};
@@ -160,7 +160,7 @@ TEST_P(VmCameraMicManagerIsActiveTest, IsNotificationActive) {
 
   for (const auto& device_and_expectation : GetParam().device_expectations) {
     EXPECT_EQ(
-        vm_camera_mic_manager_->GetDeviceActive(device_and_expectation.first),
+        vm_camera_mic_manager_->IsDeviceActive(device_and_expectation.first),
         device_and_expectation.second);
   }
 

chrome/browser/ui/ash/media_client_impl.cc

@@ -259,11 +259,14 @@ void MediaClientImpl::OnVmCameraMicActiveChanged(
     chromeos::VmCameraMicManager* manager) {
   using DeviceType = chromeos::VmCameraMicManager::DeviceType;
   vm_media_capture_state_ = MediaCaptureState::kNone;
-  if (manager->GetDeviceActive(DeviceType::kCamera))
+  if (manager->IsDeviceActive(DeviceType::kCamera))
     vm_media_capture_state_ |= MediaCaptureState::kVideo;
-  if (manager->GetDeviceActive(DeviceType::kMic))
+  if (manager->IsDeviceActive(DeviceType::kMic))
     vm_media_capture_state_ |= MediaCaptureState::kAudio;
-  media_controller_->NotifyVmCaptureState(vm_media_capture_state_);
+
+  media_controller_->NotifyVmMediaNotificationState(
+      manager->IsNotificationActive(DeviceType::kCamera),
+      manager->IsNotificationActive(DeviceType::kMic));
 }
 
 void MediaClientImpl::EnableCustomMediaKeyHandler(

chrome/browser/ui/ash/media_client_impl_unittest.cc

@@ -30,7 +30,7 @@ class TestMediaController : public ash::MediaController {
       const base::flat_map<AccountId, ash::MediaCaptureState>& capture_states)
       override {}
 
-  void NotifyVmCaptureState(ash::MediaCaptureState capture_states) override {}
+  void NotifyVmMediaNotificationState(bool camera, bool mic) override {}
 
   bool force_media_client_key_handling() const {
     return force_media_client_key_handling_;