Skip to content

Commit

Permalink
assistant: Remove LibAssistantV2 flag
Browse files Browse the repository at this point in the history
We launched the LibAssistantV2 in M-115 and we can remove the flag now.

Bug: b/300168375
Test: passed the tests
Change-Id: I8c8d763c740ae038faea6a8891981c3874810925
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4989762
Reviewed-by: Yuki Awano <yawano@google.com>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217260}
  • Loading branch information
Tao Wu authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent a1cdb19 commit d2a4ff8
Show file tree
Hide file tree
Showing 23 changed files with 111 additions and 475 deletions.
17 changes: 0 additions & 17 deletions chrome/browser/about_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3069,8 +3069,6 @@ constexpr char kWallpaperFastRefreshInternalName[] = "wallpaper-fast-refresh";
constexpr char kWallpaperGooglePhotosSharedAlbumsInternalName[] =
"wallpaper-google-photos-shared-albums";
constexpr char kWallpaperPerDeskName[] = "per-desk-wallpaper";
constexpr char kLibAssistantV2MigrationInternalName[] =
"cros-libassistant-v2-migration";
constexpr char kTimeOfDayWallpaperInternalName[] = "time-of-day-wallpaper";
constexpr char kTimeOfDayScreenSaverInternalName[] = "time-of-day-screen-saver";
constexpr char kTimeOfDayDlcInternalName[] = "time-of-day-dlc";
Expand Down Expand Up @@ -10282,13 +10280,6 @@ const FeatureEntry kFeatureEntries[] = {
FEATURE_VALUE_TYPE(enterprise_auth::kCloudApAuthAttachAsHeader)},
#endif // BUILDFLAG(IS_WIN)

#if BUILDFLAG(IS_CHROMEOS_ASH)
{kLibAssistantV2MigrationInternalName,
flag_descriptions::kLibAssistantV2MigrationName,
flag_descriptions::kLibAssistantV2MigrationDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::assistant::features::kEnableLibAssistantV2)},
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

#if BUILDFLAG(IS_CHROMEOS_ASH)
{"enable-per-desk-z-order", flag_descriptions::kEnablePerDeskZOrderName,
flag_descriptions::kEnablePerDeskZOrderDescription, kOsCrOS,
Expand Down Expand Up @@ -11440,14 +11431,6 @@ bool ShouldSkipConditionalFeatureEntry(const flags_ui::FlagsStorage* storage,
channel != version_info::Channel::UNKNOWN;
}

// Only show LibAssistant V2 migration flag if channel is one of
// Dev/Canary/Unknown.
if (!strcmp(kLibAssistantV2MigrationInternalName, entry.internal_name)) {
return (channel != version_info::Channel::DEV &&
channel != version_info::Channel::CANARY &&
channel != version_info::Channel::UNKNOWN);
}

// Disable and prevent users from enabling Floss on boards that were
// explicitly built without it (b/228902194 for more info).
if (!strcmp(kBluetoothUseFlossInternalName, entry.internal_name)) {
Expand Down
4 changes: 0 additions & 4 deletions chrome/browser/flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7201,10 +7201,6 @@ const char kOobeSimonName[] = "Simon features for OOBE";
const char kOobeSimonDescription[] =
"Feature to enable the Simon features in out of box experience.";

const char kLibAssistantV2MigrationName[] = "LibAssistant V2 migration";
const char kLibAssistantV2MigrationDescription[] =
"Enables loading LibAssistant V2 for Assistant on ChromeOS.";

const char kLacrosSharedComponentsDirName[] =
"Place browser components in a shared location";
const char kLacrosSharedComponentsDirDescription[] =
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -4140,8 +4140,6 @@ extern const char kOobeJellyModalDescription[];
extern const char kOobeSimonName[];
extern const char kOobeSimonDescription[];

extern const char kLibAssistantV2MigrationName[];
extern const char kLibAssistantV2MigrationDescription[];
// Prefer keeping this section sorted to adding new declarations down here.

extern const char kLacrosSharedComponentsDirName[];
Expand Down
16 changes: 4 additions & 12 deletions chrome/browser/ui/ash/assistant/test_support/fake_s3_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/ash/services/assistant/service.h"
#include "chromeos/assistant/internal/internal_constants.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -211,20 +210,13 @@ void FakeS3Server::StartS3ServerProcess(FakeS3Mode mode) {
}

base::FilePath fake_s3_server_main;
if (assistant::features::IsLibAssistantV2Enabled()) {
fake_s3_server_main =
GetExecutableDir().Append(FILE_PATH_LITERAL(kFakeS3ServerBinaryV2));
} else {
fake_s3_server_main =
GetExecutableDir().Append(FILE_PATH_LITERAL(kFakeS3ServerBinary));
}
fake_s3_server_main =
GetExecutableDir().Append(FILE_PATH_LITERAL(kFakeS3ServerBinaryV2));

base::CommandLine command_line(fake_s3_server_main);
AppendArgument(&command_line, "--port", base::NumberToString(port()));
if (assistant::features::IsLibAssistantV2Enabled()) {
AppendArgument(&command_line, "--http_port",
base::NumberToString(port() + 1));
}
AppendArgument(&command_line, "--http_port",
base::NumberToString(port() + 1));
AppendArgument(&command_line, "--mode", FakeS3ModeToString(mode));
AppendArgument(&command_line, "--auth_token", GetAccessToken());
AppendArgument(&command_line, "--test_data_file", GetTestDataFileName());
Expand Down
8 changes: 0 additions & 8 deletions chromeos/ash/services/assistant/public/cpp/features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ BASE_FEATURE(kDisableVoiceMatch,
"DisableVoiceMatch",
base::FEATURE_DISABLED_BY_DEFAULT);

BASE_FEATURE(kEnableLibAssistantV2,
"LibAssistantV2",
base::FEATURE_ENABLED_BY_DEFAULT);

BASE_FEATURE(kEnableLibAssistantDLC,
"LibAssistantDLC",
base::FEATURE_ENABLED_BY_DEFAULT);
Expand Down Expand Up @@ -112,10 +108,6 @@ bool IsLibAssistantSandboxEnabled() {
sandbox::policy::switches::kNoSandbox);
}

bool IsLibAssistantV2Enabled() {
return base::FeatureList::IsEnabled(kEnableLibAssistantV2);
}

bool IsLibAssistantDLCEnabled() {
return base::FeatureList::IsEnabled(kEnableLibAssistantDLC);
}
Expand Down
6 changes: 0 additions & 6 deletions chromeos/ash/services/assistant/public/cpp/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ BASE_DECLARE_FEATURE(kEnablePowerManager);
COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
BASE_DECLARE_FEATURE(kEnableLibAssistantBetaBackend);

// Enables the LibAssistantV2 APIs and related features.
COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
BASE_DECLARE_FEATURE(kEnableLibAssistantV2);

COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
BASE_DECLARE_FEATURE(kEnableLibAssistantDLC);

Expand Down Expand Up @@ -85,8 +81,6 @@ COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsWaitSchedulingEnabled();
COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
bool IsLibAssistantSandboxEnabled();

COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsLibAssistantV2Enabled();

COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsLibAssistantDLCEnabled();

COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsAssistantLearnMoreEnabled();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "base/test/scoped_feature_list.h"
#include "base/test/task_environment.h"
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/ash/services/libassistant/libassistant_service.h"
#include "chromeos/ash/services/libassistant/public/mojom/authentication_state_observer.mojom.h"
#include "chromeos/ash/services/libassistant/test_support/libassistant_service_tester.h"
Expand All @@ -28,8 +26,9 @@ std::vector<int> GetAuthenticationErrorCodes() {

std::vector<int> result;
for (int code = kMinErrorCode; code <= kMaxErrorCode; ++code) {
if (chromeos::assistant::IsAuthError(code))
if (chromeos::assistant::IsAuthError(code)) {
result.push_back(code);
}
}

return result;
Expand Down Expand Up @@ -98,29 +97,23 @@ class AuthenticationStateObserverTest : public ::testing::Test {
void FlushMojomPipes() { service_tester_.FlushForTesting(); }

void OnCommunicationError(int error_code) {
if (assistant::features::IsLibAssistantV2Enabled()) {
if (!chromeos::assistant::IsAuthError(error_code)) {
return;
}

::assistant::api::OnDeviceStateEventRequest request;
auto* communication_error =
request.mutable_event()->mutable_on_communication_error();
communication_error->set_error_code(
::assistant::api::events::DeviceStateEvent::OnCommunicationError::
AUTH_TOKEN_FAIL);

service_tester_.service()
.conversation_controller()
.OnGrpcMessageForTesting(request);
} else {
assistant_manager_delegate().OnCommunicationError(error_code);
if (!chromeos::assistant::IsAuthError(error_code)) {
return;
}

::assistant::api::OnDeviceStateEventRequest request;
auto* communication_error =
request.mutable_event()->mutable_on_communication_error();
communication_error->set_error_code(
::assistant::api::events::DeviceStateEvent::OnCommunicationError::
AUTH_TOKEN_FAIL);

service_tester_.service().conversation_controller().OnGrpcMessageForTesting(
request);
}

private:
base::test::SingleThreadTaskEnvironment environment_;
base::test::ScopedFeatureList feature_list_;
::testing::StrictMock<AuthenticationStateObserverMock> observer_mock_;
LibassistantServiceTester service_tester_;
};
Expand Down
17 changes: 3 additions & 14 deletions chromeos/ash/services/libassistant/conversation_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -235,26 +235,15 @@ void ConversationController::AddAuthenticationStateObserver(
authentication_state_observers_.Add(std::move(observer));
}

void ConversationController::OnAssistantClientCreated(
AssistantClient* assistant_client) {
if (!assistant::features::IsLibAssistantV2Enabled()) {
// Registers ActionModule when AssistantClient has been created but not yet
// started.
assistant_client->RegisterActionModule(action_module_.get());
}
}

void ConversationController::OnAssistantClientRunning(
AssistantClient* assistant_client) {
// Only when Libassistant is running we can start sending queries.
assistant_client_ = assistant_client;
requests_are_allowed_ = true;

if (assistant::features::IsLibAssistantV2Enabled()) {
// Register the action module when all libassistant services are ready.
// `action_module_` outlives gRPC services.
assistant_client->RegisterActionModule(action_module_.get());
}
// Register the action module when all libassistant services are ready.
// `action_module_` outlives gRPC services.
assistant_client->RegisterActionModule(action_module_.get());

assistant_client_->AddConversationStateEventObserver(events_observer_.get());
assistant_client_->AddDeviceStateEventObserver(events_observer_.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) ConversationController
mojo::PendingRemote<mojom::AuthenticationStateObserver> observer);

// AssistantClientObserver:
void OnAssistantClientCreated(AssistantClient* assistant_client) override;
void OnAssistantClientRunning(AssistantClient* assistant_client) override;
void OnDestroyingAssistantClient(AssistantClient* assistant_client) override;

Expand Down
22 changes: 8 additions & 14 deletions chromeos/ash/services/libassistant/grpc/assistant_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include "base/functional/callback_helpers.h"
#include "base/notreached.h"
#include "base/system/sys_info.h"
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/ash/services/libassistant/callback_utils.h"
#include "chromeos/ash/services/libassistant/grpc/assistant_client_v1.h"
#include "chromeos/ash/services/libassistant/grpc/external_services/action_service.h"
Expand Down Expand Up @@ -435,19 +434,14 @@ void AssistantClientImpl::AddAlarmTimerEventObserver(
std::unique_ptr<AssistantClient> AssistantClient::Create(
std::unique_ptr<assistant_client::AssistantManager> assistant_manager,
assistant_client::AssistantManagerInternal* assistant_manager_internal) {
if (assistant::features::IsLibAssistantV2Enabled()) {
const bool is_chromeos_device = base::SysInfo::IsRunningOnChromeOS();
// Note that we should *not* depend on |assistant_manager_internal| for V2,
// so |assistant_manager_internal| will be nullptr after the migration has
// done.
return std::make_unique<AssistantClientImpl>(
std::move(assistant_manager), assistant_manager_internal,
chromeos::assistant::GetLibassistantServiceAddress(is_chromeos_device),
chromeos::assistant::GetAssistantServiceAddress(is_chromeos_device));
}

return std::make_unique<AssistantClientV1>(std::move(assistant_manager),
assistant_manager_internal);
const bool is_chromeos_device = base::SysInfo::IsRunningOnChromeOS();
// Note that we should *not* depend on |assistant_manager_internal| for V2,
// so |assistant_manager_internal| will be nullptr after the migration has
// done.
return std::make_unique<AssistantClientImpl>(
std::move(assistant_manager), assistant_manager_internal,
chromeos::assistant::GetLibassistantServiceAddress(is_chromeos_device),
chromeos::assistant::GetAssistantServiceAddress(is_chromeos_device));
}

} // namespace ash::libassistant
13 changes: 0 additions & 13 deletions chromeos/ash/services/libassistant/grpc/assistant_client_v1.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include "base/synchronization/lock.h"
#include "base/task/sequenced_task_runner.h"
#include "base/time/time.h"
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/ash/services/libassistant/callback_utils.h"
#include "chromeos/ash/services/libassistant/grpc/assistant_client.h"
#include "chromeos/ash/services/libassistant/grpc/utils/media_status_utils.h"
Expand Down Expand Up @@ -137,11 +136,6 @@ class AssistantClientV1::DeviceStateListener

// Now |AssistantManager| is fully started, add media manager listener.
assistant_client_->AddMediaManagerListener();

// We will be checking the heartbeat signal sent back for Libassistant for
// v2.
if (!assistant::features::IsLibAssistantV2Enabled())
assistant_client_->NotifyAllServicesReady();
}

private:
Expand Down Expand Up @@ -337,13 +331,6 @@ void AssistantClientV1::StartServices(
ServicesStatusObserver* services_status_observer) {
DCHECK(services_status_observer);
services_status_observer_ = services_status_observer;

// Instead we will be checking the heartbeat signal sent back from Libassisant
// in v2.
if (!assistant::features::IsLibAssistantV2Enabled()) {
services_status_observer_->OnServicesStatusChanged(
ServicesStatus::ONLINE_BOOTING_UP);
}
}

void AssistantClientV1::SetChromeOSApiDelegate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

#include "base/functional/callback_helpers.h"
#include "base/test/task_environment.h"
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/ash/services/libassistant/grpc/assistant_client_v1.h"
#include "chromeos/ash/services/libassistant/grpc/services_status_observer.h"
#include "chromeos/assistant/internal/test_support/fake_assistant_manager.h"
Expand Down Expand Up @@ -102,13 +101,6 @@ class AssistantClientV1Test : public testing::Test {

TEST_F(AssistantClientV1Test, ShouldNotifyServicesStarted) {
MockServicesStatusObserver services_status_observer;

// If LibAssistantV2 is enabled, this will no be called.
if (!assistant::features::IsLibAssistantV2Enabled()) {
EXPECT_CALL(services_status_observer,
OnServicesStatusChanged(ServicesStatus::ONLINE_BOOTING_UP));
}

StartServices(&services_status_observer);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,7 @@ base::FilePath GetLibassisantPath(const std::string& root_path) {
DCHECK(root_path == kLibAssistantDlcRootPath);
base::FilePath libassistant_dlc_root =
base::FilePath(root_path).AsEndingWithSeparator();
if (assistant::features::IsLibAssistantV2Enabled()) {
return libassistant_dlc_root.Append(base::FilePath(kLibAssistantV2DlcPath));
}

return libassistant_dlc_root.Append(base::FilePath(kLibAssistantV1DlcPath));
return libassistant_dlc_root.Append(base::FilePath(kLibAssistantV2DlcPath));
}

void RecordLibassistantDlcInstallResult(
Expand Down
37 changes: 13 additions & 24 deletions chromeos/ash/services/libassistant/media_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "base/memory/raw_ptr.h"
#include "base/strings/stringprintf.h"
#include "base/test/task_environment.h"
#include "chromeos/ash/services/assistant/public/cpp/features.h"
#include "chromeos/ash/services/libassistant/grpc/utils/media_status_utils.h"
#include "chromeos/ash/services/libassistant/public/mojom/media_controller.mojom.h"
#include "chromeos/ash/services/libassistant/test_support/fake_assistant_client.h"
Expand Down Expand Up @@ -142,33 +141,23 @@ class AssistantMediaControllerTest : public testing::Test {
}

void SendPlaybackState(const assistant_client::MediaStatus& input) {
if (assistant::features::IsLibAssistantV2Enabled()) {
::assistant::api::OnDeviceStateEventRequest request;
auto* status = request.mutable_event()
->mutable_on_state_changed()
->mutable_new_state()
->mutable_media_status();
ConvertMediaStatusToV2FromV1(input, status);
media_controller().SendGrpcMessageForTesting(request);
} else {
libassistant_media_manager().listener().OnPlaybackStateChange(input);
}
::assistant::api::OnDeviceStateEventRequest request;
auto* status = request.mutable_event()
->mutable_on_state_changed()
->mutable_new_state()
->mutable_media_status();
ConvertMediaStatusToV2FromV1(input, status);
media_controller().SendGrpcMessageForTesting(request);
}

void CallFallbackMediaHandler(const std::string& action,
const std::string& action_proto) {
if (assistant::features::IsLibAssistantV2Enabled()) {
::assistant::api::OnMediaActionFallbackEventRequest request;
auto* media_action =
request.mutable_event()->mutable_on_media_action_event();
media_action->set_action_name(action);
media_action->set_action_args(action_proto);
media_controller().SendGrpcMessageForTesting(request);
} else {
auto handler =
service_tester_.assistant_manager_internal().media_action_fallback();
handler(action, action_proto);
}
::assistant::api::OnMediaActionFallbackEventRequest request;
auto* media_action =
request.mutable_event()->mutable_on_media_action_event();
media_action->set_action_name(action);
media_action->set_action_args(action_proto);
media_controller().SendGrpcMessageForTesting(request);
}

void FlushMojomPipes() {
Expand Down

0 comments on commit d2a4ff8

Please sign in to comment.