Skip to content

Commit

Permalink
oobe: introduce wizard context object
Browse files Browse the repository at this point in the history
This CL introdces WizardContext object that would be used to pass
data between screens in a unified way.

Due to implementation complexities, ErrorScreen might get null context
now, it will be resolved later.

Also this CL updates mocking syntax to a new approach that does not
require to specify number of arguments.

Bug: 1102342
Change-Id: I38ed842c9450cc87d2c0d0ac0dc925525280dac4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2282928
Commit-Queue: Denis Kuznetsov [CET] <antrim@chromium.org>
Commit-Queue: Roman Sorokin [CET] <rsorokin@chromium.org>
Auto-Submit: Denis Kuznetsov [CET] <antrim@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785690}
  • Loading branch information
Denis Kuznetsov authored and Commit Bot committed Jul 7, 2020
1 parent 8ad157a commit be5be69
Show file tree
Hide file tree
Showing 51 changed files with 245 additions and 170 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/chromeos/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1802,6 +1802,8 @@ source_set("chromeos") {
"login/version_updater/version_updater.h",
"login/web_kiosk_controller.cc",
"login/web_kiosk_controller.h",
"login/wizard_context.cc",
"login/wizard_context.h",
"login/wizard_controller.cc",
"login/wizard_controller.h",
"mobile/mobile_activator.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,13 +251,13 @@ void AutoEnrollmentCheckScreen::ShowErrorScreen(
base::BindOnce(&AutoEnrollmentCheckScreen::OnErrorScreenHidden,
weak_ptr_factory_.GetWeakPtr()));
error_screen_->SetParentScreen(AutoEnrollmentCheckScreenView::kScreenId);
error_screen_->Show();
error_screen_->Show(context());
histogram_helper_->OnErrorShow(error_state);
}

void AutoEnrollmentCheckScreen::OnErrorScreenHidden() {
error_screen_->SetParentScreen(OobeScreen::SCREEN_UNKNOWN);
Show();
Show(context());
}

void AutoEnrollmentCheckScreen::SignalCompletion() {
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/chromeos/login/enrollment/enrollment_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ void EnrollmentScreen::ProcessRetry() {
++num_retries_;
LOG(WARNING) << "Enrollment retries: " << num_retries_
<< ", current auth: " << current_auth_ << ".";
Show();
Show(context());
}

void EnrollmentScreen::OnCancel() {
Expand All @@ -297,7 +297,7 @@ void EnrollmentScreen::OnCancel() {
UMA(policy::kMetricEnrollmentCancelled);

if (AdvanceToNextAuth()) {
Show();
Show(context());
return;
}

Expand Down Expand Up @@ -343,7 +343,7 @@ void EnrollmentScreen::OnEnrollmentError(policy::EnrollmentStatus status) {
current_auth_ == AUTH_ATTESTATION) {
UMA(policy::kMetricEnrollmentDeviceNotPreProvisioned);
if (AdvanceToNextAuth()) {
Show();
Show(context());
return;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h"
#include "chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h"
#include "chrome/browser/chromeos/login/enrollment/mock_enrollment_screen.h"
#include "chrome/browser/chromeos/login/wizard_context.h"
#include "chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h"
#include "chrome/browser/chromeos/policy/enrollment_config.h"
#include "chrome/browser/policy/enrollment_status.h"
Expand All @@ -35,6 +36,8 @@ class EnrollmentScreenUnitTest : public testing::Test {

// Creates the EnrollmentScreen and sets required parameters.
virtual void SetUpEnrollmentScreen() {
wizard_context_ = std::make_unique<WizardContext>();

enrollment_screen_ = std::make_unique<EnrollmentScreen>(
&mock_view_,
base::BindRepeating(&EnrollmentScreenUnitTest::HandleScreenExit,
Expand Down Expand Up @@ -63,6 +66,7 @@ class EnrollmentScreenUnitTest : public testing::Test {

protected:
std::unique_ptr<EnrollmentScreen> enrollment_screen_;
std::unique_ptr<WizardContext> wizard_context_;

// The last result reported by |enrollment_screen_|.
base::Optional<EnrollmentScreen::Result> last_screen_result_;
Expand Down Expand Up @@ -174,7 +178,7 @@ class ZeroTouchEnrollmentScreenUnitTest : public EnrollmentScreenUnitTest {
enrollment_screen_->retry_policy_.jitter_factor = 0;

// Start zero-touch enrollment.
enrollment_screen_->Show();
enrollment_screen_->Show(wizard_context_.get());

// Fast forward time by 1 minute.
FastForwardTime(base::TimeDelta::FromMinutes(1));
Expand All @@ -190,7 +194,7 @@ class ZeroTouchEnrollmentScreenUnitTest : public EnrollmentScreenUnitTest {
SetUpEnrollmentScreen();

// Start zero-touch enrollment.
enrollment_screen_->Show();
enrollment_screen_->Show(wizard_context_.get());

// Verify that enrollment flow finished and exited cleanly.
ASSERT_TRUE(last_screen_result_.has_value());
Expand All @@ -208,7 +212,7 @@ class ZeroTouchEnrollmentScreenUnitTest : public EnrollmentScreenUnitTest {
EXPECT_CALL(*GetMockScreenView(), ShowSigninScreen()).Times(1);

// Start enrollment.
enrollment_screen_->Show();
enrollment_screen_->Show(wizard_context_.get());
}

private:
Expand Down Expand Up @@ -238,7 +242,7 @@ TEST_F(ZeroTouchEnrollmentScreenUnitTest, DoNotRetryOnTopOfUser) {
enrollment_screen_->retry_policy_.jitter_factor = 0;

// Start zero-touch enrollment.
enrollment_screen_->Show();
enrollment_screen_->Show(wizard_context_.get());

// Schedule user retry button click after 30 sec.
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
Expand All @@ -261,7 +265,7 @@ TEST_F(ZeroTouchEnrollmentScreenUnitTest, DoNotRetryAfterSuccess) {
SetUpEnrollmentScreen();

// Start zero-touch enrollment.
enrollment_screen_->Show();
enrollment_screen_->Show(wizard_context_.get());

// Fast forward time by 1 minute.
FastForwardTime(base::TimeDelta::FromMinutes(1));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ class MockAutoEnrollmentCheckScreen : public AutoEnrollmentCheckScreen {
const base::RepeatingClosure& exit_callback);
~MockAutoEnrollmentCheckScreen() override;

MOCK_METHOD0(ShowImpl, void());
MOCK_METHOD0(HideImpl, void());
MOCK_METHOD(void, ShowImpl, ());
MOCK_METHOD(void, HideImpl, ());

void RealShow();
void ExitScreen();
Expand All @@ -32,8 +32,8 @@ class MockAutoEnrollmentCheckScreenView : public AutoEnrollmentCheckScreenView {

void SetDelegate(Delegate* screen) override;

MOCK_METHOD1(MockSetDelegate, void(Delegate* screen));
MOCK_METHOD0(Show, void());
MOCK_METHOD(void, MockSetDelegate, (Delegate * screen));
MOCK_METHOD(void, Show, ());

private:
Delegate* screen_ = nullptr;
Expand Down
52 changes: 28 additions & 24 deletions chrome/browser/chromeos/login/enrollment/mock_enrollment_screen.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ class MockEnrollmentScreen : public EnrollmentScreen {
const ScreenExitCallback& exit_callback);
~MockEnrollmentScreen() override;

MOCK_METHOD0(ShowImpl, void());
MOCK_METHOD0(HideImpl, void());
MOCK_METHOD(void, ShowImpl, ());
MOCK_METHOD(void, HideImpl, ());

void ExitScreen(Result result);
};
Expand All @@ -31,28 +31,32 @@ class MockEnrollmentScreenView : public EnrollmentScreenView {
MockEnrollmentScreenView();
virtual ~MockEnrollmentScreenView();

MOCK_METHOD2(SetEnrollmentConfig,
void(Controller*, const policy::EnrollmentConfig& config));
MOCK_METHOD2(SetEnterpriseDomainAndDeviceType,
void(const std::string& domain,
const base::string16& device_type));
MOCK_METHOD0(Show, void());
MOCK_METHOD0(Hide, void());
MOCK_METHOD0(ShowSigninScreen, void());
MOCK_METHOD1(ShowLicenseTypeSelectionScreen,
void(const base::DictionaryValue&));
MOCK_METHOD4(ShowActiveDirectoryScreen,
void(const std::string& domain_join_config,
const std::string& machine_name,
const std::string& username,
authpolicy::ErrorType error));
MOCK_METHOD2(ShowAttributePromptScreen,
void(const std::string& asset_id, const std::string& location));
MOCK_METHOD0(ShowEnrollmentSuccessScreen, void());
MOCK_METHOD0(ShowEnrollmentSpinnerScreen, void());
MOCK_METHOD1(ShowAuthError, void(const GoogleServiceAuthError&));
MOCK_METHOD1(ShowOtherError, void(EnterpriseEnrollmentHelper::OtherError));
MOCK_METHOD1(ShowEnrollmentStatus, void(policy::EnrollmentStatus status));
MOCK_METHOD(void,
SetEnrollmentConfig,
(Controller*, const policy::EnrollmentConfig& config));
MOCK_METHOD(void,
SetEnterpriseDomainAndDeviceType,
(const std::string& domain, const base::string16& device_type));
MOCK_METHOD(void, Show, ());
MOCK_METHOD(void, Hide, ());
MOCK_METHOD(void, ShowSigninScreen, ());
MOCK_METHOD(void,
ShowLicenseTypeSelectionScreen,
(const base::DictionaryValue&));
MOCK_METHOD(void,
ShowActiveDirectoryScreen,
(const std::string& domain_join_config,
const std::string& machine_name,
const std::string& username,
authpolicy::ErrorType error));
MOCK_METHOD(void,
ShowAttributePromptScreen,
(const std::string& asset_id, const std::string& location));
MOCK_METHOD(void, ShowEnrollmentSuccessScreen, ());
MOCK_METHOD(void, ShowEnrollmentSpinnerScreen, ());
MOCK_METHOD(void, ShowAuthError, (const GoogleServiceAuthError&));
MOCK_METHOD(void, ShowOtherError, (EnterpriseEnrollmentHelper::OtherError));
MOCK_METHOD(void, ShowEnrollmentStatus, (policy::EnrollmentStatus status));
};

} // namespace chromeos
Expand Down
12 changes: 9 additions & 3 deletions chrome/browser/chromeos/login/error_screen_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "chrome/browser/chromeos/login/test/js_checker.h"
#include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
#include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
#include "chrome/browser/chromeos/login/wizard_context.h"
#include "chrome/browser/chromeos/login/wizard_controller.h"
#include "chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h"
#include "chrome/browser/ui/webui/chromeos/login/error_screen_handler.h"
Expand Down Expand Up @@ -66,6 +67,7 @@ class NetworkErrorScreenTest : public InProcessBrowserTest {
~NetworkErrorScreenTest() override = default;

void SetUpOnMainThread() override {
wizard_context_ = std::make_unique<WizardContext>();
network_helper_ = std::make_unique<NetworkStateTestHelper>(
/*use_default_devices_and_services=*/false);
InProcessBrowserTest::SetUpOnMainThread();
Expand All @@ -80,7 +82,7 @@ class NetworkErrorScreenTest : public InProcessBrowserTest {
// the network list, picked one arbitrarily.
GetScreen()->SetUIState(NetworkError::UI_STATE_UPDATE);

GetScreen()->Show();
GetScreen()->Show(wizard_context_.get());

// Wait until network list adds the wifi test network.
test::OobeJS()
Expand Down Expand Up @@ -125,6 +127,8 @@ class NetworkErrorScreenTest : public InProcessBrowserTest {
base::RunLoop().RunUntilIdle();
}

std::unique_ptr<WizardContext> wizard_context_;

private:
std::unique_ptr<NetworkStateTestHelper> network_helper_;

Expand Down Expand Up @@ -187,7 +191,7 @@ IN_PROC_BROWSER_TEST_F(NetworkErrorScreenTest, HideCallback) {
GetScreen()->SetHideCallback(
base::BindLambdaForTesting([&]() { callback_called = true; }));

GetScreen()->Show();
GetScreen()->Show(wizard_context_.get());
OobeScreenWaiter(ErrorScreenView::kScreenId).Wait();
GetScreen()->Hide();

Expand All @@ -203,9 +207,11 @@ class GuestErrorScreenTest : public MixinBasedInProcessBrowserTest {
MixinBasedInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
SessionManagerClient::InitializeFakeInMemory();
FakeSessionManagerClient::Get()->set_supports_browser_restart(true);
wizard_context_ = std::make_unique<WizardContext>();
}

protected:
std::unique_ptr<WizardContext> wizard_context_;
LoginManagerMixin login_manager_{&mixin_host_};

private:
Expand All @@ -217,7 +223,7 @@ class GuestErrorScreenTest : public MixinBasedInProcessBrowserTest {
IN_PROC_BROWSER_TEST_F(GuestErrorScreenTest, PRE_GuestLogin) {
GetScreen()->AllowGuestSignin(true);
GetScreen()->SetUIState(NetworkError::UI_STATE_UPDATE);
GetScreen()->Show();
GetScreen()->Show(wizard_context_.get());

OobeScreenWaiter(ErrorScreenView::kScreenId).Wait();
test::OobeJS().ExpectVisiblePath({"error-guest-signin-link"});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ ArcTermsOfServiceScreen::~ArcTermsOfServiceScreen() {
}
}

bool ArcTermsOfServiceScreen::MaybeSkip() {
bool ArcTermsOfServiceScreen::MaybeSkip(WizardContext* context) {
if (!arc::IsArcTermsOfServiceOobeNegotiationNeeded()) {
exit_callback_.Run(Result::NOT_APPLICABLE);
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class ArcTermsOfServiceScreen : public BaseScreen,

protected:
// BaseScreen:
bool MaybeSkip() override;
bool MaybeSkip(WizardContext* context) override;
void ShowImpl() override;
void HideImpl() override;
void OnUserAction(const std::string& action_id) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ AssistantOptInFlowScreen::~AssistantOptInFlowScreen() {
view_->Unbind();
}

bool AssistantOptInFlowScreen::MaybeSkip() {
bool AssistantOptInFlowScreen::MaybeSkip(WizardContext* context) {
if (!g_libassistant_enabled ||
chrome_user_manager_util::IsPublicSessionOrEphemeralLogin()) {
exit_callback_.Run(Result::NOT_APPLICABLE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class AssistantOptInFlowScreen : public BaseScreen {

protected:
// BaseScreen:
bool MaybeSkip() override;
bool MaybeSkip(WizardContext* context) override;
void ShowImpl() override;
void HideImpl() override;
void OnUserAction(const std::string& action_id) override;
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/chromeos/login/screens/base_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,19 @@ BaseScreen::BaseScreen(OobeScreenId screen_id,

BaseScreen::~BaseScreen() {}

void BaseScreen::Show() {
void BaseScreen::Show(WizardContext* context) {
wizard_context_ = context;
ShowImpl();
is_hidden_ = false;
}

void BaseScreen::Hide() {
HideImpl();
is_hidden_ = true;
wizard_context_ = nullptr;
}

bool BaseScreen::MaybeSkip() {
bool BaseScreen::MaybeSkip(WizardContext* context) {
return false;
}

Expand Down
12 changes: 10 additions & 2 deletions chrome/browser/chromeos/login/screens/base_screen.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

namespace chromeos {

class WizardContext;

// Base class for the all OOBE/login/before-session screens.
// Screens are identified by ID, screen and it's JS counterpart must have same
// id.
Expand All @@ -30,14 +32,14 @@ class BaseScreen {
virtual ~BaseScreen();

// Makes wizard screen visible.
void Show();
void Show(WizardContext* context);

// Makes wizard screen invisible.
void Hide();

// Returns whether the screen should be skipped i. e. should be exited due to
// specific unmet conditions. Returns true if skips the screen.
virtual bool MaybeSkip() WARN_UNUSED_RESULT;
virtual bool MaybeSkip(WizardContext* context) WARN_UNUSED_RESULT;

// Forwards user action if screen is shown.
void HandleUserAction(const std::string& action_id);
Expand Down Expand Up @@ -72,13 +74,19 @@ class BaseScreen {
// triggering while the screen is not displayed.
base::Value* GetConfiguration() { return configuration_; }

WizardContext* context() { return wizard_context_; }

private:
bool is_hidden_ = true;

// Configuration itself is owned by WizardController and is accessible
// to screen only between OnShow / OnHide calls.
base::Value* configuration_ = nullptr;

// Wizard context itself is owned by WizardController and is accessible
// to screen only between OnShow / OnHide calls.
WizardContext* wizard_context_ = nullptr;

const OobeScreenId screen_id_;

const OobeScreenPriority screen_priority_;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/chromeos/login/screens/discover_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ DiscoverScreen::~DiscoverScreen() {
view_->Bind(nullptr);
}

bool DiscoverScreen::MaybeSkip() {
bool DiscoverScreen::MaybeSkip(WizardContext* context) {
PrefService* prefs = ProfileManager::GetActiveUserProfile()->GetPrefs();
if (chrome_user_manager_util::IsPublicSessionOrEphemeralLogin() ||
!chromeos::quick_unlock::IsPinEnabled(prefs) ||
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/chromeos/login/screens/discover_screen.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class DiscoverScreen : public BaseScreen {

protected:
// BaseScreen:
bool MaybeSkip() override;
bool MaybeSkip(WizardContext* context) override;
void ShowImpl() override;
void HideImpl() override;
void OnUserAction(const std::string& action_id) override;
Expand Down

0 comments on commit be5be69

Please sign in to comment.