From 996957206c76dcd39908903f7c18d2caf944f629 Mon Sep 17 00:00:00 2001 From: Yicheng Li Date: Fri, 14 Aug 2020 01:24:49 +0000 Subject: [PATCH] ash: Promote AuthDialogDebugView to be default view 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 Reviewed-by: Xiyuan Xia Cr-Commit-Position: refs/heads/master@{#797959} --- ash/BUILD.gn | 4 +- ...g_view.cc => auth_dialog_contents_view.cc} | 50 ++++++++++--------- ...bug_view.h => auth_dialog_contents_view.h} | 19 +++---- ash/in_session_auth/in_session_auth_dialog.cc | 22 ++++---- .../in_session_auth_dialog_controller_impl.cc | 8 +-- chromeos/constants/chromeos_switches.cc | 5 -- chromeos/constants/chromeos_switches.h | 2 - 7 files changed, 51 insertions(+), 59 deletions(-) rename ash/in_session_auth/{auth_dialog_debug_view.cc => auth_dialog_contents_view.cc} (85%) rename ash/in_session_auth/{auth_dialog_debug_view.h => auth_dialog_contents_view.h} (82%) diff --git a/ash/BUILD.gn b/ash/BUILD.gn index ea6189884c9f2e..82a9a953279ac8 100644 --- a/ash/BUILD.gn +++ b/ash/BUILD.gn @@ -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", diff --git a/ash/in_session_auth/auth_dialog_debug_view.cc b/ash/in_session_auth/auth_dialog_contents_view.cc similarity index 85% rename from ash/in_session_auth/auth_dialog_debug_view.cc rename to ash/in_session_auth/auth_dialog_contents_view.cc index ec683198dd61fc..1f6defad6ad34d 100644 --- a/ash/in_session_auth/auth_dialog_debug_view.cc +++ b/ash/in_session_auth/auth_dialog_contents_view.cc @@ -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 #include @@ -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 @@ -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)); @@ -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); @@ -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()); title_->SetEnabledColor(SK_ColorBLACK); title_->SetSubpixelRenderingEnabled(false); @@ -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()); prompt_->SetEnabledColor(SK_ColorBLACK); prompt_->SetSubpixelRenderingEnabled(false); @@ -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(CreateInSessionAuthPalette())); @@ -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::Style::kAlphanumeric, CreateInSessionAuthPalette(), base::BindRepeating(&LoginPasswordView::InsertNumber, @@ -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()); spacing->SetPreferredSize(gfx::Size(kContainerPreferredWidth, height)); } -void AuthDialogDebugView::AddActionButtonsView() { +void AuthDialogContentsView::AddActionButtonsView() { action_view_container_ = container_->AddChildView(std::make_unique()); auto* buttons_layout = action_view_container_->SetLayoutManager( @@ -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(); @@ -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(this, base::ASCIIToUTF16(text)); @@ -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 success) {} +void AuthDialogContentsView::OnAuthComplete(base::Optional success) {} } // namespace ash diff --git a/ash/in_session_auth/auth_dialog_debug_view.h b/ash/in_session_auth/auth_dialog_contents_view.h similarity index 82% rename from ash/in_session_auth/auth_dialog_debug_view.h rename to ash/in_session_auth/auth_dialog_contents_view.h index 79de4bad9d3408..b4a677977e0198 100644 --- a/ash/in_session_auth/auth_dialog_debug_view.h +++ b/ash/in_session_auth/auth_dialog_contents_view.h @@ -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 @@ -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 { @@ -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; @@ -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 weak_factory_{this}; + base::WeakPtrFactory 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_ diff --git a/ash/in_session_auth/in_session_auth_dialog.cc b/ash/in_session_auth/in_session_auth_dialog.cc index 5a3174ecbe9e5f..0cfde1b0500135 100644 --- a/ash/in_session_auth/in_session_auth_dialog.cc +++ b/ash/in_session_auth/in_session_auth_dialog.cc @@ -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" @@ -57,18 +56,15 @@ std::unique_ptr 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(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(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; diff --git a/ash/in_session_auth/in_session_auth_dialog_controller_impl.cc b/ash/in_session_auth/in_session_auth_dialog_controller_impl.cc index 7536bde78b6d4a..3c9a39204079f5 100644 --- a/ash/in_session_auth/in_session_auth_dialog_controller_impl.cc +++ b/ash/in_session_auth/in_session_auth_dialog_controller_impl.cc @@ -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" @@ -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, @@ -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(auth_methods); } diff --git a/chromeos/constants/chromeos_switches.cc b/chromeos/constants/chromeos_switches.cc index b6b445a32ed562..12b8f535c915e9 100644 --- a/chromeos/constants/chromeos_switches.cc +++ b/chromeos/constants/chromeos_switches.cc @@ -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"; diff --git a/chromeos/constants/chromeos_switches.h b/chromeos/constants/chromeos_switches.h index f471952038e6d1..a876e43e98b936 100644 --- a/chromeos/constants/chromeos_switches.h +++ b/chromeos/constants/chromeos_switches.h @@ -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)