Fix hanging UI on invalid Assistant query.
In |AssistantInteractionController::OnOpenAppResponse| we forgot to call the callback if it is invoked outside of a valid conversation. This blocks the entire |AssistantInteractionSubscriber| mojom pipeline, causing the Assistant UI to become unresponsive. The fix was easy (call the callback), but I used this opportunity to add the first unittests for the |AssistantInteractionController|. Bug: b/146227377 Change-Id: I705aed3d4cabf3761de339d7d1c8a1ce6178b90d Tests: New ash_unittests AssistantInteractionController* Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2029607 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org> Commit-Queue: Jeroen Dhollander <jeroendh@google.com> Cr-Commit-Position: refs/heads/master@{#737058}
This commit is contained in:

committed by
Commit Bot

parent
100f870d33
commit
4c26a82d98
@ -1706,6 +1706,7 @@ test("ash_unittests") {
|
||||
"app_menu/notification_menu_view_unittest.cc",
|
||||
"app_menu/notification_overflow_view_unittest.cc",
|
||||
"assistant/assistant_alarm_timer_controller_unittest.cc",
|
||||
"assistant/assistant_interaction_controller_unittest.cc",
|
||||
"assistant/assistant_notification_controller_unittest.cc",
|
||||
"assistant/assistant_screen_context_controller_unittest.cc",
|
||||
"assistant/assistant_state_controller_unittest.cc",
|
||||
@ -2333,6 +2334,8 @@ static_library("test_support") {
|
||||
"test/ash_test_suite.h",
|
||||
"test/ash_test_views_delegate.cc",
|
||||
"test/ash_test_views_delegate.h",
|
||||
"test/fake_android_intent_helper.cc",
|
||||
"test/fake_android_intent_helper.h",
|
||||
"test/ui_controls_factory_ash.cc",
|
||||
"test/ui_controls_factory_ash.h",
|
||||
"test_media_client.cc",
|
||||
|
@ -714,8 +714,10 @@ void AssistantInteractionController::OnOpenUrlResponse(const GURL& url,
|
||||
void AssistantInteractionController::OnOpenAppResponse(
|
||||
chromeos::assistant::mojom::AndroidAppInfoPtr app_info,
|
||||
OnOpenAppResponseCallback callback) {
|
||||
if (!HasActiveInteraction())
|
||||
if (!HasActiveInteraction()) {
|
||||
std::move(callback).Run(false);
|
||||
return;
|
||||
}
|
||||
|
||||
auto* android_helper = AndroidIntentHelper::GetInstance();
|
||||
if (!android_helper) {
|
||||
@ -793,10 +795,9 @@ void AssistantInteractionController::OnProcessPendingResponse() {
|
||||
}
|
||||
|
||||
// Start processing.
|
||||
model_.pending_response()->Process(
|
||||
base::BindOnce(
|
||||
&AssistantInteractionController::OnPendingResponseProcessed,
|
||||
weak_factory_.GetWeakPtr()));
|
||||
model_.pending_response()->Process(base::BindOnce(
|
||||
&AssistantInteractionController::OnPendingResponseProcessed,
|
||||
weak_factory_.GetWeakPtr()));
|
||||
}
|
||||
|
||||
void AssistantInteractionController::OnPendingResponseProcessed(bool success) {
|
||||
|
143
ash/assistant/assistant_interaction_controller_unittest.cc
Normal file
143
ash/assistant/assistant_interaction_controller_unittest.cc
Normal file
@ -0,0 +1,143 @@
|
||||
// Copyright 2020 The Chromium Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style license that can be
|
||||
// found in the LICENSE file.
|
||||
|
||||
#include "ash/assistant/assistant_interaction_controller.h"
|
||||
#include "ash/assistant/test/assistant_ash_test_base.h"
|
||||
#include "ash/test/fake_android_intent_helper.h"
|
||||
#include "base/bind.h"
|
||||
#include "chromeos/services/assistant/public/mojom/assistant.mojom-forward.h"
|
||||
#include "testing/gmock/include/gmock/gmock.h"
|
||||
|
||||
namespace ash {
|
||||
|
||||
namespace {
|
||||
|
||||
using chromeos::assistant::mojom::AndroidAppInfo;
|
||||
using chromeos::assistant::mojom::AndroidAppInfoPtr;
|
||||
using chromeos::assistant::mojom::AssistantInteractionMetadata;
|
||||
using chromeos::assistant::mojom::AssistantInteractionType;
|
||||
using ::testing::StrictMock;
|
||||
|
||||
constexpr bool kErrorResult = false;
|
||||
constexpr bool kSuccessResult = true;
|
||||
|
||||
// A simple mock to allow mocking of the callback passed to |OnOpenAppResponse|.
|
||||
// Example usage:
|
||||
//
|
||||
// OpenAppCallbackMock callback;
|
||||
//
|
||||
// EXPECT_CALL(callback, Callback(true));
|
||||
// FunctionTakingACallback(callback.Bind());
|
||||
class OpenAppCallbackMock {
|
||||
public:
|
||||
MOCK_METHOD(void, Callback, (bool));
|
||||
|
||||
auto Bind() {
|
||||
return base::BindOnce(&OpenAppCallbackMock::Callback,
|
||||
base::Unretained(this));
|
||||
}
|
||||
};
|
||||
|
||||
class AssistantInteractionControllerTest : public AssistantAshTestBase {
|
||||
public:
|
||||
AssistantInteractionControllerTest() = default;
|
||||
|
||||
void StartInteraction() {
|
||||
interaction_controller()->OnInteractionStarted(
|
||||
AssistantInteractionMetadata::New());
|
||||
}
|
||||
|
||||
AndroidAppInfoPtr CreateAndroidAppInfo(
|
||||
const std::string& app_name = "unknown") {
|
||||
AndroidAppInfoPtr result = AndroidAppInfo::New();
|
||||
result->localized_app_name = app_name;
|
||||
return result;
|
||||
}
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
TEST_F(AssistantInteractionControllerTest,
|
||||
ShouldBecomeActiveWhenInteractionStarts) {
|
||||
EXPECT_EQ(interaction_model()->interaction_state(),
|
||||
InteractionState::kInactive);
|
||||
|
||||
interaction_controller()->OnInteractionStarted(
|
||||
AssistantInteractionMetadata::New());
|
||||
|
||||
EXPECT_EQ(interaction_model()->interaction_state(),
|
||||
InteractionState::kActive);
|
||||
}
|
||||
|
||||
TEST_F(AssistantInteractionControllerTest,
|
||||
ShouldCallCallbackWhenOpenAppIsCalledWhileInactive) {
|
||||
StrictMock<OpenAppCallbackMock> callback;
|
||||
|
||||
EXPECT_CALL(callback, Callback(kErrorResult));
|
||||
EXPECT_EQ(interaction_model()->interaction_state(),
|
||||
InteractionState::kInactive);
|
||||
|
||||
interaction_controller()->OnOpenAppResponse(CreateAndroidAppInfo(),
|
||||
callback.Bind());
|
||||
}
|
||||
|
||||
TEST_F(AssistantInteractionControllerTest,
|
||||
ShouldCallCallbackWhenOpenAppIsCalledWithoutAnAndroidIntentHelper) {
|
||||
StrictMock<OpenAppCallbackMock> callback;
|
||||
|
||||
StartInteraction();
|
||||
EXPECT_CALL(callback, Callback(kErrorResult));
|
||||
|
||||
interaction_controller()->OnOpenAppResponse(CreateAndroidAppInfo(),
|
||||
callback.Bind());
|
||||
}
|
||||
|
||||
TEST_F(AssistantInteractionControllerTest,
|
||||
ShouldCallCallbackWhenOpenAppIsCalledForUnknownAndroidApp) {
|
||||
StrictMock<OpenAppCallbackMock> callback;
|
||||
|
||||
StartInteraction();
|
||||
FakeAndroidIntentHelper fake_helper;
|
||||
EXPECT_CALL(callback, Callback(kErrorResult));
|
||||
|
||||
interaction_controller()->OnOpenAppResponse(
|
||||
CreateAndroidAppInfo("unknown-app-name"), callback.Bind());
|
||||
}
|
||||
|
||||
TEST_F(AssistantInteractionControllerTest,
|
||||
ShouldLaunchAppAndCallCallbackWhenOpenAppIsCalled) {
|
||||
StrictMock<OpenAppCallbackMock> callback;
|
||||
const std::string app_name = "AppName";
|
||||
const std::string intent = "intent://AppName";
|
||||
|
||||
StartInteraction();
|
||||
FakeAndroidIntentHelper fake_helper;
|
||||
fake_helper.AddApp(app_name, intent);
|
||||
EXPECT_CALL(callback, Callback(kSuccessResult));
|
||||
|
||||
interaction_controller()->OnOpenAppResponse(CreateAndroidAppInfo(app_name),
|
||||
callback.Bind());
|
||||
|
||||
EXPECT_EQ(intent, fake_helper.last_launched_android_intent());
|
||||
}
|
||||
|
||||
TEST_F(AssistantInteractionControllerTest,
|
||||
ShouldAddSchemeToIntentWhenLaunchingAndroidApp) {
|
||||
StrictMock<OpenAppCallbackMock> callback;
|
||||
const std::string app_name = "AppName";
|
||||
const std::string intent = "#Intent-without-a-scheme";
|
||||
const std::string intent_with_scheme = "intent://" + intent;
|
||||
|
||||
StartInteraction();
|
||||
FakeAndroidIntentHelper fake_helper;
|
||||
fake_helper.AddApp(app_name, intent);
|
||||
EXPECT_CALL(callback, Callback(kSuccessResult));
|
||||
|
||||
interaction_controller()->OnOpenAppResponse(CreateAndroidAppInfo(app_name),
|
||||
callback.Bind());
|
||||
|
||||
EXPECT_EQ(intent_with_scheme, fake_helper.last_launched_android_intent());
|
||||
}
|
||||
|
||||
} // namespace ash
|
@ -260,6 +260,10 @@ AssistantInteractionController* AssistantAshTestBase::interaction_controller() {
|
||||
return controller_->interaction_controller();
|
||||
}
|
||||
|
||||
const AssistantInteractionModel* AssistantAshTestBase::interaction_model() {
|
||||
return interaction_controller()->model();
|
||||
}
|
||||
|
||||
TestAssistantService* AssistantAshTestBase::assistant_service() {
|
||||
return ash_test_helper()->test_assistant_service();
|
||||
}
|
||||
|
@ -28,6 +28,7 @@ namespace ash {
|
||||
|
||||
class AssistantController;
|
||||
class AssistantInteractionController;
|
||||
class AssistantInteractionModel;
|
||||
class AssistantTestApi;
|
||||
class TestAssistantService;
|
||||
class TestAssistantWebViewFactory;
|
||||
@ -152,8 +153,10 @@ class AssistantAshTestBase : public AshTestBase {
|
||||
void EnableKeyboard() { SetVirtualKeyboardEnabled(true); }
|
||||
void DisableKeyboard() { SetVirtualKeyboardEnabled(false); }
|
||||
|
||||
private:
|
||||
AssistantInteractionController* interaction_controller();
|
||||
const AssistantInteractionModel* interaction_model();
|
||||
|
||||
private:
|
||||
TestAssistantService* assistant_service();
|
||||
|
||||
std::unique_ptr<AssistantTestApi> test_api_;
|
||||
|
29
ash/test/fake_android_intent_helper.cc
Normal file
29
ash/test/fake_android_intent_helper.cc
Normal file
@ -0,0 +1,29 @@
|
||||
// Copyright 2020 The Chromium Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style license that can be
|
||||
// found in the LICENSE file.
|
||||
|
||||
#include "ash/test/fake_android_intent_helper.h"
|
||||
|
||||
namespace ash {
|
||||
|
||||
FakeAndroidIntentHelper::FakeAndroidIntentHelper() = default;
|
||||
FakeAndroidIntentHelper::~FakeAndroidIntentHelper() = default;
|
||||
|
||||
void FakeAndroidIntentHelper::LaunchAndroidIntent(const std::string& intent) {
|
||||
last_launched_intent_ = intent;
|
||||
}
|
||||
|
||||
base::Optional<std::string> FakeAndroidIntentHelper::GetAndroidAppLaunchIntent(
|
||||
chromeos::assistant::mojom::AndroidAppInfoPtr app_info) {
|
||||
auto iterator = apps_.find(app_info->localized_app_name);
|
||||
if (iterator != apps_.end())
|
||||
return iterator->second;
|
||||
return base::nullopt;
|
||||
}
|
||||
|
||||
void FakeAndroidIntentHelper::AddApp(const LocalizedAppName& name,
|
||||
const Intent& intent) {
|
||||
apps_[name] = intent;
|
||||
}
|
||||
|
||||
} // namespace ash
|
51
ash/test/fake_android_intent_helper.h
Normal file
51
ash/test/fake_android_intent_helper.h
Normal file
@ -0,0 +1,51 @@
|
||||
// Copyright 2020 The Chromium Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style license that can be
|
||||
// found in the LICENSE file.
|
||||
|
||||
#ifndef ASH_TEST_FAKE_ANDROID_INTENT_HELPER_H_
|
||||
#define ASH_TEST_FAKE_ANDROID_INTENT_HELPER_H_
|
||||
|
||||
#include <map>
|
||||
#include <string>
|
||||
|
||||
#include "ash/public/cpp/android_intent_helper.h"
|
||||
#include "base/optional.h"
|
||||
|
||||
namespace ash {
|
||||
|
||||
// Fake instance of the |AndroidIntentHelper| used for test purposes.
|
||||
// Will be installed as the singleton when constructed, and allows the injection
|
||||
// of fake Android apps.
|
||||
class FakeAndroidIntentHelper : public AndroidIntentHelper {
|
||||
public:
|
||||
using LocalizedAppName = std::string;
|
||||
using Intent = std::string;
|
||||
|
||||
FakeAndroidIntentHelper();
|
||||
~FakeAndroidIntentHelper() override;
|
||||
FakeAndroidIntentHelper(FakeAndroidIntentHelper&) = delete;
|
||||
FakeAndroidIntentHelper& operator=(FakeAndroidIntentHelper&) = delete;
|
||||
|
||||
// AndroidIntentHelper overrides:
|
||||
void LaunchAndroidIntent(const std::string& intent) override;
|
||||
base::Optional<std::string> GetAndroidAppLaunchIntent(
|
||||
chromeos::assistant::mojom::AndroidAppInfoPtr app_info) override;
|
||||
|
||||
// Adds a fake Android app.
|
||||
// |intent| will be returned from GetAndroidAppLaunchIntent() if the value of
|
||||
// |AndroidAppInfo::localized_app_name| matches |name|.
|
||||
void AddApp(const LocalizedAppName& name, const Intent& intent);
|
||||
|
||||
// Returns the intent of the last Android app that was launched.
|
||||
const base::Optional<Intent>& last_launched_android_intent() const {
|
||||
return last_launched_intent_;
|
||||
}
|
||||
|
||||
private:
|
||||
std::map<LocalizedAppName, Intent> apps_;
|
||||
base::Optional<Intent> last_launched_intent_;
|
||||
};
|
||||
|
||||
} // namespace ash
|
||||
|
||||
#endif // ASH_TEST_FAKE_ANDROID_INTENT_HELPER_H_
|
Reference in New Issue
Block a user