From 4d5aea65ac91ad83359d6ab03a6b6f0a6e0c6780 Mon Sep 17 00:00:00 2001 From: Andre Le Date: Thu, 16 Mar 2023 23:36:08 +0000 Subject: [PATCH] VC UI: Implement new id system for VC effects Camera and audio effects are currently using their own id system, which makes it hard to do metrics collection. This CL introduces a new id class used by both camera and audio effects. Also, when creating an effect, it is required to provide a new id to make sure that metrics are collected for that new effect. Bug: b:265197876 Change-Id: Id9d33315e65abe6cdfb3336c96425a9bdc2adb0d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4347952 Reviewed-by: Alex Newcomer Commit-Queue: Andre Le Cr-Commit-Position: refs/heads/main@{#1118429} --- ash/system/audio/audio_effects_controller.cc | 83 ++++++++++--------- ash/system/audio/audio_effects_controller.h | 18 ++-- .../audio_effects_controller_unittest.cc | 41 +++++---- .../camera/camera_effects_controller.cc | 77 +++++++++-------- ash/system/camera/camera_effects_controller.h | 6 +- .../camera_effects_controller_unittest.cc | 52 +++++------- .../bubble/bubble_view_unittest.cc | 22 +++-- .../effects/fake_video_conference_effects.cc | 31 +++---- .../effects/fake_video_conference_effects.h | 13 +-- .../video_conference_tray_effects_delegate.h | 6 +- ...o_conference_tray_effects_manager_types.cc | 10 +-- ...eo_conference_tray_effects_manager_types.h | 28 ++++--- 12 files changed, 195 insertions(+), 192 deletions(-) diff --git a/ash/system/audio/audio_effects_controller.cc b/ash/system/audio/audio_effects_controller.cc index f224d7faca33b..541cfb21f287e 100644 --- a/ash/system/audio/audio_effects_controller.cc +++ b/ash/system/audio/audio_effects_controller.cc @@ -33,53 +33,49 @@ AudioEffectsController::~AudioEffectsController() { } } -bool AudioEffectsController::IsEffectSupported( - AudioEffectId effect_id /*=AudioEffectId::kNone*/) { +bool AudioEffectsController::IsEffectSupported(VcEffectId effect_id) { switch (effect_id) { - case AudioEffectId::kNoiseCancellation: + case VcEffectId::kNoiseCancellation: return CrasAudioHandler::Get()->noise_cancellation_supported(); - case AudioEffectId::kLiveCaption: + case VcEffectId::kLiveCaption: return captions::IsLiveCaptionFeatureSupported(); - case AudioEffectId::kNone: - return IsEffectSupported(AudioEffectId::kNoiseCancellation) || - IsEffectSupported(AudioEffectId::kLiveCaption); + case VcEffectId::kBackgroundBlur: + case VcEffectId::kPortraitRelighting: + case VcEffectId::kTestEffect: + NOTREACHED(); + return false; } - - NOTREACHED(); - return false; } -absl::optional AudioEffectsController::GetEffectState(int effect_id) { +absl::optional AudioEffectsController::GetEffectState( + VcEffectId effect_id) { switch (effect_id) { - case AudioEffectId::kNoiseCancellation: { + case VcEffectId::kNoiseCancellation: return CrasAudioHandler::Get()->GetNoiseCancellationState() ? 1 : 0; - } - case AudioEffectId::kLiveCaption: { + case VcEffectId::kLiveCaption: return Shell::Get()->accessibility_controller()->live_caption().enabled() ? 1 : 0; - } - case AudioEffectId::kNone: + case VcEffectId::kBackgroundBlur: + case VcEffectId::kPortraitRelighting: + case VcEffectId::kTestEffect: + NOTREACHED(); return absl::nullopt; } - - NOTREACHED(); - return absl::nullopt; } void AudioEffectsController::OnEffectControlActivated( - absl::optional effect_id, + VcEffectId effect_id, absl::optional value) { - DCHECK(effect_id.has_value()); - switch (effect_id.value()) { - case AudioEffectId::kNoiseCancellation: { + switch (effect_id) { + case VcEffectId::kNoiseCancellation: { // Toggle noise cancellation. CrasAudioHandler* audio_handler = CrasAudioHandler::Get(); bool new_state = !audio_handler->GetNoiseCancellationState(); audio_handler->SetNoiseCancellationState(new_state); return; } - case AudioEffectId::kLiveCaption: { + case VcEffectId::kLiveCaption: { // Toggle live caption. AccessibilityControllerImpl* controller = Shell::Get()->accessibility_controller(); @@ -87,11 +83,12 @@ void AudioEffectsController::OnEffectControlActivated( !controller->live_caption().enabled()); return; } - case AudioEffectId::kNone: + case VcEffectId::kBackgroundBlur: + case VcEffectId::kPortraitRelighting: + case VcEffectId::kTestEffect: + NOTREACHED(); return; } - - NOTREACHED(); } void AudioEffectsController::OnActiveUserPrefServiceChanged( @@ -105,26 +102,32 @@ void AudioEffectsController::OnActiveUserPrefServiceChanged( return; } - if (IsEffectSupported(AudioEffectId::kNoiseCancellation)) { + const bool noise_cancellation_supported = + IsEffectSupported(VcEffectId::kNoiseCancellation); + const bool live_caption_supported = + IsEffectSupported(VcEffectId::kLiveCaption); + + if (noise_cancellation_supported) { AddNoiseCancellationEffect(); } - if (IsEffectSupported(AudioEffectId::kLiveCaption)) { + if (live_caption_supported) { AddLiveCaptionEffect(); } - if (IsEffectSupported()) { + if (noise_cancellation_supported || live_caption_supported) { effects_manager.RegisterDelegate(this); } } void AudioEffectsController::AddNoiseCancellationEffect() { std::unique_ptr effect = std::make_unique( - VcEffectType::kToggle, + /*type=*/VcEffectType::kToggle, + /*get_state_callback=*/ base::BindRepeating(&AudioEffectsController::GetEffectState, base::Unretained(this), - static_cast(AudioEffectId::kNoiseCancellation))); - effect->set_id(static_cast(AudioEffectId::kNoiseCancellation)); + VcEffectId::kNoiseCancellation), + /*effect_id=*/VcEffectId::kNoiseCancellation); effect->AddState(std::make_unique( /*icon=*/&kVideoConferenceNoiseCancellationOnIcon, /*label_text=*/ @@ -135,8 +138,7 @@ void AudioEffectsController::AddNoiseCancellationEffect() { /*button_callback=*/ base::BindRepeating(&AudioEffectsController::OnEffectControlActivated, weak_factory_.GetWeakPtr(), - /*effect_id=*/ - static_cast(AudioEffectId::kNoiseCancellation), + /*effect_id=*/VcEffectId::kNoiseCancellation, /*value=*/0))); effect->set_dependency_flags(VcHostedEffect::ResourceDependency::kMicrophone); AddEffect(std::move(effect)); @@ -144,11 +146,11 @@ void AudioEffectsController::AddNoiseCancellationEffect() { void AudioEffectsController::AddLiveCaptionEffect() { std::unique_ptr effect = std::make_unique( - VcEffectType::kToggle, + /*type=*/VcEffectType::kToggle, + /*get_state_callback=*/ base::BindRepeating(&AudioEffectsController::GetEffectState, - base::Unretained(this), - static_cast(AudioEffectId::kLiveCaption))); - effect->set_id(static_cast(AudioEffectId::kLiveCaption)); + base::Unretained(this), VcEffectId::kLiveCaption), + /*effect_id=*/VcEffectId::kLiveCaption); effect->AddState(std::make_unique( /*icon=*/&kVideoConferenceLiveCaptionOnIcon, /*label_text=*/ @@ -158,8 +160,7 @@ void AudioEffectsController::AddLiveCaptionEffect() { /*button_callback=*/ base::BindRepeating(&AudioEffectsController::OnEffectControlActivated, weak_factory_.GetWeakPtr(), - /*effect_id=*/ - static_cast(AudioEffectId::kLiveCaption), + /*effect_id=*/VcEffectId::kLiveCaption, /*value=*/0))); AddEffect(std::move(effect)); } diff --git a/ash/system/audio/audio_effects_controller.h b/ash/system/audio/audio_effects_controller.h index ab9259de7b57c..ea1db92beffa6 100644 --- a/ash/system/audio/audio_effects_controller.h +++ b/ash/system/audio/audio_effects_controller.h @@ -15,15 +15,11 @@ namespace ash { +enum class VcEffectId; + class ASH_EXPORT AudioEffectsController : public VcEffectsDelegate, public SessionObserver { public: - enum AudioEffectId { - kNone = 0, - kNoiseCancellation = 1, - kLiveCaption = 2, - }; - AudioEffectsController(); AudioEffectsController(const AudioEffectsController&) = delete; @@ -31,14 +27,12 @@ class ASH_EXPORT AudioEffectsController : public VcEffectsDelegate, ~AudioEffectsController() override; - // Returns whether `effect_id` is supported. If passed an `effect_id` of - // `AudioEffectId::kNone`, the function returns whether *any* effects are - // supported. - bool IsEffectSupported(AudioEffectId effect_id = AudioEffectId::kNone); + // Returns whether `effect_id` is supported. + bool IsEffectSupported(VcEffectId effect_id); // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override; - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override; + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override; // SessionObserver: diff --git a/ash/system/audio/audio_effects_controller_unittest.cc b/ash/system/audio/audio_effects_controller_unittest.cc index d895c75e7c41e..e28ae3b5e0a0a 100644 --- a/ash/system/audio/audio_effects_controller_unittest.cc +++ b/ash/system/audio/audio_effects_controller_unittest.cc @@ -89,7 +89,7 @@ TEST_F(AudioEffectsControllerTest, NoiseCancellationNotSupported) { // `AudioEffectsController` reports noise that cancellation is not-supported. EXPECT_FALSE(audio_effects_controller()->IsEffectSupported( - AudioEffectsController::AudioEffectId::kNoiseCancellation)); + VcEffectId::kNoiseCancellation)); } TEST_F(AudioEffectsControllerTest, NoiseCancellationSupported) { @@ -101,12 +101,11 @@ TEST_F(AudioEffectsControllerTest, NoiseCancellationSupported) { // `AudioEffectsController` reports that noise cancellation is supported. EXPECT_TRUE(audio_effects_controller()->IsEffectSupported( - AudioEffectsController::AudioEffectId::kNoiseCancellation)); + VcEffectId::kNoiseCancellation)); // Makes sure the dependency flag is set when the effect is supported. auto* effect = audio_effects_controller()->GetEffect(0); - ASSERT_EQ(AudioEffectsController::AudioEffectId::kNoiseCancellation, - effect->id()); + ASSERT_EQ(VcEffectId::kNoiseCancellation, effect->id()); EXPECT_EQ(VcHostedEffect::ResourceDependency::kMicrophone, effect->dependency_flags()); } @@ -123,13 +122,13 @@ TEST_F(AudioEffectsControllerTest, NoiseCancellationNotEnabled) { // Noise cancellation effect state is disabled. absl::optional effect_state = audio_effects_controller()->GetEffectState( - AudioEffectsController::AudioEffectId::kNoiseCancellation); + VcEffectId::kNoiseCancellation); EXPECT_TRUE(effect_state.has_value()); EXPECT_EQ(effect_state, 0); cras_audio_handler()->SetNoiseCancellationState(true); effect_state = audio_effects_controller()->GetEffectState( - AudioEffectsController::AudioEffectId::kNoiseCancellation); + VcEffectId::kNoiseCancellation); EXPECT_TRUE(effect_state.has_value()); EXPECT_EQ(effect_state, 1); } @@ -146,7 +145,7 @@ TEST_F(AudioEffectsControllerTest, NoiseCancellationEnabled) { // Noise cancellation effect state is disabled. absl::optional effect_state = audio_effects_controller()->GetEffectState( - AudioEffectsController::AudioEffectId::kNoiseCancellation); + VcEffectId::kNoiseCancellation); EXPECT_TRUE(effect_state.has_value()); EXPECT_EQ(effect_state, 1); } @@ -166,7 +165,7 @@ TEST_F(AudioEffectsControllerTest, NoiseCancellationSetNotEnabled) { // User pressed the noise cancellation toggle. audio_effects_controller()->OnEffectControlActivated( - AudioEffectsController::AudioEffectId::kNoiseCancellation, absl::nullopt); + VcEffectId::kNoiseCancellation, absl::nullopt); // State should now be disabled. EXPECT_FALSE(cras_audio_handler()->GetNoiseCancellationState()); @@ -187,7 +186,7 @@ TEST_F(AudioEffectsControllerTest, NoiseCancellationSetEnabled) { // User pressed the noise cancellation toggle. audio_effects_controller()->OnEffectControlActivated( - AudioEffectsController::AudioEffectId::kNoiseCancellation, absl::nullopt); + VcEffectId::kNoiseCancellation, absl::nullopt); // State should now be enabled. EXPECT_TRUE(cras_audio_handler()->GetNoiseCancellationState()); @@ -198,8 +197,8 @@ TEST_F(AudioEffectsControllerTest, LiveCaptionNotSupported) { // No live caption feature flags enabled, so `AudioEffectsController` reports // that live caption is not supported. - EXPECT_FALSE(audio_effects_controller()->IsEffectSupported( - AudioEffectsController::AudioEffectId::kLiveCaption)); + EXPECT_FALSE( + audio_effects_controller()->IsEffectSupported(VcEffectId::kLiveCaption)); } TEST_F(AudioEffectsControllerTest, LiveCaptionSupported) { @@ -213,8 +212,8 @@ TEST_F(AudioEffectsControllerTest, LiveCaptionSupported) { // Live caption feature flags are enabled, so `AudioEffectsController` reports // that live caption is supported. - EXPECT_TRUE(audio_effects_controller()->IsEffectSupported( - AudioEffectsController::AudioEffectId::kLiveCaption)); + EXPECT_TRUE( + audio_effects_controller()->IsEffectSupported(VcEffectId::kLiveCaption)); } TEST_F(AudioEffectsControllerTest, LiveCaptionNotEnabled) { @@ -234,8 +233,8 @@ TEST_F(AudioEffectsControllerTest, LiveCaptionNotEnabled) { EXPECT_FALSE(controller->live_caption().enabled()); // Live caption effect state is disabled. - absl::optional state = audio_effects_controller()->GetEffectState( - AudioEffectsController::AudioEffectId::kLiveCaption); + absl::optional state = + audio_effects_controller()->GetEffectState(VcEffectId::kLiveCaption); EXPECT_TRUE(state.has_value()); EXPECT_FALSE(state.value()); } @@ -257,8 +256,8 @@ TEST_F(AudioEffectsControllerTest, LiveCaptionEnabled) { EXPECT_TRUE(controller->live_caption().enabled()); // Live caption effect state is enabled. - absl::optional state = audio_effects_controller()->GetEffectState( - AudioEffectsController::AudioEffectId::kLiveCaption); + absl::optional state = + audio_effects_controller()->GetEffectState(VcEffectId::kLiveCaption); EXPECT_TRUE(state.has_value()); EXPECT_TRUE(state.value()); } @@ -280,8 +279,8 @@ TEST_F(AudioEffectsControllerTest, LiveCaptionSetNotEnabled) { EXPECT_TRUE(controller->live_caption().enabled()); // User pressed the live caption toggle. - audio_effects_controller()->OnEffectControlActivated( - AudioEffectsController::AudioEffectId::kLiveCaption, absl::nullopt); + audio_effects_controller()->OnEffectControlActivated(VcEffectId::kLiveCaption, + absl::nullopt); // Live caption is now disabled. EXPECT_FALSE(controller->live_caption().enabled()); @@ -304,8 +303,8 @@ TEST_F(AudioEffectsControllerTest, LiveCaptionSetEnabled) { EXPECT_FALSE(controller->live_caption().enabled()); // User pressed the live caption toggle. - audio_effects_controller()->OnEffectControlActivated( - AudioEffectsController::AudioEffectId::kLiveCaption, absl::nullopt); + audio_effects_controller()->OnEffectControlActivated(VcEffectId::kLiveCaption, + absl::nullopt); // Live caption is now enabled. EXPECT_TRUE(controller->live_caption().enabled()); diff --git a/ash/system/camera/camera_effects_controller.cc b/ash/system/camera/camera_effects_controller.cc index 6be2ba0f1c4c0..1aae47ccf9519 100644 --- a/ash/system/camera/camera_effects_controller.cc +++ b/ash/system/camera/camera_effects_controller.cc @@ -177,31 +177,29 @@ void CameraEffectsController::OnActiveUserPrefServiceChanged( InitializeEffectControls(); } -absl::optional CameraEffectsController::GetEffectState(int effect_id) { - switch (static_cast(effect_id)) { - case cros::mojom::CameraEffect::kBackgroundBlur: +absl::optional CameraEffectsController::GetEffectState( + VcEffectId effect_id) { + switch (effect_id) { + case VcEffectId::kBackgroundBlur: return MapBackgroundBlurCameraHalStateToEffectState( current_effects_->blur_level, current_effects_->blur_enabled); - case cros::mojom::CameraEffect::kPortraitRelight: + case VcEffectId::kPortraitRelighting: return current_effects_->relight_enabled; - case cros::mojom::CameraEffect::kBackgroundReplace: - case cros::mojom::CameraEffect::kNone: + case VcEffectId::kNoiseCancellation: + case VcEffectId::kLiveCaption: + case VcEffectId::kTestEffect: + NOTREACHED(); return absl::nullopt; } - - NOTREACHED(); - return absl::nullopt; } void CameraEffectsController::OnEffectControlActivated( - absl::optional effect_id, + VcEffectId effect_id, absl::optional state) { - DCHECK(effect_id.has_value()); - cros::mojom::EffectsConfigPtr new_effects = current_effects_.Clone(); - switch (effect_id.value()) { - case static_cast(cros::mojom::CameraEffect::kBackgroundBlur): { + switch (effect_id) { + case VcEffectId::kBackgroundBlur: { // UI should not pass in any invalid state. if (!state.has_value() || !IsValidBackgroundBlurState(state.value())) { state = static_cast( @@ -219,11 +217,16 @@ void CameraEffectsController::OnEffectControlActivated( } break; } - case static_cast(cros::mojom::CameraEffect::kPortraitRelight): { + case VcEffectId::kPortraitRelighting: { new_effects->relight_enabled = state.value_or(!new_effects->relight_enabled); break; } + case VcEffectId::kNoiseCancellation: + case VcEffectId::kLiveCaption: + case VcEffectId::kTestEffect: + NOTREACHED(); + return; } SetCameraEffects(std::move(new_effects)); @@ -363,14 +366,14 @@ void CameraEffectsController::InitializeEffectControls() { // states. if (IsEffectControlAvailable(cros::mojom::CameraEffect::kBackgroundBlur)) { auto effect = std::make_unique( - VcEffectType::kSetValue, - base::BindRepeating( - &CameraEffectsController::GetEffectState, base::Unretained(this), - static_cast(cros::mojom::CameraEffect::kBackgroundBlur))); + /*type=*/VcEffectType::kSetValue, + /*get_state_callback=*/ + base::BindRepeating(&CameraEffectsController::GetEffectState, + base::Unretained(this), + VcEffectId::kBackgroundBlur), + /*effect_id=*/VcEffectId::kBackgroundBlur); effect->set_label_text(l10n_util::GetStringUTF16( IDS_ASH_VIDEO_CONFERENCE_BUBBLE_BACKGROUND_BLUR_NAME)); - effect->set_id( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur)); AddBackgroundBlurStateToEffect( effect.get(), kVideoConferenceBackgroundBlurOffIcon, /*state_value=*/BackgroundBlurEffectState::kOff, @@ -392,12 +395,12 @@ void CameraEffectsController::InitializeEffectControls() { // and its state. if (IsEffectControlAvailable(cros::mojom::CameraEffect::kPortraitRelight)) { std::unique_ptr effect = std::make_unique( - VcEffectType::kToggle, - base::BindRepeating( - &CameraEffectsController::GetEffectState, base::Unretained(this), - static_cast(cros::mojom::CameraEffect::kPortraitRelight))); - effect->set_id( - static_cast(cros::mojom::CameraEffect::kPortraitRelight)); + /*type=*/VcEffectType::kToggle, + /*get_state_callback=*/ + base::BindRepeating(&CameraEffectsController::GetEffectState, + base::Unretained(this), + VcEffectId::kPortraitRelighting), + /*effect_id=*/VcEffectId::kPortraitRelighting); effect->AddState(std::make_unique( /*icon=*/&kVideoConferencePortraitRelightOnIcon, /*label_text=*/ @@ -406,12 +409,10 @@ void CameraEffectsController::InitializeEffectControls() { /*accessible_name_id=*/ IDS_ASH_VIDEO_CONFERENCE_BUBBLE_PORTRAIT_RELIGHT_NAME, /*button_callback=*/ - base::BindRepeating( - &CameraEffectsController::OnEffectControlActivated, - base::Unretained(this), - /*effect_id=*/ - static_cast(cros::mojom::CameraEffect::kPortraitRelight), - /*value=*/absl::nullopt))); + base::BindRepeating(&CameraEffectsController::OnEffectControlActivated, + base::Unretained(this), + /*effect_id=*/VcEffectId::kPortraitRelighting, + /*value=*/absl::nullopt))); effect->set_dependency_flags(VcHostedEffect::ResourceDependency::kCamera); AddEffect(std::move(effect)); } @@ -435,12 +436,10 @@ void CameraEffectsController::AddBackgroundBlurStateToEffect( /*label_text=*/l10n_util::GetStringUTF16(string_id), /*accessible_name_id=*/string_id, /*button_callback=*/ - base::BindRepeating( - &CameraEffectsController::OnEffectControlActivated, - weak_factory_.GetWeakPtr(), - /*effect_id=*/ - static_cast(cros::mojom::CameraEffect::kBackgroundBlur), - /*value=*/state_value), + base::BindRepeating(&CameraEffectsController::OnEffectControlActivated, + weak_factory_.GetWeakPtr(), + /*effect_id=*/VcEffectId::kBackgroundBlur, + /*value=*/state_value), /*state=*/state_value)); } diff --git a/ash/system/camera/camera_effects_controller.h b/ash/system/camera/camera_effects_controller.h index d4443862f050c..6b482b8ef1c62 100644 --- a/ash/system/camera/camera_effects_controller.h +++ b/ash/system/camera/camera_effects_controller.h @@ -26,6 +26,8 @@ struct VectorIcon; namespace ash { +enum class VcEffectId; + // CameraEffectsController is the interface for any object in ash to // enable/change camera effects. class ASH_EXPORT CameraEffectsController : public media::CameraEffectObserver, @@ -66,8 +68,8 @@ class ASH_EXPORT CameraEffectsController : public media::CameraEffectObserver, void OnActiveUserPrefServiceChanged(PrefService* pref_service) override; // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override; - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override; + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override; // media::CameraEffectObserver: diff --git a/ash/system/camera/camera_effects_controller_unittest.cc b/ash/system/camera/camera_effects_controller_unittest.cc index 031d1bc477017..da36a6e5afb5f 100644 --- a/ash/system/camera/camera_effects_controller_unittest.cc +++ b/ash/system/camera/camera_effects_controller_unittest.cc @@ -47,17 +47,16 @@ class CameraEffectsControllerTest : public NoSessionAshTestBase { } // Sets background blur state. - void SetBackgroundBlurEffectState(int state) { + void SetBackgroundBlurEffectState(absl::optional state) { camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur), state); + VcEffectId::kBackgroundBlur, state); } // Gets the state of the background blur effect from the effect's host, // `camera_effects_controller_`. int GetBackgroundBlurEffectState() { absl::optional effect_state = - camera_effects_controller_->GetEffectState( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur)); + camera_effects_controller_->GetEffectState(VcEffectId::kBackgroundBlur); DCHECK(effect_state.has_value()); return effect_state.value(); } @@ -70,12 +69,19 @@ class CameraEffectsControllerTest : public NoSessionAshTestBase { ->GetInteger(prefs::kBackgroundBlur); } + // Toggles portrait relighting state. + void TogglePortraitRelightingEffectState() { + // The `state` argument doesn't matter for toggle effects. + camera_effects_controller_->OnEffectControlActivated( + VcEffectId::kPortraitRelighting, /*state=*/absl::nullopt); + } + // Gets the state of the portrait relighting effect from the effect's host, // `camera_effects_controller_`. bool GetPortraitRelightingEffectState() { absl::optional effect_state = camera_effects_controller_->GetEffectState( - static_cast(cros::mojom::CameraEffect::kPortraitRelight)); + VcEffectId::kPortraitRelighting); DCHECK(effect_state.has_value()); return static_cast(effect_state.value()); } @@ -135,32 +141,27 @@ TEST_F(CameraEffectsControllerTest, BackgroundBlurOnEffectControlActivated) { CameraEffectsController::BackgroundBlurEffectState::kMedium, CameraEffectsController::BackgroundBlurEffectState::kHeavy, CameraEffectsController::BackgroundBlurEffectState::kMaximum}) { - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur), state); + SetBackgroundBlurEffectState(state); EXPECT_EQ(GetBackgroundBlurPref(), state); EXPECT_EQ(GetBackgroundBlurEffectState(), state); } // Invalid background blur effect state should set the state to kOff. - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur), + SetBackgroundBlurEffectState( static_cast( CameraEffectsController::BackgroundBlurEffectState::kMaximum) + - 1); + 1); EXPECT_EQ(GetBackgroundBlurPref(), CameraEffectsController::BackgroundBlurEffectState::kOff); EXPECT_EQ(GetBackgroundBlurEffectState(), CameraEffectsController::BackgroundBlurEffectState::kOff); // Set the background blur state to be kMaximum. - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur), + SetBackgroundBlurEffectState( CameraEffectsController::BackgroundBlurEffectState::kMaximum); // Setting the background blur state to null will reset the effects as // kOff. - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kBackgroundBlur), - absl::nullopt); + SetBackgroundBlurEffectState(absl::nullopt); EXPECT_EQ(GetBackgroundBlurPref(), CameraEffectsController::BackgroundBlurEffectState::kOff); EXPECT_EQ(GetBackgroundBlurEffectState(), @@ -175,25 +176,18 @@ TEST_F(CameraEffectsControllerTest, EXPECT_FALSE(GetPortraitRelightingEffectState()); EXPECT_FALSE(GetPortraitRelightingPref()); - // Activating the effect should toggle it to "true." The `value` argument - // doesn't matter for toggle effects. - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kPortraitRelight), - absl::nullopt); + // Activating the effect should toggle it to "true." + TogglePortraitRelightingEffectState(); EXPECT_TRUE(GetPortraitRelightingEffectState()); EXPECT_TRUE(GetPortraitRelightingPref()); // Another toggle should set it to "false." - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kPortraitRelight), - absl::nullopt); + TogglePortraitRelightingEffectState(); EXPECT_FALSE(GetPortraitRelightingEffectState()); EXPECT_FALSE(GetPortraitRelightingPref()); // And one more toggle should set it back to "true." - camera_effects_controller_->OnEffectControlActivated( - static_cast(cros::mojom::CameraEffect::kPortraitRelight), - absl::nullopt); + TogglePortraitRelightingEffectState(); EXPECT_TRUE(GetPortraitRelightingEffectState()); EXPECT_TRUE(GetPortraitRelightingPref()); } @@ -256,14 +250,12 @@ TEST_F(CameraEffectsControllerTest, ResourceDependencyFlags) { // Makes sure that all registered effects have the correct dependency flag. auto* background_blur = camera_effects_controller()->GetEffect(0); - ASSERT_EQ(static_cast(cros::mojom::CameraEffect::kBackgroundBlur), - background_blur->id()); + ASSERT_EQ(VcEffectId::kBackgroundBlur, background_blur->id()); EXPECT_EQ(VcHostedEffect::ResourceDependency::kCamera, background_blur->dependency_flags()); auto* portrait_relight = camera_effects_controller()->GetEffect(1); - ASSERT_EQ(static_cast(cros::mojom::CameraEffect::kPortraitRelight), - portrait_relight->id()); + ASSERT_EQ(VcEffectId::kPortraitRelighting, portrait_relight->id()); EXPECT_EQ(VcHostedEffect::ResourceDependency::kCamera, portrait_relight->dependency_flags()); } diff --git a/ash/system/video_conference/bubble/bubble_view_unittest.cc b/ash/system/video_conference/bubble/bubble_view_unittest.cc index ae2881a3206e5..1a946d4e4d8f1 100644 --- a/ash/system/video_conference/bubble/bubble_view_unittest.cc +++ b/ash/system/video_conference/bubble/bubble_view_unittest.cc @@ -42,13 +42,14 @@ class SquareCinnamonCereal : public VcEffectsDelegate { VcEffectType::kToggle, base::BindRepeating(&SquareCinnamonCereal::GetEffectState, base::Unretained(this), - /*effect_id=*/VcEffectState::kUnusedId)); + /*effect_id=*/VcEffectId::kTestEffect), + VcEffectId::kTestEffect); auto state = std::make_unique( &ash::kPrivacyIndicatorsCameraIcon, u"Square Cinnamon Cereal", IDS_PRIVACY_NOTIFICATION_TITLE_CAMERA, base::BindRepeating(&SquareCinnamonCereal::OnEffectControlActivated, base::Unretained(this), - /*effect_id=*/VcEffectState::kUnusedId, + /*effect_id=*/VcEffectId::kTestEffect, /*value=*/absl::nullopt)); effect->AddState(std::move(state)); effect->set_container_id(kSquareCinnamonCerealViewId); @@ -60,8 +61,10 @@ class SquareCinnamonCereal : public VcEffectsDelegate { ~SquareCinnamonCereal() override = default; // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override { return 0; } - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override { + return 0; + } + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override {} }; @@ -74,13 +77,14 @@ class SnackNationForever : public VcEffectsDelegate { VcEffectType::kSetValue, base::BindRepeating(&SnackNationForever::GetEffectState, base::Unretained(this), - /*effect_id=*/VcEffectState::kUnusedId)); + /*effect_id=*/VcEffectId::kTestEffect), + VcEffectId::kTestEffect); auto state = std::make_unique( &ash::kPrivacyIndicatorsCameraIcon, u"Snack Nation", IDS_PRIVACY_NOTIFICATION_TITLE_CAMERA, base::BindRepeating(&SnackNationForever::OnEffectControlActivated, base::Unretained(this), - /*effect_id=*/VcEffectState::kUnusedId, + /*effect_id=*/VcEffectId::kTestEffect, /*value=*/0), /*state=*/0); effect->AddState(std::move(state)); @@ -93,8 +97,10 @@ class SnackNationForever : public VcEffectsDelegate { ~SnackNationForever() override = default; // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override { return 0; } - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override { + return 0; + } + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override {} }; diff --git a/ash/system/video_conference/effects/fake_video_conference_effects.cc b/ash/system/video_conference/effects/fake_video_conference_effects.cc index 2f5b68ba82f6d..c534a29c3d9c2 100644 --- a/ash/system/video_conference/effects/fake_video_conference_effects.cc +++ b/ash/system/video_conference/effects/fake_video_conference_effects.cc @@ -31,7 +31,8 @@ SimpleToggleEffect::SimpleToggleEffect( VcEffectType::kToggle, base::BindRepeating(&SimpleToggleEffect::GetEffectState, base::Unretained(this), - /*effect_id=*/VcEffectState::kUnusedId)); + /*effect_id=*/VcEffectId::kTestEffect), + VcEffectId::kTestEffect); // Use default `icon` and/or `accessible_name_id` if none was passed in. std::unique_ptr state = std::make_unique( @@ -39,20 +40,20 @@ SimpleToggleEffect::SimpleToggleEffect( accessible_name_id.value_or(IDS_PRIVACY_NOTIFICATION_TITLE_CAMERA), base::BindRepeating(&SimpleToggleEffect::OnEffectControlActivated, weak_factory_.GetWeakPtr(), - /*effect_id=*/VcEffectState::kUnusedId, + /*effect_id=*/VcEffectId::kTestEffect, /*value=*/absl::nullopt)); effect->AddState(std::move(state)); AddEffect(std::move(effect)); } -absl::optional SimpleToggleEffect::GetEffectState(int effect_id) { +absl::optional SimpleToggleEffect::GetEffectState(VcEffectId effect_id) { // Subclass `SimpleToggleEffect` if a specific integer or enum value (other // than 0) needs to be returned. Returning `absl::nullopt` is taken as "no // value could be obtained" and treated as an error condition. return 0; } -void SimpleToggleEffect::OnEffectControlActivated(absl::optional effect_id, +void SimpleToggleEffect::OnEffectControlActivated(VcEffectId effect_id, absl::optional state) { ++num_activations_for_testing_; } @@ -92,9 +93,9 @@ ShaggyFurEffect::ShaggyFurEffect() { VcEffectType::kSetValue, base::BindRepeating(&ShaggyFurEffect::GetEffectState, base::Unretained(this), - /*effect_id=*/0)); + /*effect_id=*/VcEffectId::kTestEffect), + VcEffectId::kTestEffect); effect->set_label_text(u"Shaggy Fur"); - effect->set_id(100); AddStateToEffect(effect.get(), /*state_value=*/static_cast(FurShagginess::kBald), /*label_text=*/u"Bald"); @@ -114,11 +115,11 @@ ShaggyFurEffect::ShaggyFurEffect() { ShaggyFurEffect::~ShaggyFurEffect() = default; -absl::optional ShaggyFurEffect::GetEffectState(int effect_id) { +absl::optional ShaggyFurEffect::GetEffectState(VcEffectId effect_id) { return static_cast(FurShagginess::kBuzzcut); } -void ShaggyFurEffect::OnEffectControlActivated(absl::optional effect_id, +void ShaggyFurEffect::OnEffectControlActivated(VcEffectId effect_id, absl::optional state) { DCHECK(state.has_value()); DCHECK(state.value() >= 0 && @@ -141,7 +142,8 @@ void ShaggyFurEffect::AddStateToEffect(VcHostedEffect* effect, /*accessible_name_id=*/IDS_PRIVACY_NOTIFICATION_TITLE_CAMERA, /*button_callback=*/ base::BindRepeating(&ShaggyFurEffect::OnEffectControlActivated, - weak_factory_.GetWeakPtr(), /*effect_id=*/0, + weak_factory_.GetWeakPtr(), + /*effect_id=*/VcEffectId::kTestEffect, /*value=*/state_value), /*state=*/state_value)); } @@ -151,9 +153,9 @@ SuperCutnessEffect::SuperCutnessEffect() { VcEffectType::kSetValue, base::BindRepeating(&SuperCutnessEffect::GetEffectState, base::Unretained(this), - /*effect_id=*/0)); + /*effect_id=*/VcEffectId::kTestEffect), + VcEffectId::kTestEffect); effect->set_label_text(u"Super Cuteness"); - effect->set_id(200); AddStateToEffect(effect.get(), /*state_value=*/static_cast(HowCute::kUglyDog), /*label_text=*/u"Ugly Dog"); @@ -173,7 +175,7 @@ SuperCutnessEffect::SuperCutnessEffect() { SuperCutnessEffect::~SuperCutnessEffect() = default; -absl::optional SuperCutnessEffect::GetEffectState(int effect_id) { +absl::optional SuperCutnessEffect::GetEffectState(VcEffectId effect_id) { if (has_invalid_effect_state_for_testing_) { return absl::nullopt; } @@ -181,7 +183,7 @@ absl::optional SuperCutnessEffect::GetEffectState(int effect_id) { return static_cast(HowCute::kTeddyBear); } -void SuperCutnessEffect::OnEffectControlActivated(absl::optional effect_id, +void SuperCutnessEffect::OnEffectControlActivated(VcEffectId effect_id, absl::optional state) { DCHECK(state.has_value()); DCHECK(state.value() >= 0 && @@ -204,7 +206,8 @@ void SuperCutnessEffect::AddStateToEffect(VcHostedEffect* effect, /*accessible_name_id=*/IDS_PRIVACY_NOTIFICATION_TITLE_CAMERA, /*button_callback=*/ base::BindRepeating(&SuperCutnessEffect::OnEffectControlActivated, - weak_factory_.GetWeakPtr(), /*effect_id=*/0, + weak_factory_.GetWeakPtr(), + /*effect_id=*/VcEffectId::kTestEffect, /*value=*/state_value), /*state=*/state_value)); } diff --git a/ash/system/video_conference/effects/fake_video_conference_effects.h b/ash/system/video_conference/effects/fake_video_conference_effects.h index 3cce16860c9cd..7376e9348bce4 100644 --- a/ash/system/video_conference/effects/fake_video_conference_effects.h +++ b/ash/system/video_conference/effects/fake_video_conference_effects.h @@ -19,6 +19,7 @@ struct VectorIcon; namespace ash { class FakeVideoConferenceTrayController; +enum class VcEffectId; namespace fake_video_conference { @@ -40,8 +41,8 @@ class SimpleToggleEffect : public VcEffectsDelegate { ~SimpleToggleEffect() override; // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override; - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override; + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override; int num_activations_for_testing() { return num_activations_for_testing_; } @@ -145,8 +146,8 @@ class ASH_EXPORT ShaggyFurEffect : public VcEffectsDelegate { ~ShaggyFurEffect() override; // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override; - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override; + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override; // Returns the number of times the button for `state` has been activated. @@ -182,8 +183,8 @@ class ASH_EXPORT SuperCutnessEffect : public VcEffectsDelegate { ~SuperCutnessEffect() override; // VcEffectsDelegate: - absl::optional GetEffectState(int effect_id) override; - void OnEffectControlActivated(absl::optional effect_id, + absl::optional GetEffectState(VcEffectId effect_id) override; + void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) override; // Returns the number of times the button for `state` has been activated. diff --git a/ash/system/video_conference/effects/video_conference_tray_effects_delegate.h b/ash/system/video_conference/effects/video_conference_tray_effects_delegate.h index dfc1a81a72d1a..c7993c27ad8cc 100644 --- a/ash/system/video_conference/effects/video_conference_tray_effects_delegate.h +++ b/ash/system/video_conference/effects/video_conference_tray_effects_delegate.h @@ -12,6 +12,8 @@ namespace ash { +enum class VcEffectId; + // An interface for hosting video conference effects, that are adjustable by the // user via the video conference bubble. Subclasses must register with // `VideoConferenceTrayEffectsManager`. At bubble construction time, @@ -49,7 +51,7 @@ class ASH_EXPORT VcEffectsDelegate { // effect state. `effect_id` specifies the effect whose state is requested, // and can be ignored if only one effect is being hosted. If no state can be // determined for `effect_id`, this function should return `absl::nullopt`. - virtual absl::optional GetEffectState(int effect_id) = 0; + virtual absl::optional GetEffectState(VcEffectId effect_id) = 0; // Invoked anytime the user makes an adjustment to an effect state. For // delegates that host more than a single effect, `effect_id` is the unique ID @@ -57,7 +59,7 @@ class ASH_EXPORT VcEffectsDelegate { // ignored and `absl::nullopt` should be passed. Similarly, `state` should be // `absl::nullopt` in cases (like toggle effects) where no specific state is // being set, an integer value otherwise. - virtual void OnEffectControlActivated(absl::optional effect_id, + virtual void OnEffectControlActivated(VcEffectId effect_id, absl::optional state) = 0; private: diff --git a/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.cc b/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.cc index 763ebebd030ce..805178b164087 100644 --- a/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.cc +++ b/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.cc @@ -9,9 +9,6 @@ namespace ash { -// static -const int VcEffectState::kUnusedId = -1; - VcEffectState::VcEffectState(const gfx::VectorIcon* icon, const std::u16string& label_text, int accessible_name_id, @@ -28,10 +25,9 @@ VcEffectState::VcEffectState(const gfx::VectorIcon* icon, VcEffectState::~VcEffectState() = default; VcHostedEffect::VcHostedEffect(VcEffectType type, - GetEffectStateCallback get_state_callback) - : type_(type), - get_state_callback_(get_state_callback), - id_(VcEffectState::kUnusedId) {} + GetEffectStateCallback get_state_callback, + VcEffectId effect_id) + : type_(type), get_state_callback_(get_state_callback), id_(effect_id) {} VcHostedEffect::~VcHostedEffect() = default; diff --git a/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.h b/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.h index 1bbed7981c7da..a2b3444dcc09c 100644 --- a/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.h +++ b/ash/system/video_conference/effects/video_conference_tray_effects_manager_types.h @@ -22,10 +22,6 @@ namespace ash { // conference effect UI control that's being hosted by a `VcEffectsDelegate`. class ASH_EXPORT VcEffectState { public: - // Use this in cases where an ID needs to be specified but isn't actually - // used. - static const int kUnusedId; - // Arguments: // // `icon` - The icon displayed, used for all effect types (if non-nullptr). @@ -88,6 +84,18 @@ enum class VcEffectType { kSetValue = 1, }; +// Represents all the available effects in the Video Conference panel. Each +// effect must have its own id for the purpose of metrics collection, unless it +// is for testing. +enum class VcEffectId { + kTestEffect = -1, + kBackgroundBlur = 0, + kPortraitRelighting = 1, + kNoiseCancellation = 2, + kLiveCaption = 3, + kMaxValue = kLiveCaption, +}; + // Represents a single video conference effect that's being "hosted" by an // implementer of the `VcEffectsDelegate` interface, used to construct the // effect's UI and perform any action that's needed to change the state of the @@ -113,8 +121,9 @@ class ASH_EXPORT VcHostedEffect { // `type` is the type of value adjustment allowed. // `get_state_callback` is invoked to obtain the current state of the effect. - explicit VcHostedEffect(VcEffectType type, - GetEffectStateCallback get_state_callback); + VcHostedEffect(VcEffectType type, + GetEffectStateCallback get_state_callback, + VcEffectId effect_id); VcHostedEffect(const VcHostedEffect&) = delete; VcHostedEffect& operator=(const VcHostedEffect&) = delete; @@ -136,8 +145,7 @@ class ASH_EXPORT VcHostedEffect { return get_state_callback_; } - void set_id(int id) { id_ = id; } - int id() const { return id_; } + VcEffectId id() const { return id_; } void set_label_text(const std::u16string label_text) { label_text_ = label_text; @@ -164,8 +172,8 @@ class ASH_EXPORT VcHostedEffect { // state of the effect. GetEffectStateCallback get_state_callback_; - // Unique ID of the effect, if desired. - int id_; + // Unique ID of the effect. + const VcEffectId id_; // Label text for the effect (that's separate from the label text of // individual child states).