Commit ace9710a authored by Brian White's avatar Brian White Committed by Commit Bot

Revert "Chrome OS Oobe: Show Ash dialog only if required"

This reverts commit 04e92329.

Reason for revert: Likely candidate for linux-chromeos-rel builder failures:

https://chromium-swarm.appspot.com/task?id=475cd90235116110
../../chrome/browser/chromeos/login/test/oobe_screen_waiter.cc:53: Failure
Expected equality of these values:
  target_screen_
    Which is: demo-preferences
  GetOobeUI()->current_screen()
    Which is: network-selection


Original change's description:
> Chrome OS Oobe: Show Ash dialog only if required
> 
> When starting a managed guest session (with no Terms of Service), a
> blank Ash dialog is shown for a brief moment before the session starts.
> This is caused by the Ash dialog for Oobe being shown even when all the
> Oobe screens are skipped.
> 
> This CL changes the behavior of the Ash dialog to only show when
> required by using a OobeUI::Observer. The dialog is not shown by
> default, and will be shown when a Oobe screen is shown.
> 
> OobeScreenWaiter now checks that the native window is visible by
> default. A small number of tests had to be changed to either show the
> native window or ignore the visibility check. The reasoning for the
> changes are in the comments for their respective tests.
> 
> The WebUI behavior is unchanged.
> 
> Bug: 1000164
> Change-Id: I9995f4beba85d7c5af7b13f81f1a654e2d52f372
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1790450
> Commit-Queue: Jit Yao Yap <jityao@google.com>
> Reviewed-by: Denis Kuznetsov <antrim@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#697539}

TBR=antrim@chromium.org,jityao@google.com

Change-Id: I4b663f335ad6ef084a2583d54519452fa4a95d19
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1000164
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808710Reviewed-by: default avatarBrian White <bcwhite@chromium.org>
Commit-Queue: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697677}
parent 4dc5fe33
......@@ -350,7 +350,7 @@ IN_PROC_BROWSER_TEST_F(OobeConfigurationEnrollmentTest, TestEnrollUsingToken) {
// Check that HID detection screen is shown if it is not specified by
// configuration.
IN_PROC_BROWSER_TEST_F(OobeConfigurationTestNoHID, TestShowHID) {
IN_PROC_BROWSER_TEST_F(OobeConfigurationTestNoHID, TestLeaveWelcomeScreen) {
LoadConfiguration();
OobeScreenWaiter(HIDDetectionView::kScreenId).Wait();
}
......
......@@ -152,10 +152,7 @@ IN_PROC_BROWSER_TEST_F(HandsOffEnrollmentTest, EnrollmentError) {
WizardController::default_controller()->AdvanceToScreen(
WelcomeView::kScreenId);
OobeScreenWaiter screen_waiter(NetworkScreenView::kScreenId);
// WebUI window is not visible until the screen animation finishes.
screen_waiter.set_no_check_native_window_visible();
screen_waiter.Wait();
OobeScreenWaiter(NetworkScreenView::kScreenId).Wait();
OobeScreenWaiter(EnrollmentScreenView::kScreenId).Wait();
......
......@@ -3,8 +3,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include <memory>
#include "ash/public/cpp/ash_switches.h"
#include "ash/public/cpp/test/shell_test_api.h"
#include "base/bind.h"
......@@ -14,8 +12,6 @@
#include "base/test/scoped_feature_list.h"
#include "base/values.h"
#include "build/branding_buildflags.h"
#include "chrome/browser/chrome_browser_main.h"
#include "chrome/browser/chrome_browser_main_extra_parts.h"
#include "chrome/browser/chromeos/arc/arc_service_launcher.h"
#include "chrome/browser/chromeos/arc/arc_session_manager.h"
#include "chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h"
......@@ -57,8 +53,6 @@
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
#include "ui/aura/window.h"
#include "ui/aura/window_observer.h"
#include "ui/display/display_switches.h"
using net::test_server::BasicHttpResponse;
......@@ -354,58 +348,6 @@ class ScopedQuickUnlockPrivateGetAuthTokenFunctionObserver
ScopedQuickUnlockPrivateGetAuthTokenFunctionObserver);
};
// Observes an |aura::Window| to see if the window was visible at some point in
// time.
class NativeWindowVisibilityObserver : public aura::WindowObserver {
public:
NativeWindowVisibilityObserver() = default;
// aura::Window will remove observers on destruction.
~NativeWindowVisibilityObserver() override = default;
void Observe(aura::Window* window) {
window_ = window;
window_->AddObserver(this);
}
void OnWindowVisibilityChanged(aura::Window* window, bool visible) override {
if (visible)
was_visible_ = visible;
}
bool was_visible() { return was_visible_; }
private:
// The window was visible at some point in time.
bool was_visible_ = false;
aura::Window* window_;
DISALLOW_COPY_AND_ASSIGN(NativeWindowVisibilityObserver);
};
// Sets the |NativeWindowVisibilityObserver| to observe the
// |LoginDisplayHost|'s |NativeWindow|. This needs to be done in
// |PostProfileInit()| as the |default_host| will not be initialized before
// this.
class NativeWindowVisibilityBrowserMainExtraParts
: public ChromeBrowserMainExtraParts {
public:
NativeWindowVisibilityBrowserMainExtraParts(
NativeWindowVisibilityObserver* observer)
: observer_(observer) {}
~NativeWindowVisibilityBrowserMainExtraParts() override = default;
// ChromeBrowserMainExtraParts:
void PostProfileInit() override {
observer_->Observe(LoginDisplayHost::default_host()->GetNativeWindow());
}
private:
NativeWindowVisibilityObserver* observer_;
DISALLOW_COPY_AND_ASSIGN(NativeWindowVisibilityBrowserMainExtraParts);
};
class OobeEndToEndTestSetupMixin : public InProcessBrowserTestMixin {
public:
struct Parameters {
......@@ -736,8 +678,7 @@ class PublicSessionOobeTest
: PublicSessionOobeTest(false /*requires_terms_of_service*/) {}
explicit PublicSessionOobeTest(bool requires_terms_of_service)
: requires_terms_of_service_(requires_terms_of_service),
observer_(std::make_unique<NativeWindowVisibilityObserver>()) {
: requires_terms_of_service_(requires_terms_of_service) {
// Prevents Chrome from starting to quit right after login display is
// finalized.
login_manager_.set_should_launch_browser(true);
......@@ -781,26 +722,11 @@ class PublicSessionOobeTest
MixinBasedInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
}
void CreatedBrowserMainParts(content::BrowserMainParts* parts) override {
MixinBasedInProcessBrowserTest::CreatedBrowserMainParts(parts);
static_cast<ChromeBrowserMainParts*>(parts)->AddParts(
new NativeWindowVisibilityBrowserMainExtraParts(observer_.get()));
}
void TearDownOnMainThread() override {
observer_.reset();
MixinBasedInProcessBrowserTest::TearDownOnMainThread();
}
bool DialogWasVisible() { return observer_->was_visible(); }
LoginManagerMixin login_manager_{&mixin_host_, {}};
private:
const bool requires_terms_of_service_;
std::unique_ptr<NativeWindowVisibilityObserver> observer_;
OobeEndToEndTestSetupMixin setup_{&mixin_host_, nullptr, GetParam()};
DeviceStateMixin device_state_{
&mixin_host_, DeviceStateMixin::State::OOBE_COMPLETED_CLOUD_ENROLLED};
......@@ -808,8 +734,6 @@ class PublicSessionOobeTest
IN_PROC_BROWSER_TEST_P(PublicSessionOobeTest, NoTermsOfService) {
login_manager_.WaitForActiveSession();
// Check that the dialog was never shown.
EXPECT_FALSE(DialogWasVisible());
}
INSTANTIATE_TEST_SUITE_P(
......
......@@ -1482,9 +1482,6 @@ void SAMLPasswordAttributesTest::SetUpOnMainThread() {
// Verifies that password attributes are extracted and stored during a
// successful log in - but only if the appropriate policy is enabled.
IN_PROC_BROWSER_TEST_P(SAMLPasswordAttributesTest, LoginSucceeded) {
// LoginDisplayHostMojo does not show Oobe dialog by default.
LoginDisplayHost::default_host()->ShowGaiaDialog(true, EmptyAccountId());
fake_saml_idp()->SetLoginHTMLTemplate("saml_login.html");
fake_saml_idp()->SetSamlResponseFile("saml_with_password_attributes.xml");
StartSamlAndWaitForIdpPageLoad(kFirstSAMLUserEmail);
......@@ -1516,9 +1513,6 @@ IN_PROC_BROWSER_TEST_P(SAMLPasswordAttributesTest, LoginSucceeded) {
// Verify that no password attributes are stored when login fails.
IN_PROC_BROWSER_TEST_P(SAMLPasswordAttributesTest, LoginFailed) {
// LoginDisplayHostMojo does not show Oobe dialog by default.
LoginDisplayHost::default_host()->ShowGaiaDialog(true, EmptyAccountId());
fake_saml_idp()->SetLoginHTMLTemplate("saml_login.html");
fake_saml_idp()->SetSamlResponseFile("saml_with_password_attributes.xml");
StartSamlAndWaitForIdpPageLoad(kFirstSAMLUserEmail);
......
......@@ -6,11 +6,7 @@
#include "base/run_loop.h"
#include "chrome/browser/chromeos/login/ui/login_display_host.h"
#include "chrome/browser/chromeos/login/ui/webui_login_view.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/aura/window.h"
#include "ui/views/controls/webview/webview.h"
#include "ui/views/view.h"
namespace chromeos {
......@@ -22,20 +18,13 @@ OobeScreenWaiter::~OobeScreenWaiter() = default;
void OobeScreenWaiter::Wait() {
DCHECK_EQ(State::IDLE, state_);
if ((!check_native_window_visible_ || IsNativeWindowVisible()) &&
IsTargetScreenReached()) {
if (GetOobeUI()->current_screen() == target_screen_) {
state_ = State::DONE;
return;
}
DCHECK(!run_loop_);
oobe_ui_observer_.Add(GetOobeUI());
if (check_native_window_visible_) {
aura::Window* native_window =
LoginDisplayHost::default_host()->GetNativeWindow();
DCHECK(native_window);
native_window_observer_.Add(native_window);
}
state_ = State::WAITING_FOR_SCREEN;
......@@ -46,8 +35,6 @@ void OobeScreenWaiter::Wait() {
DCHECK_EQ(State::DONE, state_);
oobe_ui_observer_.RemoveAll();
if (check_native_window_visible_)
native_window_observer_.RemoveAll();
if (assert_last_screen_)
EXPECT_EQ(target_screen_, GetOobeUI()->current_screen());
......@@ -73,33 +60,10 @@ void OobeScreenWaiter::OnCurrentScreenChanged(OobeScreenId current_screen,
return;
}
if (check_native_window_visible_ && !IsNativeWindowVisible()) {
return;
}
if (IsTargetScreenReached())
if (new_screen == target_screen_)
EndWait();
}
void OobeScreenWaiter::OnWindowVisibilityChanged(aura::Window* window,
bool visible) {
DCHECK_NE(state_, State::IDLE);
DCHECK(check_native_window_visible_);
if (IsNativeWindowVisible() && IsTargetScreenReached())
EndWait();
}
bool OobeScreenWaiter::IsTargetScreenReached() {
return GetOobeUI()->current_screen() == target_screen_;
}
bool OobeScreenWaiter::IsNativeWindowVisible() {
aura::Window* native_window =
LoginDisplayHost::default_host()->GetNativeWindow();
return native_window && native_window->IsVisible();
}
void OobeScreenWaiter::OnDestroyingOobeUI() {
oobe_ui_observer_.RemoveAll();
......
......@@ -10,7 +10,6 @@
#include "chrome/browser/chromeos/login/oobe_screen.h"
#include "chrome/browser/chromeos/login/test/test_condition_waiter.h"
#include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
#include "ui/aura/window_observer.h"
namespace base {
class RunLoop;
......@@ -22,17 +21,13 @@ class OobeUI;
// A waiter that blocks until the target oobe screen is reached.
class OobeScreenWaiter : public OobeUI::Observer,
public test::TestConditionWaiter,
public aura::WindowObserver {
public test::TestConditionWaiter {
public:
explicit OobeScreenWaiter(OobeScreenId target_screen);
~OobeScreenWaiter() override;
void set_no_assert_last_screen() { assert_last_screen_ = false; }
void set_assert_next_screen() { assert_next_screen_ = true; }
void set_no_check_native_window_visible() {
check_native_window_visible_ = false;
}
// OobeUI::Observer implementation:
void OnCurrentScreenChanged(OobeScreenId current_screen,
......@@ -42,21 +37,12 @@ class OobeScreenWaiter : public OobeUI::Observer,
// TestConditionWaiter;
void Wait() override;
// aura::WindowObserver:
void OnWindowVisibilityChanged(aura::Window* window, bool visible) override;
private:
enum class State { IDLE, WAITING_FOR_SCREEN, DONE };
OobeUI* GetOobeUI();
void EndWait();
// Returns true if the target screen is reached.
bool IsTargetScreenReached();
// Returns true if the native window is visible.
bool IsNativeWindowVisible();
const OobeScreenId target_screen_;
State state_ = State::IDLE;
......@@ -66,20 +52,12 @@ class OobeScreenWaiter : public OobeUI::Observer,
// This applies to the time period Wait() is running only.
bool assert_last_screen_ = true;
// If set, the waiter will assert OOBE UI does not transition to any other
// screen before transitioning to the target screen.
// If set, the watier will assert OOBE UI does not transition to any other
// screen before transisioning to the target screen.
bool assert_next_screen_ = false;
// If set, the waiter will only finish waiting if the target screen has been
// reached and the native window is visible. The assert_last_screen and
// assert_next_screen checks will only be done if the native window is
// visible. True by default.
bool check_native_window_visible_ = true;
ScopedObserver<OobeUI, OobeScreenWaiter> oobe_ui_observer_{this};
ScopedObserver<aura::Window, OobeScreenWaiter> native_window_observer_{this};
std::unique_ptr<base::RunLoop> run_loop_;
DISALLOW_COPY_AND_ASSIGN(OobeScreenWaiter);
......
......@@ -78,7 +78,6 @@ LoginDisplayHostMojo::~LoginDisplayHostMojo() {
LoginScreenClient::Get()->SetDelegate(nullptr);
if (dialog_) {
dialog_->GetOobeUI()->signin_screen_handler()->SetDelegate(nullptr);
StopObservingOobeUI();
dialog_->Close();
}
}
......@@ -86,7 +85,6 @@ LoginDisplayHostMojo::~LoginDisplayHostMojo() {
void LoginDisplayHostMojo::OnDialogDestroyed(
const OobeUIDialogDelegate* dialog) {
if (dialog == dialog_) {
StopObservingOobeUI();
dialog_ = nullptr;
wizard_controller_.reset();
}
......@@ -103,25 +101,25 @@ void LoginDisplayHostMojo::ShowPasswordChangedDialog(bool show_password_error,
DCHECK(GetOobeUI());
GetOobeUI()->signin_screen_handler()->ShowPasswordChangedDialog(
show_password_error, email);
ShowDialog();
dialog_->Show();
}
void LoginDisplayHostMojo::ShowWhitelistCheckFailedError() {
DCHECK(GetOobeUI());
GetOobeUI()->signin_screen_handler()->ShowWhitelistCheckFailedError();
ShowDialog();
dialog_->Show();
}
void LoginDisplayHostMojo::ShowErrorScreen(LoginDisplay::SigninError error_id) {
DCHECK(GetOobeUI());
GetOobeUI()->signin_screen_handler()->ShowErrorScreen(error_id);
ShowDialog();
dialog_->Show();
}
void LoginDisplayHostMojo::ShowSigninUI(const std::string& email) {
DCHECK(GetOobeUI());
GetOobeUI()->signin_screen_handler()->ShowSigninUI(email);
ShowDialog();
dialog_->Show();
}
void LoginDisplayHostMojo::HandleDisplayCaptivePortal() {
......@@ -176,15 +174,12 @@ void LoginDisplayHostMojo::SetStatusAreaVisible(bool visible) {
}
void LoginDisplayHostMojo::StartWizard(OobeScreenId first_screen) {
OobeUI* oobe_ui = GetOobeUI();
DCHECK(oobe_ui);
// Dialog is not shown immediately, and will be shown only when a screen
// change occurs. This prevents the dialog from showing when there are no
// screens to show.
ObserveOobeUI();
DCHECK(GetOobeUI());
wizard_controller_ = std::make_unique<WizardController>();
wizard_controller_->Init(first_screen);
dialog_->Show();
}
WizardController* LoginDisplayHostMojo::GetWizardController() {
......@@ -246,11 +241,11 @@ void LoginDisplayHostMojo::OnPreferencesChanged() {
}
void LoginDisplayHostMojo::OnStartAppLaunch() {
ShowFullScreen();
dialog_->ShowFullScreen();
}
void LoginDisplayHostMojo::OnStartArcKiosk() {
ShowFullScreen();
dialog_->ShowFullScreen();
}
void LoginDisplayHostMojo::OnBrowserCreated() {
......@@ -269,7 +264,7 @@ void LoginDisplayHostMojo::ShowGaiaDialog(bool can_close,
ShowGaiaDialogCommon(prefilled_account);
ShowDialog();
dialog_->Show();
}
void LoginDisplayHostMojo::HideOobeDialog() {
......@@ -285,7 +280,7 @@ void LoginDisplayHostMojo::HideOobeDialog() {
}
LoadWallpaper(focused_pod_account_id_);
HideDialog();
dialog_->Hide();
}
void LoginDisplayHostMojo::UpdateOobeDialogSize(int width, int height) {
......@@ -456,17 +451,6 @@ void LoginDisplayHostMojo::OnOldEncryptionDetected(
const UserContext& user_context,
bool has_incomplete_migration) {}
void LoginDisplayHostMojo::OnCurrentScreenChanged(OobeScreenId current_screen,
OobeScreenId new_screen) {
DCHECK(dialog_);
if (!dialog_->IsVisible())
ShowDialog();
}
void LoginDisplayHostMojo::OnDestroyingOobeUI() {
StopObservingOobeUI();
}
void LoginDisplayHostMojo::LoadOobeDialog() {
if (dialog_)
return;
......@@ -500,45 +484,4 @@ void LoginDisplayHostMojo::OnChallengeResponseKeysPrepared(
existing_user_controller_->Login(user_context, chromeos::SigninSpecifics());
}
void LoginDisplayHostMojo::ShowDialog() {
ObserveOobeUI();
dialog_->Show();
}
void LoginDisplayHostMojo::ShowFullScreen() {
ObserveOobeUI();
dialog_->ShowFullScreen();
}
void LoginDisplayHostMojo::HideDialog() {
// Stop observing so that dialog will not be shown when a screen change
// occurs. Screen changes can occur even when the dialog is not shown (e.g.
// with hidden error screens).
StopObservingOobeUI();
dialog_->Hide();
}
void LoginDisplayHostMojo::ObserveOobeUI() {
if (added_as_oobe_observer_)
return;
OobeUI* oobe_ui = GetOobeUI();
if (!oobe_ui)
return;
oobe_ui->AddObserver(this);
added_as_oobe_observer_ = true;
}
void LoginDisplayHostMojo::StopObservingOobeUI() {
if (!added_as_oobe_observer_)
return;
added_as_oobe_observer_ = false;
OobeUI* oobe_ui = GetOobeUI();
if (oobe_ui)
oobe_ui->RemoveObserver(this);
}
} // namespace chromeos
......@@ -18,7 +18,6 @@
#include "chrome/browser/chromeos/login/ui/login_display_host_common.h"
#include "chrome/browser/chromeos/login/ui/oobe_ui_dialog_delegate.h"
#include "chrome/browser/ui/ash/login_screen_client.h"
#include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
#include "chromeos/login/auth/auth_status_consumer.h"
#include "chromeos/login/auth/challenge_response_key.h"
......@@ -35,8 +34,7 @@ class MojoSystemInfoDispatcher;
// screen.
class LoginDisplayHostMojo : public LoginDisplayHostCommon,
public LoginScreenClient::Delegate,
public AuthStatusConsumer,
public OobeUI::Observer {
public AuthStatusConsumer {
public:
LoginDisplayHostMojo();
~LoginDisplayHostMojo() override;
......@@ -128,11 +126,6 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
void OnOldEncryptionDetected(const UserContext& user_context,
bool has_incomplete_migration) override;
// OobeUI::Observer:
void OnCurrentScreenChanged(OobeScreenId current_screen,
OobeScreenId new_screen) override;
void OnDestroyingOobeUI() override;
private:
void LoadOobeDialog();
......@@ -144,17 +137,6 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
base::OnceCallback<void(bool)> on_auth_complete_callback,
std::vector<ChallengeResponseKey> challenge_response_keys);
// Helper methods to show and hide the dialog.
void ShowDialog();
void ShowFullScreen();
void HideDialog();
// Adds this as a |OobeUI::Observer| if it has not already been added as one.
void ObserveOobeUI();
// Removes this as a |OobeUI::Observer| if it has been added as an observer.
void StopObservingOobeUI();
// State associated with a pending authentication attempt.
struct AuthState {
AuthState(AccountId account_id, base::OnceCallback<void(bool)> callback);
......@@ -199,9 +181,6 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
SecurityTokenPinDialogHostAshImpl security_token_pin_dialog_host_ash_impl_;
// Set if this has been added as a |OobeUI::Observer|.
bool added_as_oobe_observer_ = false;
base::WeakPtrFactory<LoginDisplayHostMojo> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(LoginDisplayHostMojo);
......
{
"welcomeNext": true
}
"welcomeNext": true,
}
\ No newline at end of file
{
"welcomeNext" : false,
"desc" : "Skip welcomeNext as this will trigger an action on the hidden welcome screen"
}
\ No newline at end of file
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