Skip to content

GitLab

T
tangled
Commit 282a876f authored by Jeroen Dhollander's avatar Jeroen Dhollander Committed by Commit Bot
Browse files
Options

Remove dead code 

This code was meant to handle volume related queries in ChromeOS,
but these queries are actually handled inside LibAssistant.

The code I removed expects to handles of the volume through a
|device.MODIFY_SETTING| request.

Instead, LibAssistant gets a |device.UPDATE_VOLUME| request which it
handles here:
    https://cs.corp.google.com/libassistant/libassistant/internal/assistant/action/device/device_control_module.cc?type=cs&q=libassistant/internal/assistant/action/device/device_control_module.cc+package:%5Elibassistant$&g=0&l=339

After discussing this with the Assistant Device Actions team,
I learned that the server can either send device.MODIFY_SETTING or
device.UPDATE_VOLUME, based on the client preference.
Given that LibAssistant expresses a preference for device.UPDATE_VOLUME,
we can safely assume we will never receive a device.MODIFY_SETTING
request.

Test: queries "increase volume", "set volume to 20%", "increase volume by 5".
Bug: b/154040671
Change-Id: I763f4d1d1ace0924f81faf7f33eb7cc364e99d99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2157585
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761116}
parent b704ad07
Show whitespace changes
Inline Side-by-side
Showing with 11 additions and 107 deletions
chromeos/services/assistant/assistant_device_settings_delegate.cc
View file @ 282a876f
......@@ -39,7 +39,6 @@ namespace {
constexpr char kWiFiDeviceSettingId[] = "WIFI";
constexpr char kBluetoothDeviceSettingId[] = "BLUETOOTH";
constexpr char kVolumeLevelDeviceSettingId[] = "VOLUME_LEVEL";
constexpr char kScreenBrightnessDeviceSettingId[] = "BRIGHTNESS_LEVEL";
constexpr char kDoNotDisturbDeviceSettingId[] = "DO_NOT_DISTURB";
constexpr char kNightLightDeviceSettingId[] = "NIGHT_LIGHT_SWITCH";
......@@ -84,14 +83,14 @@ double ConvertSliderValueToLevel(double value,
double default_value) {
switch (unit) {
case client_op::ModifySettingArgs_Unit_RANGE:
// "set volume to 20%".
// "set brightness to 20%".
return value;
case client_op::ModifySettingArgs_Unit_STEP:
// "set volume to 20". Treat the step as a percentage.
// "set brightness to 20". Treat the step as a percentage.
return value / 100.0f;
// Currently, factor (e.g., 'double the volume') and decibel units aren't
// handled by the backend. This could change in the future.
// Currently, factor (e.g., 'double the brightness') and decibel units
// aren't handled by the backend. This could change in the future.
case client_op::ModifySettingArgs_Unit_FACTOR:
case client_op::ModifySettingArgs_Unit_DECIBEL:
break;
......@@ -231,29 +230,6 @@ class NightLightSetting : public SettingWithDeviceAction {
}
};
class VolumeSetting : public Setting {
public:
explicit VolumeSetting(assistant_client::VolumeControl* volume_control)
: volume_control_(volume_control) {}
const char* setting_id() const override {
return kVolumeLevelDeviceSettingId;
}
void Modify(const client_op::ModifySettingArgs& request) override {
DCHECK(volume_control_ != nullptr);
HandleSliderChange(
request,
[this](double value) {
this->volume_control_->SetSystemVolume(value, true);
},
[this]() { return this->volume_control_->GetSystemVolume(); });
}
private:
assistant_client::VolumeControl* const volume_control_;
};
class BrightnessSetting : public SettingWithDeviceAction {
public:
explicit BrightnessSetting(ServiceContext* context)
......@@ -289,14 +265,12 @@ class BrightnessSetting : public SettingWithDeviceAction {
} // namespace
AssistantDeviceSettingsDelegate::AssistantDeviceSettingsDelegate(
ServiceContext* context,
assistant_client::VolumeControl* volume_control) {
ServiceContext* context) {
AddSetting(std::make_unique<WifiSetting>(context));
AddSetting(std::make_unique<BluetoothSetting>(context));
AddSetting(std::make_unique<NightLightSetting>(context));
AddSetting(std::make_unique<DoNotDisturbSetting>(
context->assistant_notification_controller()));
AddSetting(std::make_unique<VolumeSetting>(volume_control));
AddSetting(std::make_unique<BrightnessSetting>(context));
if (base::CommandLine::ForCurrentProcess()->HasSwitch(
......
chromeos/services/assistant/assistant_device_settings_delegate.h
View file @ 282a876f
......@@ -12,10 +12,6 @@
#include "ash/public/mojom/assistant_controller.mojom-forward.h"
#include "chromeos/services/assistant/public/mojom/assistant.mojom-forward.h"
namespace assistant_client {
class VolumeControl;
} // namespace assistant_client
namespace assistant {
namespace api {
namespace client_op {
......@@ -36,9 +32,7 @@ class Setting;
// the device settings, like Bluetooth or WiFi.
class AssistantDeviceSettingsDelegate {
public:
explicit AssistantDeviceSettingsDelegate(
ServiceContext* context,
assistant_client::VolumeControl* volume_control);
explicit AssistantDeviceSettingsDelegate(ServiceContext* context);
AssistantDeviceSettingsDelegate(AssistantDeviceSettingsDelegate&) = delete;
AssistantDeviceSettingsDelegate& operator=(AssistantDeviceSettingsDelegate&) =
delete;
......
chromeos/services/assistant/assistant_device_settings_delegate_unittest.cc
View file @ 282a876f
......@@ -36,7 +36,6 @@ using ::testing::StrictMock;
constexpr char kWiFi[] = "WIFI";
constexpr char kBluetooth[] = "BLUETOOTH";
constexpr char kVolumeLevel[] = "VOLUME_LEVEL";
constexpr char kScreenBrightness[] = "BRIGHTNESS_LEVEL";
constexpr char kDoNotDisturb[] = "DO_NOT_DISTURB";
constexpr char kNightLight[] = "NIGHT_LIGHT_SWITCH";
......@@ -45,8 +44,7 @@ constexpr char kSwitchAccess[] = "SWITCH_ACCESS";
// Returns the settings that are always supported.
// Does not contain |SWITCH_ACCESS| as that is conditionally supported.
const std::vector<std::string> kAlwaysSupportedSettings = {
kWiFi, kBluetooth, kVolumeLevel, kScreenBrightness,
kDoNotDisturb, kNightLight,
kWiFi, kBluetooth, kScreenBrightness, kDoNotDisturb, kNightLight,
};
class DeviceActionsMock : public mojom::DeviceActions {
......@@ -86,17 +84,6 @@ class DeviceActionsMock : public mojom::DeviceActions {
double current_brightness_ = 0.0;
};
class VolumeControlMock : public assistant_client::VolumeControl {
public:
MOCK_METHOD(void, SetAudioFocus, (assistant_client::OutputStreamType));
MOCK_METHOD(float, GetSystemVolume, ());
MOCK_METHOD(void, SetSystemVolume, (float new_volume, bool user_initiated));
MOCK_METHOD(float, GetAlarmVolume, ());
MOCK_METHOD(void, SetAlarmVolume, (float new_volume, bool user_initiated));
MOCK_METHOD(bool, IsSystemMuted, ());
MOCK_METHOD(void, SetSystemMuted, (bool muted));
};
class AssistantNotificationControllerMock
: public ash::mojom::AssistantNotificationController {
public:
......@@ -142,10 +129,9 @@ class AssistantDeviceSettingsDelegateTest : public testing::Test {
AssistantDeviceSettingsDelegate* delegate() { return delegate_.get(); }
void CreateAssistantDeviceSettingsDelegate(
assistant_client::VolumeControl* volume_control = nullptr) {
void CreateAssistantDeviceSettingsDelegate() {
delegate_ = std::make_unique<AssistantDeviceSettingsDelegate>(
service_context_.get(), volume_control);
service_context_.get());
}
private:
......@@ -308,56 +294,6 @@ TEST_F(AssistantDeviceSettingsDelegateTest, ShouldTurnNightLightOnAndOff) {
delegate()->HandleModifyDeviceSetting(args);
}
TEST_F(AssistantDeviceSettingsDelegateTest, ShouldSetVolume) {
StrictMock<VolumeControlMock> volume_control;
CreateAssistantDeviceSettingsDelegate(&volume_control);
EXPECT_CALL(volume_control, GetSystemVolume).Times(AnyNumber());
ModifySettingArgs args;
args.set_setting_id(kVolumeLevel);
args.set_change(Change::ModifySettingArgs_Change_SET);
// Set volume to 20%
args.set_numeric_value(0.2);
args.set_unit(Unit::ModifySettingArgs_Unit_RANGE);
EXPECT_CALL(volume_control, SetSystemVolume(FloatNear(0.2, kEpsilon),
/*user_initiated=*/true));
delegate()->HandleModifyDeviceSetting(args);
// Set volume to 20.
// This will be converted to a percentage
args.set_numeric_value(20);
args.set_unit(Unit::ModifySettingArgs_Unit_STEP);
EXPECT_CALL(volume_control, SetSystemVolume(FloatNear(0.2, kEpsilon),
/*user_initiated=*/true));
delegate()->HandleModifyDeviceSetting(args);
}
TEST_F(AssistantDeviceSettingsDelegateTest, ShouldIncreaseAndDecreaseVolume) {
StrictMock<VolumeControlMock> volume_control;
CreateAssistantDeviceSettingsDelegate(&volume_control);
EXPECT_CALL(volume_control, GetSystemVolume).Times(AnyNumber());
ModifySettingArgs args;
args.set_setting_id(kVolumeLevel);
args.set_change(Change::ModifySettingArgs_Change_SET);
// Set volume to 20%
args.set_numeric_value(0.2);
args.set_unit(Unit::ModifySettingArgs_Unit_RANGE);
EXPECT_CALL(volume_control, SetSystemVolume(FloatNear(0.2, kEpsilon),
/*user_initiated=*/true));
delegate()->HandleModifyDeviceSetting(args);
// Set volume to 20.
// This will be converted to a percentage
args.set_numeric_value(20);
args.set_unit(Unit::ModifySettingArgs_Unit_STEP);
EXPECT_CALL(volume_control, SetSystemVolume(FloatNear(0.2, kEpsilon),
/*user_initiated=*/true));
delegate()->HandleModifyDeviceSetting(args);
}
TEST_F(AssistantDeviceSettingsDelegateTest, ShouldSetBrightness) {
StrictMock<DeviceActionsMock> device_actions;
service_context()->set_device_actions(&device_actions);
......
chromeos/services/assistant/assistant_manager_service_impl.cc
View file @ 282a876f
......@@ -169,8 +169,8 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
platform_api_ = delegate_->CreatePlatformApi(
media_session_.get(), background_thread_.task_runner());
settings_delegate_ = std::make_unique<AssistantDeviceSettingsDelegate>(
context, &platform_api_->GetAudioOutputProvider().GetVolumeControl());
settings_delegate_ =
std::make_unique<AssistantDeviceSettingsDelegate>(context);
mojo::Remote<media_session::mojom::MediaControllerManager>
media_controller_manager;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment