Commit 6f1cfb69 authored by Lucas Tenório's avatar Lucas Tenório Committed by Commit Bot

Skip other screens when finishing Supervision Onboarding.

With this change, we are showing the Supervision Onboarding screens right
after the ARC ToS screen. We are also skipping any screens that show after
a successful Supervision Onboarding.

This change also removes the ExitFlow() method for the WebviewHost
interface. Since this data was just looping back to the WebUI implementation,
we can just add a new C++ interface that bypasses the JS bridge.

TBR=ochang@chromium.org

Bug: 958995
Change-Id: I340c3899e8ca3d0bdb493ca487e682600467212a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1653388
Commit-Queue: Lucas Tenório <ltenorio@chromium.org>
Reviewed-by: default avatarMichael Giuffrida <michaelpg@chromium.org>
Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#668538}
parent 0e03d82f
......@@ -1968,6 +1968,7 @@ source_set("chromeos") {
"supervision/onboarding_constants.h",
"supervision/onboarding_controller_impl.cc",
"supervision/onboarding_controller_impl.h",
"supervision/onboarding_delegate.h",
"supervision/onboarding_flow_model.cc",
"supervision/onboarding_flow_model.h",
"supervision/onboarding_logger.cc",
......
......@@ -12,15 +12,10 @@
#include "components/version_info/version_info.h"
namespace chromeos {
namespace {
constexpr const char kFinishedUserAction[] = "setup-finished";
} // namespace
SupervisionOnboardingScreen::SupervisionOnboardingScreen(
SupervisionOnboardingScreenView* view,
const base::RepeatingClosure& exit_callback)
const ScreenExitCallback& exit_callback)
: BaseScreen(SupervisionOnboardingScreenView::kScreenId),
view_(view),
exit_callback_(exit_callback) {
......@@ -49,7 +44,7 @@ void SupervisionOnboardingScreen::Show() {
}
#endif
Exit();
SkipOnboarding();
}
void SupervisionOnboardingScreen::Hide() {
......@@ -57,22 +52,18 @@ void SupervisionOnboardingScreen::Hide() {
view_->Hide();
}
void SupervisionOnboardingScreen::OnUserAction(const std::string& action_id) {
if (action_id == kFinishedUserAction) {
Exit();
return;
}
BaseScreen::OnUserAction(action_id);
}
void SupervisionOnboardingScreen::OnViewDestroyed(
SupervisionOnboardingScreenView* view) {
if (view_ == view)
view_ = nullptr;
}
void SupervisionOnboardingScreen::Exit() {
exit_callback_.Run();
void SupervisionOnboardingScreen::SkipOnboarding() {
exit_callback_.Run(Result::kSkipped);
}
void SupervisionOnboardingScreen::FinishOnboarding() {
exit_callback_.Run(Result::kFinished);
}
} // namespace chromeos
......@@ -10,31 +10,41 @@
#include "base/callback.h"
#include "base/macros.h"
#include "chrome/browser/chromeos/login/screens/base_screen.h"
#include "chrome/browser/chromeos/supervision/onboarding_delegate.h"
namespace chromeos {
class SupervisionOnboardingScreenView;
class SupervisionOnboardingScreen : public BaseScreen {
class SupervisionOnboardingScreen : public BaseScreen,
public supervision::OnboardingDelegate {
public:
enum class Result {
// User reached the end of the flow and exited successfully.
kFinished,
// User chose to skip the flow or we skipped the flow for internal reasons.
kSkipped,
};
using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
SupervisionOnboardingScreen(SupervisionOnboardingScreenView* view,
const base::RepeatingClosure& exit_callback);
const ScreenExitCallback& exit_callback);
~SupervisionOnboardingScreen() override;
// BaseScreen:
void Show() override;
void Hide() override;
void OnUserAction(const std::string& action_id) override;
// Called when view is destroyed so there's no dead reference to it.
void OnViewDestroyed(SupervisionOnboardingScreenView* view);
// Called when supervision onboarding has finished, exits the screen.
void Exit();
private:
// supervision::OnboardingDelegate:
void SkipOnboarding() override;
void FinishOnboarding() override;
SupervisionOnboardingScreenView* view_;
base::RepeatingClosure exit_callback_;
ScreenExitCallback exit_callback_;
DISALLOW_COPY_AND_ASSIGN(SupervisionOnboardingScreen);
};
......
......@@ -274,7 +274,7 @@ class SupervisionOnboardingBaseTest : public MixinBasedInProcessBrowserTest {
SupervisionOnboardingScreen* supervision_onboarding_screen_;
private:
void HandleScreenExit() {
void HandleScreenExit(SupervisionOnboardingScreen::Result result) {
ASSERT_FALSE(screen_exited_);
screen_exited_ = true;
if (screen_exit_callback_)
......
......@@ -1078,14 +1078,7 @@ void WizardController::OnArcTermsOfServiceAccepted() {
return;
}
// If the recommend app screen should be shown, show it after the user
// finished with the PlayStore Terms of Service. Otherwise, advance to the
// assistant opt-in flow screen.
if (ShouldShowRecommendAppsScreen()) {
ShowRecommendAppsScreen();
} else {
ShowAssistantOptInFlowScreen();
}
ShowSupervisionOnboardingScreen();
}
void WizardController::OnRecommendAppsScreenExit(
......@@ -1117,7 +1110,7 @@ void WizardController::OnAssistantOptInFlowScreenExit() {
void WizardController::OnMultiDeviceSetupScreenExit() {
OnScreenExit(MultiDeviceSetupScreenView::kScreenId, 0 /* exit_code */);
ShowSupervisionOnboardingScreen();
OnOobeFlowFinished();
}
void WizardController::OnResetScreenExit() {
......@@ -1145,10 +1138,29 @@ void WizardController::OnDeviceModificationCanceled() {
}
}
void WizardController::OnSupervisionOnboardingScreenExit() {
OnScreenExit(SupervisionOnboardingScreenView::kScreenId, 0 /* exit_code */);
void WizardController::OnSupervisionOnboardingScreenExit(
SupervisionOnboardingScreen::Result result) {
OnScreenExit(SupervisionOnboardingScreenView::kScreenId,
static_cast<int>(result));
OnOobeFlowFinished();
// In this case, the user went through the whole Supervision Onboarding flow
// successfully, so we should just finish the OOBE/Login here.
// Note: This intentionally skips the other screens like Assistant and
// recommended app downloads.
if (result == SupervisionOnboardingScreen::Result::kFinished) {
OnOobeFlowFinished();
return;
}
// If the recommend app screen should be shown, show it after the user
// skipped the Supervision Onboarding. Otherwise, advance to the
// assistant opt-in flow screen.
if (ShouldShowRecommendAppsScreen()) {
ShowRecommendAppsScreen();
return;
}
ShowAssistantOptInFlowScreen();
}
void WizardController::OnSupervisionTransitionScreenExit() {
......
......@@ -30,6 +30,7 @@
#include "chrome/browser/chromeos/login/screens/kiosk_autolaunch_screen.h"
#include "chrome/browser/chromeos/login/screens/network_screen.h"
#include "chrome/browser/chromeos/login/screens/recommend_apps_screen.h"
#include "chrome/browser/chromeos/login/screens/supervision_onboarding_screen.h"
#include "chrome/browser/chromeos/login/screens/terms_of_service_screen.h"
#include "chrome/browser/chromeos/login/screens/update_screen.h"
#include "chrome/browser/chromeos/policy/enrollment_config.h"
......@@ -227,7 +228,8 @@ class WizardController {
void OnMultiDeviceSetupScreenExit();
void OnResetScreenExit();
void OnDeviceModificationCanceled();
void OnSupervisionOnboardingScreenExit();
void OnSupervisionOnboardingScreenExit(
SupervisionOnboardingScreen::Result result);
void OnSupervisionTransitionScreenExit();
void OnOobeFlowFinished();
......
......@@ -70,12 +70,6 @@ interface OnboardingWebviewHost {
// Requests the host to load the given page.
LoadPage(OnboardingPage page) => (OnboardingLoadPageResult result);
// Requests that the host exit the flow immediately. This might mean
// different things depending on the type of host. If we are running in OOBE
// we will exit the supervision screen and move to the next OOBE step, if we
// are running in a custom WebUI, we should close it.
ExitFlow();
};
// Result of a LoadPage call. Contains data about errors and custom data
......
......@@ -15,8 +15,9 @@
namespace chromeos {
namespace supervision {
OnboardingControllerImpl::OnboardingControllerImpl(Profile* profile)
: flow_model_(std::make_unique<OnboardingFlowModel>(profile)),
OnboardingControllerImpl::OnboardingControllerImpl(Profile* profile,
OnboardingDelegate* delegate)
: flow_model_(std::make_unique<OnboardingFlowModel>(profile, delegate)),
presenter_(std::make_unique<OnboardingPresenter>(flow_model_.get())),
logger_(std::make_unique<OnboardingLogger>(flow_model_.get())),
kiosk_next_observer_(
......
......@@ -16,6 +16,7 @@ class Profile;
namespace chromeos {
namespace supervision {
class OnboardingDelegate;
class OnboardingFlowModel;
class OnboardingPresenter;
class OnboardingLogger;
......@@ -23,7 +24,8 @@ class KioskNextFlowObserver;
class OnboardingControllerImpl : public mojom::OnboardingController {
public:
explicit OnboardingControllerImpl(Profile* profile);
explicit OnboardingControllerImpl(Profile* profile,
OnboardingDelegate* delegate);
~OnboardingControllerImpl() override;
void BindRequest(mojom::OnboardingControllerRequest request);
......
......@@ -12,6 +12,7 @@
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/chromeos/supervision/mojom/onboarding_controller.mojom.h"
#include "chrome/browser/chromeos/supervision/onboarding_constants.h"
#include "chrome/browser/chromeos/supervision/onboarding_delegate.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/test_extension_system.h"
#include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
......@@ -32,6 +33,32 @@ const char kFakeAccessToken[] = "fake-access-token";
} // namespace
class FakeOnboardingDelegate : public OnboardingDelegate {
public:
~FakeOnboardingDelegate() override {}
bool finished_onboarding() { return finished_; }
bool skipped_onboarding() { return skipped_; }
private:
// OnboardingDelegate:
void SkipOnboarding() override {
ASSERT_FALSE(finished_);
ASSERT_FALSE(skipped_);
skipped_ = true;
}
void FinishOnboarding() override {
ASSERT_FALSE(finished_);
ASSERT_FALSE(skipped_);
finished_ = true;
}
bool skipped_ = false;
bool finished_ = false;
};
class FakeOnboardingWebviewHost : mojom::OnboardingWebviewHost {
public:
explicit FakeOnboardingWebviewHost(
......@@ -66,8 +93,6 @@ class FakeOnboardingWebviewHost : mojom::OnboardingWebviewHost {
presentations_.clear();
}
bool exited_flow() const { return exited_flow_; }
const base::Optional<mojom::OnboardingPage>& page_loaded() {
return page_loaded_;
}
......@@ -83,26 +108,16 @@ class FakeOnboardingWebviewHost : mojom::OnboardingWebviewHost {
void LoadPage(mojom::OnboardingPagePtr page,
LoadPageCallback callback) override {
ASSERT_FALSE(exited_flow_);
page_loaded_ = *page;
std::move(callback).Run(load_page_result_.Clone());
}
void ExitFlow() override {
ASSERT_FALSE(exited_flow_);
exited_flow_ = true;
}
mojo::Binding<mojom::OnboardingWebviewHost> binding_;
mojom::OnboardingLoadPageResult load_page_result_{
net::Error::OK, kDeviceOnboardingExperimentName};
bool exited_flow_ = false;
std::vector<mojom::OnboardingPresentationPtr> presentations_;
base::Optional<mojom::OnboardingPage> page_loaded_;
......@@ -129,7 +144,9 @@ class OnboardingControllerBaseTest : public testing::Test {
base::CommandLine::ForCurrentProcess(), base::FilePath(),
/*autoupdate_enabled=*/false);
controller_impl_ = std::make_unique<OnboardingControllerImpl>(profile());
delegate_ = std::make_unique<FakeOnboardingDelegate>();
controller_impl_ =
std::make_unique<OnboardingControllerImpl>(profile(), delegate());
controller_impl_->BindRequest(mojo::MakeRequest(&controller_));
}
......@@ -213,6 +230,8 @@ class OnboardingControllerBaseTest : public testing::Test {
return identity_test_env_adaptor_->identity_test_env();
}
FakeOnboardingDelegate* delegate() { return delegate_.get(); }
FakeOnboardingWebviewHost* webview_host() { return webview_host_.get(); }
private:
......@@ -220,6 +239,7 @@ class OnboardingControllerBaseTest : public testing::Test {
std::unique_ptr<TestingProfile> profile_;
std::unique_ptr<IdentityTestEnvironmentProfileAdaptor>
identity_test_env_adaptor_;
std::unique_ptr<FakeOnboardingDelegate> delegate_;
std::unique_ptr<OnboardingControllerImpl> controller_impl_;
mojom::OnboardingControllerPtr controller_;
std::unique_ptr<FakeOnboardingWebviewHost> webview_host_;
......@@ -242,7 +262,7 @@ class OnboardingControllerFlowDisabledTest
TEST_F(OnboardingControllerFlowDisabledTest, ExitFlowWhenFlowIsDisabled) {
BindHostAndReturnLoadPageSuccess();
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerFlowDisabledTest,
......@@ -344,7 +364,8 @@ TEST_F(OnboardingControllerTest, OverridePageUrlsByCommandLine) {
TEST_F(OnboardingControllerTest, StayInFlowWhenLoadSucceeds) {
BindHostAndReturnLoadPageSuccess();
EXPECT_FALSE(webview_host()->exited_flow());
EXPECT_FALSE(delegate()->skipped_onboarding());
EXPECT_FALSE(delegate()->finished_onboarding());
}
TEST_F(OnboardingControllerTest, PresentReadyStateWhenLoadSucceeds) {
......@@ -372,7 +393,7 @@ TEST_F(OnboardingControllerTest, ExitFlowOnAuthError) {
BindHostAndSetupFailedAuth();
EXPECT_FALSE(webview_host()->page_loaded().has_value());
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerTest, PresentOnlyLoadingStateOnAuthError) {
......@@ -393,7 +414,7 @@ TEST_F(OnboardingControllerTest, SetNotEligibleForKioskNextOnAuthError) {
TEST_F(OnboardingControllerTest, ExitFlowOnLoadPageError) {
BindHostAndReturnLoadPageError();
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerTest, PresentOnlyLoadingStateOnLoadPageError) {
......@@ -414,7 +435,7 @@ TEST_F(OnboardingControllerTest, SetNotEligibleForKioskNextOnLoadPageError) {
TEST_F(OnboardingControllerTest, ExitFlowWhenHeaderValueIsMissing) {
BindHostAndReturnMissingCustomHeader();
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerTest,
......@@ -438,7 +459,7 @@ TEST_F(OnboardingControllerTest,
TEST_F(OnboardingControllerTest, ExitFlowWhenHeaderValueIsWrong) {
BindHostAndReturnWrongCustomHeader();
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerTest,
......@@ -462,13 +483,14 @@ TEST_F(OnboardingControllerTest,
TEST_F(OnboardingControllerTest, StayInFlowWhenNavigatingToDetailsPage) {
NavigateToDetailsPage();
EXPECT_FALSE(webview_host()->exited_flow());
EXPECT_FALSE(delegate()->skipped_onboarding());
EXPECT_FALSE(delegate()->finished_onboarding());
}
TEST_F(OnboardingControllerTest, DetailsPageExitsFlowOnFailedPageLoad) {
NavigateToDetailsPage(/*return_error=*/true);
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerTest, DetailsPageIsPresentedCorrectly) {
......@@ -500,13 +522,14 @@ TEST_F(OnboardingControllerTest, DetailsPageIsLoadedCorrectly) {
TEST_F(OnboardingControllerTest, StayInFlowWhenNavigatingToAllSetPage) {
NavigateToAllSetPage();
EXPECT_FALSE(webview_host()->exited_flow());
EXPECT_FALSE(delegate()->skipped_onboarding());
EXPECT_FALSE(delegate()->finished_onboarding());
}
TEST_F(OnboardingControllerTest, AllSetPageExitsFlowOnFailedPageLoad) {
NavigateToAllSetPage(/*return_error=*/true);
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->skipped_onboarding());
}
TEST_F(OnboardingControllerTest, AllSetPageIsPresentedCorrectly) {
......@@ -538,9 +561,9 @@ TEST_F(OnboardingControllerTest, AllSetPageIsLoadedCorrectly) {
TEST_F(OnboardingControllerTest, AllSetPageCanFinishFlow) {
NavigateToAllSetPage();
EXPECT_FALSE(webview_host()->exited_flow());
EXPECT_FALSE(delegate()->finished_onboarding());
HandleAction(mojom::OnboardingAction::kShowNextPage);
EXPECT_TRUE(webview_host()->exited_flow());
EXPECT_TRUE(delegate()->finished_onboarding());
}
TEST_F(OnboardingControllerTest, AllSetPageEnablesKioskNext) {
......
// Copyright 2019 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 CHROME_BROWSER_CHROMEOS_SUPERVISION_ONBOARDING_DELEGATE_H_
#define CHROME_BROWSER_CHROMEOS_SUPERVISION_ONBOARDING_DELEGATE_H_
namespace chromeos {
namespace supervision {
// Interface for classes that host the Supervision Onboarding flow. Since the
// flow might be hosted by different WebUIs, this is the common interface used
// to communicate between internal onboarding classes and their WebUI handlers.
class OnboardingDelegate {
public:
virtual ~OnboardingDelegate() {}
// Called when we want to skip the onboarding flow. This can happen if the
// user actively skipped the flow or we decided that the flow should be
// skipped for other reasons (errors, missing flags, eligibility, etc).
virtual void SkipOnboarding() = 0;
// Called when the user successfully finishes the onboarding flow by reaching
// its conclusion.
virtual void FinishOnboarding() = 0;
};
} // namespace supervision
} // namespace chromeos
#endif // CHROME_BROWSER_CHROMEOS_SUPERVISION_ONBOARDING_DELEGATE_H_
......@@ -11,6 +11,7 @@
#include "base/logging.h"
#include "base/strings/string_util.h"
#include "chrome/browser/chromeos/supervision/onboarding_constants.h"
#include "chrome/browser/chromeos/supervision/onboarding_delegate.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chromeos/constants/chromeos_switches.h"
#include "services/identity/public/cpp/primary_account_access_token_fetcher.h"
......@@ -41,8 +42,9 @@ GURL SupervisionServerBaseUrl() {
} // namespace
OnboardingFlowModel::OnboardingFlowModel(Profile* profile)
: profile_(profile) {}
OnboardingFlowModel::OnboardingFlowModel(Profile* profile,
OnboardingDelegate* delegate)
: profile_(profile), delegate_(delegate) {}
OnboardingFlowModel::~OnboardingFlowModel() = default;
......@@ -82,8 +84,13 @@ void OnboardingFlowModel::ExitFlow(ExitReason reason) {
observer.WillExitFlow(current_step_, reason);
}
webview_host_->ExitFlow();
webview_host_ = nullptr;
if (reason == ExitReason::kUserReachedEnd) {
delegate_->FinishOnboarding();
return;
}
delegate_->SkipOnboarding();
}
mojom::OnboardingWebviewHost& OnboardingFlowModel::GetWebviewHost() {
......
......@@ -22,11 +22,13 @@ class PrimaryAccountAccessTokenFetcher;
namespace chromeos {
namespace supervision {
class OnboardingDelegate;
// Class that manages the onboarding flow state, handling user actions and
// loading new pages. It notifies its observers of flow changes.
class OnboardingFlowModel {
public:
explicit OnboardingFlowModel(Profile* profile);
explicit OnboardingFlowModel(Profile* profile, OnboardingDelegate* delegate);
~OnboardingFlowModel();
// Represents each onboarding flow step.
......@@ -97,6 +99,7 @@ class OnboardingFlowModel {
void LoadPageCallback(mojom::OnboardingLoadPageResultPtr result);
Profile* profile_;
OnboardingDelegate* delegate_;
mojom::OnboardingWebviewHostPtr webview_host_;
Step current_step_ = Step::kStart;
base::ObserverList<Observer> observer_list_;
......
......@@ -180,7 +180,6 @@
this.setPresentation_.bind(this));
this.hostCallbackRouter_.loadPage.addListener(
this.webviewLoader_.loadPage.bind(this.webviewLoader_));
this.hostCallbackRouter_.exitFlow.addListener(this.exitFlow_.bind(this));
this.controller_.bindWebviewHost(this.hostCallbackRouter_.createProxy());
},
......@@ -223,11 +222,5 @@
this.controller_.handleAction(
chromeos.supervision.mojom.OnboardingAction.kShowNextPage);
},
/** @private */
exitFlow_: function() {
chrome.send(
'login.SupervisionOnboardingScreen.userActed', ['setup-finished']);
},
});
}
......@@ -20,9 +20,7 @@ constexpr StaticOobeScreenId SupervisionOnboardingScreenView::kScreenId;
SupervisionOnboardingScreenHandler::SupervisionOnboardingScreenHandler(
JSCallsContainer* js_calls_container)
: BaseScreenHandler(kScreenId, js_calls_container) {
set_user_acted_method_path("login.SupervisionOnboardingScreen.userActed");
}
: BaseScreenHandler(kScreenId, js_calls_container) {}
SupervisionOnboardingScreenHandler::~SupervisionOnboardingScreenHandler() {
if (screen_)
......@@ -48,6 +46,7 @@ void SupervisionOnboardingScreenHandler::Bind(
}
void SupervisionOnboardingScreenHandler::Unbind() {
supervision_onboarding_controller_.reset();
screen_ = nullptr;
BaseScreenHandler::SetBaseScreen(nullptr);
}
......@@ -68,10 +67,11 @@ void SupervisionOnboardingScreenHandler::Initialize() {}
void SupervisionOnboardingScreenHandler::BindSupervisionOnboardingController(
supervision::mojom::OnboardingControllerRequest request) {
DCHECK(screen_);
if (!supervision_onboarding_controller_) {
supervision_onboarding_controller_ =
std::make_unique<supervision::OnboardingControllerImpl>(
ProfileManager::GetPrimaryUserProfile());
ProfileManager::GetPrimaryUserProfile(), screen_);
}
supervision_onboarding_controller_->BindRequest(std::move(request));
......
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