FingerprintSetupScreen: Skip screen properly.

Use new method for skipping screen. Add tests for UMA stats.

Bug: 1064561
Change-Id: Ieabfdab904e6e83501a3188a3ff8d4184da0bb10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2142264
Commit-Queue: Roman Aleksandrov <raleksandrov@google.com>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758226}

chrome/browser/chromeos/login/screens/fingerprint_setup_browsertest.cc

@@ -33,6 +33,7 @@ class FingerprintSetupTest : public OobeBaseTest {
     // Override the screen exit callback with our own method.
     FingerprintSetupScreen* fingerprint_screen = FingerprintSetupScreen::Get(
         WizardController::default_controller()->screen_manager());
+    original_callback_ = fingerprint_screen->get_exit_callback_for_testing();
     fingerprint_screen->set_exit_callback_for_testing(
         base::BindRepeating(&FingerprintSetupTest::OnFingerprintSetupScreenExit,
                             base::Unretained(this)));
@@ -55,8 +56,10 @@ class FingerprintSetupTest : public OobeBaseTest {
     run_loop.Run();
   }
 
-  void OnFingerprintSetupScreenExit() {
+  void OnFingerprintSetupScreenExit(FingerprintSetupScreen::Result result) {
     screen_exit_ = true;
+    screen_result_ = result;
+    original_callback_.Run(result);
     if (screen_exit_callback_) {
       std::move(screen_exit_callback_).Run();
     }
@@ -84,8 +87,12 @@ class FingerprintSetupTest : public OobeBaseTest {
         {"fingerprint-setup-impl", "fingerprintAddAnother"});
   }
 
+  base::Optional<FingerprintSetupScreen::Result> screen_result_;
+  base::HistogramTester histogram_tester_;
+
  private:
   bool screen_exit_ = false;
+  FingerprintSetupScreen::ScreenExitCallback original_callback_;
   base::RepeatingClosure screen_exit_callback_;
 };
 
@@ -104,6 +111,11 @@ IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintEnrollHalf) {
   test::OobeJS().TapOnPath({"fingerprint-setup-impl", "skipFingerprintEnroll"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), FingerprintSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Fingerprint-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Fingerprint-setup", 1);
 }
 
 IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintEnrollFull) {
@@ -115,6 +127,11 @@ IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintEnrollFull) {
   test::OobeJS().TapOnPath({"fingerprint-setup-impl", "fingerprintEnrollDone"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), FingerprintSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Fingerprint-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Fingerprint-setup", 1);
 }
 
 IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintEnrollLimit) {
@@ -133,6 +150,11 @@ IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintEnrollLimit) {
   test::OobeJS().TapOnPath({"fingerprint-setup-impl", "fingerprintEnrollDone"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), FingerprintSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Fingerprint-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Fingerprint-setup", 1);
 }
 
 IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintDisabled) {
@@ -143,6 +165,12 @@ IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintDisabled) {
       FingerprintSetupScreenView::kScreenId);
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            FingerprintSetupScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Fingerprint-setup.Next", 0);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Fingerprint-setup", 0);
 }
 
 IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintSetupScreenElements) {
@@ -161,6 +189,11 @@ IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintSetupCancel) {
   test::OobeJS().TapOnPath({"fingerprint-setup-impl", "skipFingerprintSetup"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), FingerprintSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Fingerprint-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Fingerprint-setup", 1);
 }
 
 IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintSetupNext) {
@@ -192,6 +225,11 @@ IN_PROC_BROWSER_TEST_F(FingerprintSetupTest, FingerprintSetupLater) {
   test::OobeJS().TapOnPath({"fingerprint-setup-impl", "setupFingerprintLater"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), FingerprintSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Fingerprint-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Fingerprint-setup", 1);
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/screens/fingerprint_setup_screen.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/screens/fingerprint_setup_screen.h"
+
 #include "chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -15,6 +16,16 @@ constexpr char kUserActionClose[] = "fingerprint-setup-done";
 
 }  // namespace
 
+// static
+std::string FingerprintSetupScreen::GetResultString(Result result) {
+  switch (result) {
+    case Result::NEXT:
+      return "Next";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
+  }
+}
+
 FingerprintSetupScreen* FingerprintSetupScreen::Get(ScreenManager* manager) {
   return static_cast<FingerprintSetupScreen*>(
       manager->GetScreen(FingerprintSetupScreenView::kScreenId));
@@ -22,7 +33,7 @@ FingerprintSetupScreen* FingerprintSetupScreen::Get(ScreenManager* manager) {
 
 FingerprintSetupScreen::FingerprintSetupScreen(
     FingerprintSetupScreenView* view,
-    const base::RepeatingClosure& exit_callback)
+    const ScreenExitCallback& exit_callback)
     : BaseScreen(FingerprintSetupScreenView::kScreenId,
                  OobeScreenPriority::DEFAULT),
       view_(view),
@@ -35,13 +46,17 @@ FingerprintSetupScreen::~FingerprintSetupScreen() {
   view_->Bind(nullptr);
 }
 
-void FingerprintSetupScreen::ShowImpl() {
+bool FingerprintSetupScreen::MaybeSkip() {
   if (!chromeos::quick_unlock::IsFingerprintEnabled(
           ProfileManager::GetActiveUserProfile()) ||
       chrome_user_manager_util::IsPublicSessionOrEphemeralLogin()) {
-    exit_callback_.Run();
-    return;
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
   }
+  return false;
+}
+
+void FingerprintSetupScreen::ShowImpl() {
   view_->Show();
 }
 
@@ -51,7 +66,7 @@ void FingerprintSetupScreen::HideImpl() {
 
 void FingerprintSetupScreen::OnUserAction(const std::string& action_id) {
   if (action_id == kUserActionClose) {
-    exit_callback_.Run();
+    exit_callback_.Run(Result::NEXT);
     return;
   }
   BaseScreen::OnUserAction(action_id);

chrome/browser/chromeos/login/screens/fingerprint_setup_screen.h

@@ -20,17 +20,28 @@ class FingerprintSetupScreenView;
 // user to enroll fingerprint on the device.
 class FingerprintSetupScreen : public BaseScreen {
  public:
+  enum class Result { NEXT, NOT_APPLICABLE };
+
+  static std::string GetResultString(Result result);
+
+  using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
   FingerprintSetupScreen(FingerprintSetupScreenView* view,
-                         const base::RepeatingClosure& exit_callback);
+                         const ScreenExitCallback& exit_callback);
   ~FingerprintSetupScreen() override;
 
   static FingerprintSetupScreen* Get(ScreenManager* manager);
 
-  void set_exit_callback_for_testing(
-      const base::RepeatingClosure& exit_callback) {
+  void set_exit_callback_for_testing(const ScreenExitCallback& exit_callback) {
     exit_callback_ = exit_callback;
   }
 
+  const ScreenExitCallback& get_exit_callback_for_testing() {
+    return exit_callback_;
+  }
+
+  // BaseScreen:
+  bool MaybeSkip() override;
+
  protected:
   // BaseScreen:
   void ShowImpl() override;
@@ -39,7 +50,7 @@ class FingerprintSetupScreen : public BaseScreen {
 
  private:
   FingerprintSetupScreenView* const view_;
-  base::RepeatingClosure exit_callback_;
+  ScreenExitCallback exit_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FingerprintSetupScreen);
 };

chrome/browser/chromeos/login/wizard_controller.cc

@@ -1124,8 +1124,10 @@ void WizardController::OnSyncConsentFinished() {
   ShowFingerprintSetupScreen();
 }
 
-void WizardController::OnFingerprintSetupScreenExit() {
-  OnScreenExit(FingerprintSetupScreenView::kScreenId, kDefaultExitReason);
+void WizardController::OnFingerprintSetupScreenExit(
+    FingerprintSetupScreen::Result result) {
+  OnScreenExit(FingerprintSetupScreenView::kScreenId,
+               FingerprintSetupScreen::GetResultString(result));
 
   ShowDiscoverScreen();
 }

chrome/browser/chromeos/login/wizard_controller.h

@@ -28,6 +28,7 @@
 #include "chrome/browser/chromeos/login/screens/enable_adb_sideloading_screen.h"
 #include "chrome/browser/chromeos/login/screens/enable_debugging_screen.h"
 #include "chrome/browser/chromeos/login/screens/eula_screen.h"
+#include "chrome/browser/chromeos/login/screens/fingerprint_setup_screen.h"
 #include "chrome/browser/chromeos/login/screens/gesture_navigation_screen.h"
 #include "chrome/browser/chromeos/login/screens/kiosk_autolaunch_screen.h"
 #include "chrome/browser/chromeos/login/screens/network_screen.h"
@@ -240,7 +241,7 @@ class WizardController {
   void OnTermsOfServiceAccepted();
   void OnSyncConsentScreenExit();
   void OnSyncConsentFinished();
-  void OnFingerprintSetupScreenExit();
+  void OnFingerprintSetupScreenExit(FingerprintSetupScreen::Result result);
   void OnDiscoverScreenExit();
   void OnArcTermsOfServiceScreenExit(ArcTermsOfServiceScreen::Result result);
   void OnArcTermsOfServiceSkipped();