webauthn: tidy up many TODO(enclave)s.

TODO(enclave) should be gone by the M128 branchpoint: either resolved or else converted into `TODO(crbug.com/xyz)` for longer-term things. Many of these have already been resolved and can just be deleted. Change-Id: I6ba8de479c0ec3047b324e67f8869bc06f893550 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5693040 Reviewed-by: Ken Buchanan <[email protected]> Commit-Queue: Adam Langley <[email protected]> Cr-Commit-Position: refs/heads/main@{#1326353}
azx1573 · Jul 11, 2024 · 5f70542 · 5f70542
1 parent 07c5f9e
commit 5f70542
Show file tree

Hide file tree

Showing 6 changed files with 7 additions and 22 deletions.
diff --git a/chrome/browser/ui/views/webauthn/authenticator_request_dialog_view.cc b/chrome/browser/ui/views/webauthn/authenticator_request_dialog_view.cc
@@ -57,22 +57,6 @@ AuthenticatorRequestDialogView::~AuthenticatorRequestDialogView() {
   if (model_) {
     model_->observers.RemoveObserver(this);
   }
-
-  // TODO(enclave): the below comment hasn't been true for some time. It can
-  // probably be removed, but we didn't want to remove it in a refactoring CL.
-
-  // AuthenticatorRequestDialogView is a WidgetDelegate, owned by views::Widget.
-  // It's only destroyed by Widget::OnNativeWidgetDestroyed() invoking
-  // DeleteDelegate(), and because WIDGET_OWNS_NATIVE_WIDGET, ~Widget() is
-  // invoked straight after, which destroys child views. views::View subclasses
-  // shouldn't be doing anything interesting in their destructors, so it should
-  // be okay to destroy the |sheet_| immediately after this line.
-  //
-  // However, as AuthenticatorRequestDialogModel is owned by |this|, and
-  // ObservableAuthenticatorList is owned by
-  // AuthenticatorRequestDialogModel, destroy all view components that
-  // might own models observing the list prior to destroying
-  // AuthenticatorRequestDialogModel.
   RemoveAllChildViews();
 }
 

diff --git a/chrome/browser/webauthn/authenticator_request_dialog_model.cc b/chrome/browser/webauthn/authenticator_request_dialog_model.cc
@@ -691,6 +691,7 @@ void AuthenticatorRequestDialogController::StartFlow(
 void AuthenticatorRequestDialogController::StartOver() {
   if (model_->step() == Step::kTrustThisComputerCreation ||
       model_->step() == Step::kTrustThisComputerAssertion) {
+    device::enclave::RecordEvent(device::enclave::Event::kOnboardingRejected);
     auto* pref_service = Profile::FromBrowserContext(
                              model_->GetRenderFrameHost()->GetBrowserContext())
                              ->GetOriginalProfile()
@@ -2245,7 +2246,7 @@ void AuthenticatorRequestDialogController::PopulateMechanisms() {
   if (enclave_needs_reauth_ && !use_conditional_mediation_) {
     // Show a button that lets the user sign in again to restore sync. This
     // cancels the request, so we can't do it for conditional UI requests.
-    // TODO(enclave): add support for conditional UI.
+    // TODO(crbug.com/345413738): add support for conditional UI.
     const std::u16string name =
         l10n_util::GetStringUTF16(IDS_WEBAUTHN_SIGN_IN_AGAIN_TITLE);
     Mechanism enclave(

diff --git a/chrome/browser/webauthn/change_pin_controller_impl.cc b/chrome/browser/webauthn/change_pin_controller_impl.cc
@@ -73,7 +73,6 @@ void ChangePinControllerImpl::StartChangePin(SuccessCallback callback) {
     return;
   }
   notify_pin_change_callback_ = std::move(callback);
-  // TODO(enclave): use local UV instead of GPM reauth when available.
   model_->SetStep(Step::kGPMReauthForPinReset);
   RecordHistogram(ChangePinEvent::kFlowStartedFromSettings);
 }

diff --git a/chrome/browser/webauthn/enclave_authenticator_browsertest.cc b/chrome/browser/webauthn/enclave_authenticator_browsertest.cc
@@ -2078,8 +2078,8 @@ class EnclaveAuthenticatorWithoutPinBrowserTest
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-// TODO(enclave): re-enable these tests, probably by overriding webauthn.dll in
-// the test class.
+// Without a Windows-on-ARM device we've been unable to debug why these
+// tests fail in that that context.
 #if BUILDFLAG(IS_WIN) && defined(ARCH_CPU_ARM64)
 #define MAYBE_NotAvailableWithoutUV DISABLED_NotAvailableWithoutUV
 #else

diff --git a/chrome/browser/webauthn/gpm_enclave_controller.cc b/chrome/browser/webauthn/gpm_enclave_controller.cc
@@ -869,6 +869,7 @@ void GPMEnclaveController::OnGPMSelected() {
 
     case AccountState::kRecoverable:
     case AccountState::kIrrecoverable:
+      device::enclave::RecordEvent(device::enclave::Event::kOnboarding);
       model_->SetStep(Step::kTrustThisComputerCreation);
       break;
 
@@ -929,6 +930,7 @@ void GPMEnclaveController::OnGPMPasskeySelected(
     case AccountState::kRecoverable:
     case AccountState::kIrrecoverable:
       if (model_->priority_phone_name.has_value()) {
+        device::enclave::RecordEvent(device::enclave::Event::kOnboarding);
         model_->SetStep(Step::kTrustThisComputerAssertion);
       } else {
         model_->SetStep(Step::kRecoverSecurityDomain);
@@ -999,6 +1001,7 @@ void GPMEnclaveController::OnGpmPinChanged(bool success) {
 void GPMEnclaveController::OnTrustThisComputer() {
   CHECK(model_->step() == Step::kTrustThisComputerAssertion ||
         model_->step() == Step::kTrustThisComputerCreation);
+  device::enclave::RecordEvent(device::enclave::Event::kOnboardingAccepted);
   // Clicking through the bootstrapping dialog resets the count even if it
   // doesn't end up being successful.
   ResetDeclinedBootstrappingCount(model_.get());

diff --git a/device/fido/enclave/metrics.h b/device/fido/enclave/metrics.h
@@ -12,8 +12,6 @@ namespace device::enclave {
 // A list of enclave-related events that are reported to UMA. Do not renumber
 // as the values are persisted.
 enum class Event {
-  // TODO(enclave): Decide whether onboarding events should be recorded in the
-  // GPM pin onboarding view since the original onboarding view was removed.
   kOnboarding = 0,
   kOnboardingRejected = 1,
   kOnboardingAccepted = 2,