Skip to content

Commit

Permalink
ash: Promote AuthDialogDebugView to be default view
Browse files Browse the repository at this point in the history 
AuthDialogDebugView was used in in-session auth dialog in the early
development stage. Now that it has the correct views and is close to
the final contents view, rename it to AuthDialogContentsView. This
change also removes the dev flag "show-auth-dialog-dev-overlay" so that:

1. developers (especially Chrome's WebAuthn team) can test the flow end to
   end without having to enter dev mode on ChromeOS.
2. UX reviewers can evaluate the user flow.

Note that the feature is still behind a feature flag "base::Feature
kWebAuthCrosPlatformAuthenticator". Developers can toggled that via
chrome://flags.

No functionality change.

Bug: b:156258540
Change-Id: I7c8e0a94993cc6d483d5280cd91fa970846930f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2355000
Commit-Queue: Yicheng Li <yichengli@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797959}
  • Loading branch information
Yicheng Li authored and Commit Bot committed Aug 14, 2020
1 parent 27cd664 commit 9969572
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 59 deletions.
4 changes: 2 additions & 2 deletions ash/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,8 @@ component("ash") {
"ime/ime_switch_type.h",
"ime/mode_indicator_observer.cc",
"ime/mode_indicator_observer.h",
"in_session_auth/auth_dialog_debug_view.cc",
"in_session_auth/auth_dialog_debug_view.h",
"in_session_auth/auth_dialog_contents_view.cc",
"in_session_auth/auth_dialog_contents_view.h",
"in_session_auth/in_session_auth_dialog.cc",
"in_session_auth/in_session_auth_dialog.h",
"in_session_auth/in_session_auth_dialog_controller_impl.cc",
Expand Down
50 changes: 26 additions & 24 deletions ...in_session_auth/auth_dialog_debug_view.cc → ...session_auth/auth_dialog_contents_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "ash/in_session_auth/auth_dialog_debug_view.h"
#include "ash/in_session_auth/auth_dialog_contents_view.h"

#include <memory>
#include <utility>
Expand Down Expand Up @@ -32,6 +32,7 @@ enum class ButtonId {
kCancel,
};

// TODO(b/164195709): Move these strings to a grd file.
const char kTitle[] = "Verify it's you";
const char kFingerprintPrompt[] = "Authenticate with fingerprint";
// If fingerprint option is available, password input field will be hidden
Expand All @@ -57,7 +58,7 @@ constexpr int kFingerprintViewWidthDp = 204;
} // namespace

// Consists of fingerprint icon view and a label.
class AuthDialogDebugView::FingerprintView : public views::View {
class AuthDialogContentsView::FingerprintView : public views::View {
public:
FingerprintView() {
SetBorder(views::CreateEmptyBorder(kFingerprintIconTopSpacingDp, 0, 0, 0));
Expand Down Expand Up @@ -99,7 +100,7 @@ class AuthDialogDebugView::FingerprintView : public views::View {
AnimatedRoundedImageView* icon_ = nullptr;
};

AuthDialogDebugView::AuthDialogDebugView(uint32_t auth_methods)
AuthDialogContentsView::AuthDialogContentsView(uint32_t auth_methods)
: auth_methods_(auth_methods) {
DCHECK(auth_methods_ & kAuthPassword);

Expand Down Expand Up @@ -135,9 +136,9 @@ AuthDialogDebugView::AuthDialogDebugView(uint32_t auth_methods)
InitPasswordView();
}

AuthDialogDebugView::~AuthDialogDebugView() = default;
AuthDialogContentsView::~AuthDialogContentsView() = default;

void AuthDialogDebugView::AddTitleView() {
void AuthDialogContentsView::AddTitleView() {
title_ = container_->AddChildView(std::make_unique<views::Label>());
title_->SetEnabledColor(SK_ColorBLACK);
title_->SetSubpixelRenderingEnabled(false);
Expand All @@ -152,7 +153,7 @@ void AuthDialogDebugView::AddTitleView() {
title_->SetElideBehavior(gfx::ElideBehavior::ELIDE_TAIL);
}

void AuthDialogDebugView::AddPromptView() {
void AuthDialogContentsView::AddPromptView() {
prompt_ = container_->AddChildView(std::make_unique<views::Label>());
prompt_->SetEnabledColor(SK_ColorBLACK);
prompt_->SetSubpixelRenderingEnabled(false);
Expand All @@ -170,7 +171,7 @@ void AuthDialogDebugView::AddPromptView() {
prompt_->SetElideBehavior(gfx::ElideBehavior::ELIDE_TAIL);
}

void AuthDialogDebugView::AddPasswordView() {
void AuthDialogContentsView::AddPasswordView() {
password_view_ = container_->AddChildView(
std::make_unique<LoginPasswordView>(CreateInSessionAuthPalette()));

Expand All @@ -189,7 +190,7 @@ void AuthDialogDebugView::AddPasswordView() {
: l10n_util::GetStringUTF16(IDS_ASH_LOGIN_POD_PASSWORD_PLACEHOLDER));
}

void AuthDialogDebugView::AddPinView() {
void AuthDialogContentsView::AddPinView() {
pin_view_ = container_->AddChildView(std::make_unique<LoginPinView>(
LoginPinView::Style::kAlphanumeric, CreateInSessionAuthPalette(),
base::BindRepeating(&LoginPasswordView::InsertNumber,
Expand All @@ -201,21 +202,22 @@ void AuthDialogDebugView::AddPinView() {
pin_view_->SetVisible(auth_methods_ & kAuthPin);
}

void AuthDialogDebugView::InitPasswordView() {
password_view_->Init(base::BindRepeating(&AuthDialogDebugView::OnAuthSubmit,
base::Unretained(this)),
base::BindRepeating(&LoginPinView::OnPasswordTextChanged,
base::Unretained(pin_view_)),
base::DoNothing(), base::DoNothing());
void AuthDialogContentsView::InitPasswordView() {
password_view_->Init(
base::BindRepeating(&AuthDialogContentsView::OnAuthSubmit,
base::Unretained(this)),
base::BindRepeating(&LoginPinView::OnPasswordTextChanged,
base::Unretained(pin_view_)),
base::DoNothing(), base::DoNothing());
}

void AuthDialogDebugView::AddVerticalSpacing(int height) {
void AuthDialogContentsView::AddVerticalSpacing(int height) {
auto* spacing =
container_->AddChildView(std::make_unique<NonAccessibleView>());
spacing->SetPreferredSize(gfx::Size(kContainerPreferredWidth, height));
}

void AuthDialogDebugView::AddActionButtonsView() {
void AuthDialogContentsView::AddActionButtonsView() {
action_view_container_ =
container_->AddChildView(std::make_unique<NonAccessibleView>());
auto* buttons_layout = action_view_container_->SetLayoutManager(
Expand All @@ -231,8 +233,8 @@ void AuthDialogDebugView::AddActionButtonsView() {
action_view_container_);
}

void AuthDialogDebugView::ButtonPressed(views::Button* sender,
const ui::Event& event) {
void AuthDialogContentsView::ButtonPressed(views::Button* sender,
const ui::Event& event) {
if (sender == cancel_button_) {
// Cancel() deletes |this|.
InSessionAuthDialogController::Get()->Cancel();
Expand All @@ -242,9 +244,9 @@ void AuthDialogDebugView::ButtonPressed(views::Button* sender,
// view and password input view.
}

views::LabelButton* AuthDialogDebugView::AddButton(const std::string& text,
int id,
views::View* container) {
views::LabelButton* AuthDialogContentsView::AddButton(const std::string& text,
int id,
views::View* container) {
// Creates a button with |text|.
auto button =
std::make_unique<views::MdTextButton>(this, base::ASCIIToUTF16(text));
Expand All @@ -256,14 +258,14 @@ views::LabelButton* AuthDialogDebugView::AddButton(const std::string& text,
return view;
}

void AuthDialogDebugView::OnAuthSubmit(const base::string16& password) {
void AuthDialogContentsView::OnAuthSubmit(const base::string16& password) {
InSessionAuthDialogController::Get()->AuthenticateUserWithPasswordOrPin(
base::UTF16ToUTF8(password),
base::BindOnce(&AuthDialogDebugView::OnAuthComplete,
base::BindOnce(&AuthDialogContentsView::OnAuthComplete,
weak_factory_.GetWeakPtr()));
}

// TODO(b/156258540): Clear password/PIN if auth failed and retry is allowed.
void AuthDialogDebugView::OnAuthComplete(base::Optional<bool> success) {}
void AuthDialogContentsView::OnAuthComplete(base::Optional<bool> success) {}

} // namespace ash
19 changes: 10 additions & 9 deletions ash/in_session_auth/auth_dialog_debug_view.h → ..._session_auth/auth_dialog_contents_view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef ASH_IN_SESSION_AUTH_AUTH_DIALOG_DEBUG_VIEW_H_
#define ASH_IN_SESSION_AUTH_AUTH_DIALOG_DEBUG_VIEW_H_
#ifndef ASH_IN_SESSION_AUTH_AUTH_DIALOG_CONTENTS_VIEW_H_
#define ASH_IN_SESSION_AUTH_AUTH_DIALOG_CONTENTS_VIEW_H_

#include <string>

Expand All @@ -23,7 +23,8 @@ class LoginPinView;

// Contains the debug views that allows the developer to interact with the
// AuthDialogController.
class AuthDialogDebugView : public views::View, public views::ButtonListener {
class AuthDialogContentsView : public views::View,
public views::ButtonListener {
public:
// Flags which describe the set of currently visible auth methods.
enum AuthMethods {
Expand All @@ -33,10 +34,10 @@ class AuthDialogDebugView : public views::View, public views::ButtonListener {
kAuthFingerprint = 1 << 2, // Use fingerprint to unlock.
};

explicit AuthDialogDebugView(uint32_t auth_methods);
AuthDialogDebugView(const AuthDialogDebugView&) = delete;
AuthDialogDebugView& operator=(const AuthDialogDebugView&) = delete;
~AuthDialogDebugView() override;
explicit AuthDialogContentsView(uint32_t auth_methods);
AuthDialogContentsView(const AuthDialogContentsView&) = delete;
AuthDialogContentsView& operator=(const AuthDialogContentsView&) = delete;
~AuthDialogContentsView() override;

// views::ButtonListener:
void ButtonPressed(views::Button* sender, const ui::Event& event) override;
Expand Down Expand Up @@ -108,9 +109,9 @@ class AuthDialogDebugView : public views::View, public views::ButtonListener {
// Container which holds action buttons.
views::View* action_view_container_ = nullptr;

base::WeakPtrFactory<AuthDialogDebugView> weak_factory_{this};
base::WeakPtrFactory<AuthDialogContentsView> weak_factory_{this};
};

} // namespace ash

#endif // ASH_IN_SESSION_AUTH_AUTH_DIALOG_DEBUG_VIEW_H_
#endif // ASH_IN_SESSION_AUTH_AUTH_DIALOG_CONTENTS_VIEW_H_
22 changes: 9 additions & 13 deletions ash/in_session_auth/in_session_auth_dialog.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@

#include "ash/in_session_auth/in_session_auth_dialog.h"

#include "ash/in_session_auth/auth_dialog_debug_view.h"
#include "ash/in_session_auth/auth_dialog_contents_view.h"
#include "base/command_line.h"
#include "chromeos/constants/chromeos_switches.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
#include "ui/views/widget/widget.h"
Expand Down Expand Up @@ -57,18 +56,15 @@ std::unique_ptr<views::Widget> CreateAuthDialogWidget(aura::Window* parent) {
} // namespace

InSessionAuthDialog::InSessionAuthDialog(uint32_t auth_methods) {
if (base::CommandLine::ForCurrentProcess()->HasSwitch(
chromeos::switches::kShowAuthDialogDevOverlay)) {
widget_ = CreateAuthDialogWidget(nullptr);
auto* contents_view = widget_->SetContentsView(
std::make_unique<AuthDialogDebugView>(auth_methods));
gfx::Rect bound = widget_->GetWindowBoundsInScreen();
// Calculate initial height based on which child views are shown.
bound.set_height(contents_view->GetPreferredSize().height());
widget_->SetBounds(bound);
widget_ = CreateAuthDialogWidget(nullptr);
auto* contents_view = widget_->SetContentsView(
std::make_unique<AuthDialogContentsView>(auth_methods));
gfx::Rect bound = widget_->GetWindowBoundsInScreen();
// Calculate initial height based on which child views are shown.
bound.set_height(contents_view->GetPreferredSize().height());
widget_->SetBounds(bound);

widget_->Show();
}
widget_->Show();
}

InSessionAuthDialog::~InSessionAuthDialog() = default;
Expand Down
8 changes: 4 additions & 4 deletions ash/in_session_auth/in_session_auth_dialog_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

#include "ash/in_session_auth/in_session_auth_dialog_controller_impl.h"

#include "ash/in_session_auth/auth_dialog_debug_view.h"
#include "ash/in_session_auth/auth_dialog_contents_view.h"
#include "ash/public/cpp/in_session_auth_dialog_client.h"
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
Expand Down Expand Up @@ -36,10 +36,10 @@ void InSessionAuthDialogControllerImpl::ShowAuthenticationDialog(
AccountId account_id =
Shell::Get()->session_controller()->GetActiveAccountId();
// Password should always be available.
uint32_t auth_methods = AuthDialogDebugView::kAuthPassword;
uint32_t auth_methods = AuthDialogContentsView::kAuthPassword;

if (client_->IsFingerprintAuthAvailable(account_id))
auth_methods |= AuthDialogDebugView::kAuthFingerprint;
auth_methods |= AuthDialogContentsView::kAuthFingerprint;

client_->CheckPinAuthAvailability(
account_id,
Expand All @@ -51,7 +51,7 @@ void InSessionAuthDialogControllerImpl::OnPinCanAuthenticate(
uint32_t auth_methods,
bool pin_auth_available) {
if (pin_auth_available)
auth_methods |= AuthDialogDebugView::kAuthPin;
auth_methods |= AuthDialogContentsView::kAuthPin;

dialog_ = std::make_unique<InSessionAuthDialog>(auth_methods);
}
Expand Down
5 changes: 0 additions & 5 deletions chromeos/constants/chromeos_switches.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,11 +526,6 @@ const char kHideAndroidFilesInFilesApp[] = "hide-android-files-in-files-app";
// "/usr/share/chromeos-assets/regulatory_labels/".
const char kRegulatoryLabelDir[] = "regulatory-label-dir";

// If true, the debug view overlay will be shown for the in-session
// authentication dialog. This enables development of the dialog.
// TODO(yichenli): Remove this after the feature is released.
const char kShowAuthDialogDevOverlay[] = "show-auth-dialog-dev-overlay";

// If true, the developer tool overlay will be shown for the login/lock screen.
// This makes it easier to test layout logic.
const char kShowLoginDevOverlay[] = "show-login-dev-overlay";
Expand Down
2 changes: 0 additions & 2 deletions chromeos/constants/chromeos_switches.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,6 @@ COMPONENT_EXPORT(CHROMEOS_CONSTANTS) extern const char kShelfHoverPreviews[];
COMPONENT_EXPORT(CHROMEOS_CONSTANTS) extern const char kShelfHotseat[];
COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
extern const char kShowAndroidFilesInFilesApp[];
COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
extern const char kShowAuthDialogDevOverlay[];
COMPONENT_EXPORT(CHROMEOS_CONSTANTS) extern const char kShowLoginDevOverlay[];
COMPONENT_EXPORT(CHROMEOS_CONSTANTS) extern const char kShowOobeDevOverlay[];
COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
Expand Down

0 comments on commit 9969572

Please sign in to comment.