Skip to content

Commit

Permalink
cros: Use ScreenManager as a component instead of deriving WizardCont…
Browse files Browse the repository at this point in the history
…roller from it

This makes it easier/cleaner to implement CoreOobeView::Delegate on ScreenManager in a follow-up CL.

BUG=672142

Review-Url: https://codereview.chromium.org/2739073002
Cr-Commit-Position: refs/heads/master@{#456228}
  • Loading branch information
jdufault authored and Commit bot committed Mar 11, 2017
1 parent 2c657c6 commit 3ceda53
Show file tree
Hide file tree
Showing 12 changed files with 96 additions and 70 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ class EnrollmentScreenTest : public WizardInProcessBrowserTest {
IN_PROC_BROWSER_TEST_F(EnrollmentScreenTest, TestCancel) {
ASSERT_TRUE(WizardController::default_controller());

EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(WizardController::default_controller());
EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(enrollment_screen);

base::RunLoop run_loop;
Expand All @@ -69,8 +69,8 @@ IN_PROC_BROWSER_TEST_F(EnrollmentScreenTest, DISABLED_TestSuccess) {
ASSERT_TRUE(WizardController::default_controller());
EXPECT_FALSE(StartupUtils::IsOobeCompleted());

EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(WizardController::default_controller());
EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(enrollment_screen);

base::RunLoop run_loop;
Expand Down Expand Up @@ -105,8 +105,8 @@ class AttestationAuthEnrollmentScreenTest : public EnrollmentScreenTest {
IN_PROC_BROWSER_TEST_F(AttestationAuthEnrollmentScreenTest, TestCancel) {
ASSERT_TRUE(WizardController::default_controller());

EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(WizardController::default_controller());
EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(enrollment_screen);

base::RunLoop run_loop;
Expand All @@ -133,7 +133,8 @@ IN_PROC_BROWSER_TEST_F(EnrollmentScreenTest, EnrollmentSpinner) {
WizardController* wcontroller = WizardController::default_controller();
ASSERT_TRUE(wcontroller);

EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(wcontroller);
EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(wcontroller->screen_manager());
ASSERT_TRUE(enrollment_screen);

EnrollmentScreenView* view = enrollment_screen->GetView();
Expand Down Expand Up @@ -177,8 +178,8 @@ class ForcedAttestationAuthEnrollmentScreenTest : public EnrollmentScreenTest {
IN_PROC_BROWSER_TEST_F(ForcedAttestationAuthEnrollmentScreenTest, TestCancel) {
ASSERT_TRUE(WizardController::default_controller());

EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(WizardController::default_controller());
EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(enrollment_screen);

base::RunLoop run_loop;
Expand Down Expand Up @@ -224,8 +225,8 @@ class MultiAuthEnrollmentScreenTest : public EnrollmentScreenTest {
IN_PROC_BROWSER_TEST_F(MultiAuthEnrollmentScreenTest, TestCancel) {
ASSERT_TRUE(WizardController::default_controller());

EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(WizardController::default_controller());
EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(enrollment_screen);

base::RunLoop run_loop;
Expand Down Expand Up @@ -269,8 +270,8 @@ class ProvisionedEnrollmentScreenTest : public EnrollmentScreenTest {
IN_PROC_BROWSER_TEST_F(ProvisionedEnrollmentScreenTest, TestBackButton) {
ASSERT_TRUE(WizardController::default_controller());

EnrollmentScreen* enrollment_screen =
EnrollmentScreen::Get(WizardController::default_controller());
EnrollmentScreen* enrollment_screen = EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(enrollment_screen);

base::RunLoop run_loop;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ class EnterpriseEnrollmentTest : public LoginManagerTest {

// Helper method to return the current EnrollmentScreen instance.
EnrollmentScreen* enrollment_screen() {
return EnrollmentScreen::Get(WizardController::default_controller());
return EnrollmentScreen::Get(
WizardController::default_controller()->screen_manager());
}

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ class TimedRunLoop {
class LanguageListWaiter : public NetworkScreen::Observer {
public:
LanguageListWaiter()
: network_screen_(
NetworkScreen::Get(WizardController::default_controller())),
: network_screen_(NetworkScreen::Get(
WizardController::default_controller()->screen_manager())),
loop_(base::TimeDelta::FromSeconds(kTimeoutSeconds), "LanguageList") {
network_screen_->AddObserver(this);
CheckLanguageList();
Expand Down
14 changes: 8 additions & 6 deletions chrome/browser/chromeos/login/screen_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,24 @@

#include "chrome/browser/chromeos/login/screen_manager.h"

#include "base/memory/ptr_util.h"
#include "chrome/browser/chromeos/login/wizard_controller.h"

namespace chromeos {

ScreenManager::ScreenManager() {
}
ScreenManager::ScreenManager(WizardController* wizard_controller)
: wizard_controller_(wizard_controller) {}

ScreenManager::~ScreenManager() {
}
ScreenManager::~ScreenManager() {}

BaseScreen* ScreenManager::GetScreen(OobeScreen screen) {
auto iter = screens_.find(screen);
if (iter != screens_.end())
return iter->second.get();

BaseScreen* result = CreateScreen(screen);
BaseScreen* result = wizard_controller_->CreateScreen(screen);
DCHECK(result) << "Can not create screen named " << GetOobeScreenName(screen);
screens_[screen] = make_linked_ptr(result);
screens_[screen] = base::WrapUnique(result);
return result;
}

Expand Down
21 changes: 12 additions & 9 deletions chrome/browser/chromeos/login/screen_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,26 @@
#define CHROME_BROWSER_CHROMEOS_LOGIN_SCREEN_MANAGER_H_

#include <map>
#include <memory>
#include <string>

#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/linked_ptr.h"
#include "chrome/browser/chromeos/login/screens/base_screen.h"

namespace chromeos {

class WizardController;

// Class that manages creation and ownership of screens.
class ScreenManager {
public:
ScreenManager();
virtual ~ScreenManager();
// |wizard_controller| is not owned by this class.
explicit ScreenManager(WizardController* wizard_controller);
~ScreenManager();

// Getter for screen with lazy initialization.
virtual BaseScreen* GetScreen(OobeScreen screen);

// Factory for screen instances.
virtual BaseScreen* CreateScreen(OobeScreen screen) = 0;
BaseScreen* GetScreen(OobeScreen screen);

bool HasScreen(OobeScreen screen);

Expand All @@ -37,8 +37,11 @@ class ScreenManager {
friend class WizardInProcessBrowserTest;
friend class WizardControllerBrokenLocalStateTest;

// Screens.
std::map<OobeScreen, linked_ptr<BaseScreen>> screens_;
// Created screens.
std::map<OobeScreen, std::unique_ptr<BaseScreen>> screens_;

// Used to allocate BaseScreen instances. Unowned.
WizardController* wizard_controller_;

DISALLOW_COPY_AND_ASSIGN(ScreenManager);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ class NetworkScreenTest : public WizardInProcessBrowserTest {
WizardInProcessBrowserTest::SetUpOnMainThread();
mock_base_screen_delegate_.reset(new MockBaseScreenDelegate());
ASSERT_TRUE(WizardController::default_controller() != nullptr);
network_screen_ =
NetworkScreen::Get(WizardController::default_controller());
network_screen_ = NetworkScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(network_screen_ != nullptr);
ASSERT_EQ(WizardController::default_controller()->current_screen(),
network_screen_);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ class UpdateScreenTest : public WizardInProcessBrowserTest {

WizardInProcessBrowserTest::SetUpOnMainThread();

ASSERT_TRUE(WizardController::default_controller() != NULL);
update_screen_ = UpdateScreen::Get(WizardController::default_controller());
ASSERT_TRUE(update_screen_ != NULL);
ASSERT_TRUE(WizardController::default_controller() != nullptr);
update_screen_ = UpdateScreen::Get(
WizardController::default_controller()->screen_manager());
ASSERT_TRUE(update_screen_ != nullptr);
ASSERT_EQ(WizardController::default_controller()->current_screen(),
update_screen_);
update_screen_->base_screen_delegate_ = mock_base_screen_delegate_.get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ namespace {
SupervisedUserCreationScreen* GetScreen(LoginDisplayHost* host) {
DCHECK(host);
DCHECK(host->GetWizardController());
SupervisedUserCreationScreen* result =
SupervisedUserCreationScreen::Get(host->GetWizardController());
SupervisedUserCreationScreen* result = SupervisedUserCreationScreen::Get(
host->GetWizardController()->screen_manager());
DCHECK(result);
return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ bool UserImageSyncObserver::GetSyncedImageIndex(int* index) {
bool UserImageSyncObserver::CanUpdateLocalImageNow() {
if (WizardController* wizard_controller =
WizardController::default_controller()) {
UserImageScreen* screen = UserImageScreen::Get(wizard_controller);
UserImageScreen* screen =
UserImageScreen::Get(wizard_controller->screen_manager());
if (wizard_controller->current_screen() == screen) {
if (screen->user_selected_image())
return false;
Expand Down
25 changes: 15 additions & 10 deletions chrome/browser/chromeos/login/wizard_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,10 @@ bool WizardController::zero_delay_enabled_ = false;
PrefService* WizardController::local_state_for_testing_ = nullptr;

WizardController::WizardController(LoginDisplayHost* host, OobeUI* oobe_ui)
: host_(host), oobe_ui_(oobe_ui), weak_factory_(this) {
: screen_manager_(this),
host_(host),
oobe_ui_(oobe_ui),
weak_factory_(this) {
DCHECK(default_controller_ == nullptr);
default_controller_ = this;
if (!ash_util::IsRunningInMash()) {
Expand Down Expand Up @@ -358,7 +361,7 @@ ErrorScreen* WizardController::GetErrorScreen() {
BaseScreen* WizardController::GetScreen(OobeScreen screen) {
if (screen == OobeScreen::SCREEN_ERROR_MESSAGE)
return GetErrorScreen();
return ScreenManager::GetScreen(screen);
return screen_manager_.GetScreen(screen);
}

BaseScreen* WizardController::CreateScreen(OobeScreen screen) {
Expand Down Expand Up @@ -430,7 +433,8 @@ void WizardController::ShowNetworkScreen() {
VLOG(1) << "Showing network screen.";
// Hide the status area initially; it only appears after OOBE first animates
// in. Keep it visible if the user goes back to the existing network screen.
SetStatusAreaVisible(HasScreen(OobeScreen::SCREEN_OOBE_NETWORK));
SetStatusAreaVisible(
screen_manager_.HasScreen(OobeScreen::SCREEN_OOBE_NETWORK));
SetCurrentScreen(GetScreen(OobeScreen::SCREEN_OOBE_NETWORK));

// There are two possible screens where we listen to the incoming Bluetooth
Expand Down Expand Up @@ -582,7 +586,8 @@ void WizardController::ShowWrongHWIDScreen() {
void WizardController::ShowAutoEnrollmentCheckScreen() {
VLOG(1) << "Showing Auto-enrollment check screen.";
SetStatusAreaVisible(true);
AutoEnrollmentCheckScreen* screen = AutoEnrollmentCheckScreen::Get(this);
AutoEnrollmentCheckScreen* screen =
AutoEnrollmentCheckScreen::Get(screen_manager());
if (retry_auto_enrollment_check_)
screen->ClearState();
screen->set_auto_enrollment_controller(host_->GetAutoEnrollmentController());
Expand Down Expand Up @@ -893,7 +898,7 @@ void WizardController::InitiateOOBEUpdate() {
void WizardController::StartOOBEUpdate() {
VLOG(1) << "StartOOBEUpdate";
SetCurrentScreenSmooth(GetScreen(OobeScreen::SCREEN_OOBE_UPDATE), true);
UpdateScreen::Get(this)->StartNetworkCheck();
UpdateScreen::Get(screen_manager())->StartNetworkCheck();
}

void WizardController::StartTimezoneResolve() {
Expand Down Expand Up @@ -1195,7 +1200,7 @@ bool WizardController::GetUsageStatisticsReporting() const {
void WizardController::SetHostNetwork() {
if (!shark_controller_)
return;
NetworkScreen* network_screen = NetworkScreen::Get(this);
NetworkScreen* network_screen = NetworkScreen::Get(screen_manager());
std::string onc_spec;
network_screen->GetConnectedWifiNetwork(&onc_spec);
if (!onc_spec.empty())
Expand All @@ -1205,7 +1210,7 @@ void WizardController::SetHostNetwork() {
void WizardController::SetHostConfiguration() {
if (!shark_controller_)
return;
NetworkScreen* network_screen = NetworkScreen::Get(this);
NetworkScreen* network_screen = NetworkScreen::Get(screen_manager());
shark_controller_->SetHostConfiguration(
true, // Eula must be accepted before we get this far.
network_screen->GetApplicationLocale(), network_screen->GetTimezone(),
Expand All @@ -1224,7 +1229,7 @@ void WizardController::ConfigureHostRequested(
StartupUtils::MarkEulaAccepted();
SetUsageStatisticsReporting(send_reports);

NetworkScreen* network_screen = NetworkScreen::Get(this);
NetworkScreen* network_screen = NetworkScreen::Get(screen_manager());
network_screen->SetApplicationLocaleAndInputMethod(lang, keyboard_layout);
network_screen->SetTimezone(timezone);

Expand All @@ -1241,7 +1246,7 @@ void WizardController::AddNetworkRequested(const std::string& onc_spec) {
remora_controller_->OnNetworkConnectivityChanged(
pairing_chromeos::HostPairingController::CONNECTIVITY_CONNECTING);

NetworkScreen* network_screen = NetworkScreen::Get(this);
NetworkScreen* network_screen = NetworkScreen::Get(screen_manager());
const chromeos::NetworkState* network_state = chromeos::NetworkHandler::Get()
->network_state_handler()
->DefaultNetwork();
Expand Down Expand Up @@ -1506,7 +1511,7 @@ void WizardController::StartEnrollmentScreen(bool force_interactive) {
: policy::EnrollmentConfig::MODE_MANUAL_REENROLLMENT;
}

EnrollmentScreen* screen = EnrollmentScreen::Get(this);
EnrollmentScreen* screen = EnrollmentScreen::Get(screen_manager());
screen->SetParameters(effective_config, shark_controller_.get());
SetStatusAreaVisible(true);
SetCurrentScreen(screen);
Expand Down
15 changes: 11 additions & 4 deletions chrome/browser/chromeos/login/wizard_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ struct TimeZoneResponseData;
// Class that manages control flow between wizard screens. Wizard controller
// interacts with screen controllers to move the user between screens.
class WizardController : public BaseScreenDelegate,
public ScreenManager,
public EulaScreen::Delegate,
public ControllerPairingScreen::Delegate,
public HostPairingScreen::Delegate,
Expand Down Expand Up @@ -113,13 +112,19 @@ class WizardController : public BaseScreenDelegate,
// Returns true if the current wizard instance has reached the login screen.
bool login_screen_started() const { return login_screen_started_; }

// ScreenManager implementation.
BaseScreen* GetScreen(OobeScreen screen) override;
BaseScreen* CreateScreen(OobeScreen screen) override;
// Returns a given screen. Creates it lazily.
BaseScreen* GetScreen(OobeScreen screen);

// Returns the current ScreenManager instance.
ScreenManager* screen_manager() { return &screen_manager_; }

// Volume percent at which spoken feedback is still audible.
static const int kMinAudibleOutputVolumePercent;

// Allocate a given BaseScreen for the given |Screen|. Used by
// |screen_manager_|.
BaseScreen* CreateScreen(OobeScreen screen);

private:
// Show specific screen.
void ShowNetworkScreen();
Expand Down Expand Up @@ -300,6 +305,8 @@ class WizardController : public BaseScreenDelegate,
// attestation-based enrollment if appropriate.
void StartEnrollmentScreen(bool force_interactive);

ScreenManager screen_manager_;

// Whether to skip any screens that may normally be shown after login
// (registration, Terms of Service, user image selection).
static bool skip_post_login_screens_;
Expand Down
Loading

0 comments on commit 3ceda53

Please sign in to comment.